* [PATCH] n_gsm: Add Mutex to avoid race when net destroy @ 2013-02-28 5:31 channing 2013-02-28 9:53 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: channing @ 2013-02-28 5:31 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-kernel when gsm Net is enabled, data on dlci is transferrd by gsm_mux_net_start_xmit(), while userspace may trigger ioctrl to call gsm_destroy_network() during data was transferring, because there is no mutex protection between the two functions, following scenario may happen: 1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net); 2) gsm_destroy_network() is called from ioctrl, and it will not call net_free() to release net device because net device is still referred in step 1) 3) continue execute step 1), gsm_mux_net_start_xmit() calls muxnet_put(mux_net), and then calls net_free() to release net device. 4) if userspace triggers gsm_create_network() at same time with net_free() in step 3). it will hit race on dlci->net. This patch is to add a mutex in tx function to avoid race between it and destroy function. Signed-off-by: Chao Bi <chao.bi@intel.com> Signed-off-by: Pillet Vincent <vincentx.pillet@intel.com> --- drivers/tty/n_gsm.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 4a43ef5..0ca810a 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb, { struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net); struct gsm_dlci *dlci = mux_net->dlci; + mutex_lock(&dlci->mutex); muxnet_get(mux_net); skb_queue_head(&dlci->skb_list, skb); @@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb, /* And tell the kernel when the last transmit started. */ net->trans_start = jiffies; muxnet_put(mux_net); + mutex_unlock(&dlci->mutex); return NETDEV_TX_OK; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] n_gsm: Add Mutex to avoid race when net destroy 2013-02-28 5:31 [PATCH] n_gsm: Add Mutex to avoid race when net destroy channing @ 2013-02-28 9:53 ` Jiri Slaby 2013-03-01 8:51 ` channing 0 siblings, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2013-02-28 9:53 UTC (permalink / raw) To: channing, Greg Kroah-Hartman; +Cc: linux-kernel, ML netdev On 02/28/2013 06:31 AM, channing wrote: > > when gsm Net is enabled, data on dlci is transferrd by > gsm_mux_net_start_xmit(), while userspace may trigger > ioctrl to call gsm_destroy_network() during data was > transferring, because there is no mutex protection between > the two functions, following scenario may happen: > > 1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net); > 2) gsm_destroy_network() is called from ioctrl, and it > will not call net_free() to release net device because > net device is still referred in step 1) > 3) continue execute step 1), gsm_mux_net_start_xmit() > calls muxnet_put(mux_net), and then calls net_free() to > release net device. > 4) if userspace triggers gsm_create_network() at same time > with net_free() in step 3). it will hit race on dlci->net. > > This patch is to add a mutex in tx function to avoid race > between it and destroy function. > > Signed-off-by: Chao Bi <chao.bi@intel.com> > Signed-off-by: Pillet Vincent <vincentx.pillet@intel.com> > --- > drivers/tty/n_gsm.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c > index 4a43ef5..0ca810a 100644 > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb, > { > struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net); > struct gsm_dlci *dlci = mux_net->dlci; > + mutex_lock(&dlci->mutex); Nack, start_xmit may be called in an atomic context -- you cannot call mutex. > muxnet_get(mux_net); > > skb_queue_head(&dlci->skb_list, skb); > @@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb, > /* And tell the kernel when the last transmit started. */ > net->trans_start = jiffies; > muxnet_put(mux_net); Instead the concept is broken. If this was the last reference (as described in your steps above), it would blow up for the same reason I refer to above, i.e. net_free here would call unregister_netdev which is not atomic. Plus it will definitely deadlock because unregister_netdev waits for start_xmit to finish. It should stop the queue and schedule a workqueue to lock the mutex, unregister the hetdev and reset dlci->net. (Or maybe just call muxnet_put with the lock held.) That will fix 4), but there is still a bug: what protects gsm_create_network to be called twice or more in a sequence thus re-setting dlci->net to a new and new pointer? > + mutex_unlock(&dlci->mutex); > return NETDEV_TX_OK; > } thanks, -- js suse labs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] n_gsm: Add Mutex to avoid race when net destroy 2013-02-28 9:53 ` Jiri Slaby @ 2013-03-01 8:51 ` channing 2013-03-01 9:10 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: channing @ 2013-03-01 8:51 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-kernel, ML netdev, vincentx.pillet On Thu, 2013-02-28 at 10:53 +0100, Jiri Slaby wrote: > On 02/28/2013 06:31 AM, channing wrote: > > > > when gsm Net is enabled, data on dlci is transferrd by > > gsm_mux_net_start_xmit(), while userspace may trigger > > ioctrl to call gsm_destroy_network() during data was > > transferring, because there is no mutex protection between > > the two functions, following scenario may happen: > > > > 1) gsm_mux_net_start_xmit() calls muxnet_get(mux_net); > > 2) gsm_destroy_network() is called from ioctrl, and it > > will not call net_free() to release net device because > > net device is still referred in step 1) > > 3) continue execute step 1), gsm_mux_net_start_xmit() > > calls muxnet_put(mux_net), and then calls net_free() to > > release net device. > > 4) if userspace triggers gsm_create_network() at same time > > with net_free() in step 3). it will hit race on dlci->net. > > > > This patch is to add a mutex in tx function to avoid race > > between it and destroy function. > > > > Signed-off-by: Chao Bi <chao.bi@intel.com> > > Signed-off-by: Pillet Vincent <vincentx.pillet@intel.com> > > --- > > drivers/tty/n_gsm.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c > > index 4a43ef5..0ca810a 100644 > > --- a/drivers/tty/n_gsm.c > > +++ b/drivers/tty/n_gsm.c > > @@ -2660,6 +2660,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb, > > { > > struct gsm_mux_net *mux_net = (struct gsm_mux_net *)netdev_priv(net); > > struct gsm_dlci *dlci = mux_net->dlci; > > + mutex_lock(&dlci->mutex); > > Nack, start_xmit may be called in an atomic context -- you cannot call > mutex. > > > muxnet_get(mux_net); > > > > skb_queue_head(&dlci->skb_list, skb); > > @@ -2669,6 +2670,7 @@ static int gsm_mux_net_start_xmit(struct sk_buff *skb, > > /* And tell the kernel when the last transmit started. */ > > net->trans_start = jiffies; > > muxnet_put(mux_net); > > Instead the concept is broken. If this was the last reference (as > described in your steps above), it would blow up for the same reason I > refer to above, i.e. net_free here would call unregister_netdev which is > not atomic. Plus it will definitely deadlock because unregister_netdev > waits for start_xmit to finish. > > It should stop the queue and schedule a workqueue to lock the mutex, > unregister the hetdev and reset dlci->net. (Or maybe just call > muxnet_put with the lock held.) Thanks, Jiri, you're right, I didn't notice that in validation because DEBUG_ATOMIC_SLEEP is not enabled in my platform :( Now I'm trying to work out the workqueue solution, when it finished I'll re-submit for review. What do you mean by "call muxnet_put with lock held"? do you mean to use spin lock instead of mutex? > > That will fix 4), but there is still a bug: what protects > gsm_create_network to be called twice or more in a sequence thus > re-setting dlci->net to a new and new pointer? Yes, that's a problem, Vincent has already noticed that and has a check in gsmtty_ioctl to avoid call net creation multi time, I thought it might be patch for other issue so didn't put them together. > > > + mutex_unlock(&dlci->mutex); > > return NETDEV_TX_OK; > > } > > thanks, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] n_gsm: Add Mutex to avoid race when net destroy 2013-03-01 8:51 ` channing @ 2013-03-01 9:10 ` Jiri Slaby 2013-03-01 11:04 ` Bi, Chao 0 siblings, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2013-03-01 9:10 UTC (permalink / raw) To: channing; +Cc: Greg Kroah-Hartman, linux-kernel, ML netdev, vincentx.pillet On 03/01/2013 09:51 AM, channing wrote: >> It should stop the queue and schedule a workqueue to lock the mutex, >> unregister the hetdev and reset dlci->net. (Or maybe just call >> muxnet_put with the lock held.) > > Thanks, Jiri, you're right, I didn't notice that in validation because > DEBUG_ATOMIC_SLEEP is not enabled in my platform :( Now I'm trying to > work out the workqueue solution, when it finished I'll re-submit for > review. What do you mean by "call muxnet_put with lock held"? do you > mean to use spin lock instead of mutex? No, I mean, in the newly added scheduled work, to lock the mutex and simply call muxnet_put. That should fix it, right? -- js suse labs ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] n_gsm: Add Mutex to avoid race when net destroy 2013-03-01 9:10 ` Jiri Slaby @ 2013-03-01 11:04 ` Bi, Chao 0 siblings, 0 replies; 5+ messages in thread From: Bi, Chao @ 2013-03-01 11:04 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-kernel, ML netdev, Pillet, VincentX [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1513 bytes --] -----Original Message----- From: Jiri Slaby [mailto:jirislaby@gmail.com] On Behalf Of Jiri Slaby Sent: Friday, March 01, 2013 5:10 PM To: Bi, Chao Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org; ML netdev; Pillet, VincentX Subject: Re: [PATCH] n_gsm: Add Mutex to avoid race when net destroy On 03/01/2013 09:51 AM, channing wrote: >> It should stop the queue and schedule a workqueue to lock the mutex, >> unregister the hetdev and reset dlci->net. (Or maybe just call >> muxnet_put with the lock held.) > > Thanks, Jiri, you're right, I didn't notice that in validation because > DEBUG_ATOMIC_SLEEP is not enabled in my platform :( Now I'm trying to > work out the workqueue solution, when it finished I'll re-submit for > review. What do you mean by "call muxnet_put with lock held"? do you > mean to use spin lock instead of mutex? No, I mean, in the newly added scheduled work, to lock the mutex and simply call muxnet_put. That should fix it, right? [chao] Yes, that's to only move muxnet_put to scheduled work with mutex protected, and the rest of gsm_mux_net_start_xmit() Is kept unchanged, and they are not in mutex lock. It could avoid race of net_free(). I'm not sure if it's safe enough, I mean if dlci->net is released before STATS(net) in gsm_mux_net_start_xmit(), will STATS(net) access unreliable data? -- js suse labs ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-01 11:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-28 5:31 [PATCH] n_gsm: Add Mutex to avoid race when net destroy channing 2013-02-28 9:53 ` Jiri Slaby 2013-03-01 8:51 ` channing 2013-03-01 9:10 ` Jiri Slaby 2013-03-01 11:04 ` Bi, Chao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).