Re: [PATCH 11/13] staging: most: net: fix race between create/destroy device

[Greg KH]

On Fri, May 12, 2017 at 12:59:59PM +0200, Christian Gromm wrote:
> From: Andrey Shvetsov 
> 
> This introduces the kref for the net_dev_context to prevent the
> destruction of the network devices that are in use.
> 
> Each get_net_dev_context is completed with the put_net_dev_context,
> except the function aim_probe_channel that calls one more
> get_net_dev_context or kref_get and the function aim_disconnect_channel
> that calls one more put_net_dev_context.
> 
> Signed-off-by: Andrey Shvetsov 
> Signed-off-by: Christian Gromm 
> ---
>  drivers/staging/most/aim-network/networking.c | 92 
> +--
>  1 file changed, 72 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/most/aim-network/networking.c 
> b/drivers/staging/most/aim-network/networking.c
> index cbd9500..8cf1c81 100644
> --- a/drivers/staging/most/aim-network/networking.c
> +++ b/drivers/staging/most/aim-network/networking.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "mostcore.h"
>  
>  #define MEP_HDR_LEN 8
> @@ -69,6 +70,7 @@ struct net_dev_context {
>   struct net_dev_channel rx;
>   struct net_dev_channel tx;
>   struct list_head list;
> + struct kref kref;
>  };
>  
>  static struct list_head net_devices = LIST_HEAD_INIT(net_devices);
> @@ -268,6 +270,26 @@ static void most_nd_setup(struct net_device *dev)
>   dev->netdev_ops = _nd_ops;
>  }
>  
> +static void release_nd(struct kref *kref)
> +{
> + struct net_dev_context *nd;
> +
> + nd = container_of(kref, struct net_dev_context, kref);
> + list_del(>list);

That might delete something off of the list, but it does not free the
structure itself, where is that handled?

Why do you need a reference count for all of this?  Doesn't the
networking core provide you the needed functions for this?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 11/13] staging: most: net: fix race between create/destroy device

[Christian Gromm]

From: Andrey Shvetsov 

This introduces the kref for the net_dev_context to prevent the
destruction of the network devices that are in use.

Each get_net_dev_context is completed with the put_net_dev_context,
except the function aim_probe_channel that calls one more
get_net_dev_context or kref_get and the function aim_disconnect_channel
that calls one more put_net_dev_context.

Signed-off-by: Andrey Shvetsov 
Signed-off-by: Christian Gromm 
---
 drivers/staging/most/aim-network/networking.c | 92 +--
 1 file changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/most/aim-network/networking.c 
b/drivers/staging/most/aim-network/networking.c
index cbd9500..8cf1c81 100644
--- a/drivers/staging/most/aim-network/networking.c
+++ b/drivers/staging/most/aim-network/networking.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "mostcore.h"
 
 #define MEP_HDR_LEN 8
@@ -69,6 +70,7 @@ struct net_dev_context {
struct net_dev_channel rx;
struct net_dev_channel tx;
struct list_head list;
+   struct kref kref;
 };
 
 static struct list_head net_devices = LIST_HEAD_INIT(net_devices);
@@ -268,6 +270,26 @@ static void most_nd_setup(struct net_device *dev)
dev->netdev_ops = _nd_ops;
 }
 
+static void release_nd(struct kref *kref)
+{
+   struct net_dev_context *nd;
+
+   nd = container_of(kref, struct net_dev_context, kref);
+   list_del(>list);
+}
+
+static inline void put_net_dev_context(struct net_dev_context *nd)
+{
+   unsigned long flags;
+   int released;
+
+   spin_lock_irqsave(_lock, flags);
+   released = kref_put(>kref, release_nd);
+   spin_unlock_irqrestore(_lock, flags);
+   if (released)
+   free_netdev(nd->dev);
+}
+
 static struct net_dev_context *get_net_dev_context(
struct most_interface *iface)
 {
@@ -277,6 +299,7 @@ static struct net_dev_context *get_net_dev_context(
spin_lock_irqsave(_lock, flags);
list_for_each_entry(nd, _devices, list) {
if (nd->iface == iface) {
+   kref_get(>kref);
spin_unlock_irqrestore(_lock, flags);
return nd;
}
@@ -300,7 +323,6 @@ static int aim_probe_channel(struct most_interface *iface, 
int channel_idx,
return -EINVAL;
 
nd = get_net_dev_context(iface);
-
if (!nd) {
struct net_device *dev;
 
@@ -309,19 +331,34 @@ static int aim_probe_channel(struct most_interface 
*iface, int channel_idx,
if (!dev)
return -ENOMEM;
 
+   /*
+* The network device for the given iface may be added with use
+* of the other channel just after the get_net_dev_context
+* call.  Free our duplicate of net_device in this case.
+*/
+   spin_lock_irqsave(_lock, flags);
+   list_for_each_entry(nd, _devices, list) {
+   if (nd->iface == iface) {
+   kref_get(>kref);
+   spin_unlock_irqrestore(_lock, flags);
+   free_netdev(dev);
+   goto ok;
+   }
+   }
+
nd = netdev_priv(dev);
+   kref_init(>kref);
nd->iface = iface;
nd->dev = dev;
-
-   spin_lock_irqsave(_lock, flags);
list_add(>list, _devices);
spin_unlock_irqrestore(_lock, flags);
}
 
+ok:
ch = ccfg->direction == MOST_CH_TX ? >tx : >rx;
if (ch->linked) {
pr_err("only one channel per instance & direction allowed\n");
-   return -EINVAL;
+   goto err;
}
 
ch->ch_id = channel_idx;
@@ -329,10 +366,14 @@ static int aim_probe_channel(struct most_interface 
*iface, int channel_idx,
if (nd->tx.linked && nd->rx.linked && register_netdev(nd->dev)) {
pr_err("register_netdev() failed\n");
ch->linked = false;
-   return -EINVAL;
+   goto err;
}
 
return 0;
+
+err:
+   put_net_dev_context(nd);
+   return -EINVAL;
 }
 
 static int aim_disconnect_channel(struct most_interface *iface,
@@ -340,7 +381,7 @@ static int aim_disconnect_channel(struct most_interface 
*iface,
 {
struct net_dev_context *nd;
struct net_dev_channel *ch;
-   unsigned long flags;
+   int ret = -EINVAL;
 
nd = get_net_dev_context(iface);
if (!nd)
@@ -351,7 +392,7 @@ static int aim_disconnect_channel(struct most_interface 
*iface,
else if (nd->tx.linked && channel_idx == nd->tx.ch_id)
ch = >tx;
else
-   return -EINVAL;
+   goto put_nd;
 
/*
   

staging: most: net: fix race between create/destroy device

[Christian Gromm]

From: Andrey Shvetsov 

This introduces the kref for the net_dev_context to prevent the
destruction of the network devices that are in use.

Each get_net_dev_context is completed with the put_net_dev_context,
except the function aim_probe_channel that calls one more
get_net_dev_context or kref_get and the function aim_disconnect_channel
that calls one more put_net_dev_context.

Signed-off-by: Andrey Shvetsov 
Signed-off-by: Christian Gromm 
---
 drivers/staging/most/aim-network/networking.c | 92 +--
 1 file changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/most/aim-network/networking.c 
b/drivers/staging/most/aim-network/networking.c
index cbd95003ec31..8cf1c81c295a 100644
--- a/drivers/staging/most/aim-network/networking.c
+++ b/drivers/staging/most/aim-network/networking.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "mostcore.h"
 
 #define MEP_HDR_LEN 8
@@ -69,6 +70,7 @@ struct net_dev_context {
struct net_dev_channel rx;
struct net_dev_channel tx;
struct list_head list;
+   struct kref kref;
 };
 
 static struct list_head net_devices = LIST_HEAD_INIT(net_devices);
@@ -268,6 +270,26 @@ static void most_nd_setup(struct net_device *dev)
dev->netdev_ops = _nd_ops;
 }
 
+static void release_nd(struct kref *kref)
+{
+   struct net_dev_context *nd;
+
+   nd = container_of(kref, struct net_dev_context, kref);
+   list_del(>list);
+}
+
+static inline void put_net_dev_context(struct net_dev_context *nd)
+{
+   unsigned long flags;
+   int released;
+
+   spin_lock_irqsave(_lock, flags);
+   released = kref_put(>kref, release_nd);
+   spin_unlock_irqrestore(_lock, flags);
+   if (released)
+   free_netdev(nd->dev);
+}
+
 static struct net_dev_context *get_net_dev_context(
struct most_interface *iface)
 {
@@ -277,6 +299,7 @@ static struct net_dev_context *get_net_dev_context(
spin_lock_irqsave(_lock, flags);
list_for_each_entry(nd, _devices, list) {
if (nd->iface == iface) {
+   kref_get(>kref);
spin_unlock_irqrestore(_lock, flags);
return nd;
}
@@ -300,7 +323,6 @@ static int aim_probe_channel(struct most_interface *iface, 
int channel_idx,
return -EINVAL;
 
nd = get_net_dev_context(iface);
-
if (!nd) {
struct net_device *dev;
 
@@ -309,19 +331,34 @@ static int aim_probe_channel(struct most_interface 
*iface, int channel_idx,
if (!dev)
return -ENOMEM;
 
+   /*
+* The network device for the given iface may be added with use
+* of the other channel just after the get_net_dev_context
+* call.  Free our duplicate of net_device in this case.
+*/
+   spin_lock_irqsave(_lock, flags);
+   list_for_each_entry(nd, _devices, list) {
+   if (nd->iface == iface) {
+   kref_get(>kref);
+   spin_unlock_irqrestore(_lock, flags);
+   free_netdev(dev);
+   goto ok;
+   }
+   }
+
nd = netdev_priv(dev);
+   kref_init(>kref);
nd->iface = iface;
nd->dev = dev;
-
-   spin_lock_irqsave(_lock, flags);
list_add(>list, _devices);
spin_unlock_irqrestore(_lock, flags);
}
 
+ok:
ch = ccfg->direction == MOST_CH_TX ? >tx : >rx;
if (ch->linked) {
pr_err("only one channel per instance & direction allowed\n");
-   return -EINVAL;
+   goto err;
}
 
ch->ch_id = channel_idx;
@@ -329,10 +366,14 @@ static int aim_probe_channel(struct most_interface 
*iface, int channel_idx,
if (nd->tx.linked && nd->rx.linked && register_netdev(nd->dev)) {
pr_err("register_netdev() failed\n");
ch->linked = false;
-   return -EINVAL;
+   goto err;
}
 
return 0;
+
+err:
+   put_net_dev_context(nd);
+   return -EINVAL;
 }
 
 static int aim_disconnect_channel(struct most_interface *iface,
@@ -340,7 +381,7 @@ static int aim_disconnect_channel(struct most_interface 
*iface,
 {
struct net_dev_context *nd;
struct net_dev_channel *ch;
-   unsigned long flags;
+   int ret = -EINVAL;
 
nd = get_net_dev_context(iface);
if (!nd)
@@ -351,7 +392,7 @@ static int aim_disconnect_channel(struct most_interface 
*iface,
else if (nd->tx.linked && channel_idx == nd->tx.ch_id)
ch = >tx;
else
-   return -EINVAL;
+   goto put_nd;