| Message ID | 1340356279-3124-1-git-send-email-bjorn@mork.no |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote: > > Reported-by: Marius Bjørnstad Kotsbak <marius.kotsbak@gmail.com> > Cc: <stable@vger.kernel.org> # v3.4 > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > drivers/net/usb/qmi_wwan.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c > index 3767a12..b01960f 100644 > --- a/drivers/net/usb/qmi_wwan.c > +++ b/drivers/net/usb/qmi_wwan.c > @@ -197,6 +197,10 @@ err: > static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on) > { > struct usbnet *dev = usb_get_intfdata(intf); > + > + /* can be called while disconnecting */ > + if (!dev) > + return 0; > return qmi_wwan_manage_power(dev, on); > } Considered there isn't any protection between usbnet_disconnet and wdm_open/wdm_close, you patch still doesn't work on the race, see below: + if (!dev) + return 0; -> usbnet_disconnect() triggered and completed, so return qmi_wwan_manage_power(dev, on); Thanks,
Sorry for mistaken triggered sending, :-( On Sat, Jun 23, 2012 at 11:32 AM, Ming Lei <tom.leiming@gmail.com> wrote: > On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote: > >> >> Reported-by: Marius Bjørnstad Kotsbak <marius.kotsbak@gmail.com> >> Cc: <stable@vger.kernel.org> # v3.4 >> Signed-off-by: Bjørn Mork <bjorn@mork.no> >> --- >> drivers/net/usb/qmi_wwan.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c >> index 3767a12..b01960f 100644 >> --- a/drivers/net/usb/qmi_wwan.c >> +++ b/drivers/net/usb/qmi_wwan.c >> @@ -197,6 +197,10 @@ err: >> static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on) >> { >> struct usbnet *dev = usb_get_intfdata(intf); >> + >> + /* can be called while disconnecting */ >> + if (!dev) >> + return 0; >> return qmi_wwan_manage_power(dev, on); >> } > Considered there isn't any protection between usbnet_disconnet and wdm_open/wdm_close, you patch still doesn't work on the race, see below: + if (!dev) + return 0; -> usbnet_disconnect() is triggered and completed here, so dev may point to a freed usbnet instance. return qmi_wwan_manage_power(dev, on); One fix I can think of is to export wdm_mutex or its lock/unlock functions, and introduce the function of qmi_wwan_disconnect, which will call usbnet_disconnect with holding wdm_mutex. Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Sat, Jun 23, 2012 at 11:32 AM, Ming Lei <tom.leiming@gmail.com> wrote: >> On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote: >> >>> >>> Reported-by: Marius Bjørnstad Kotsbak <marius.kotsbak@gmail.com> >>> Cc: <stable@vger.kernel.org> # v3.4 >>> Signed-off-by: Bjørn Mork <bjorn@mork.no> >>> --- >>> drivers/net/usb/qmi_wwan.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c >>> index 3767a12..b01960f 100644 >>> --- a/drivers/net/usb/qmi_wwan.c >>> +++ b/drivers/net/usb/qmi_wwan.c >>> @@ -197,6 +197,10 @@ err: >>> static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on) >>> { >>> struct usbnet *dev = usb_get_intfdata(intf); >>> + >>> + /* can be called while disconnecting */ >>> + if (!dev) >>> + return 0; >>> return qmi_wwan_manage_power(dev, on); >>> } >> > > Considered there isn't any protection between usbnet_disconnet and > wdm_open/wdm_close, you patch still doesn't work on the race, see > below: > > + if (!dev) > + return 0; > > -> usbnet_disconnect() is triggered and completed here, so > dev may point to a freed usbnet instance. > return qmi_wwan_manage_power(dev, on); usbnet_disconnect() cannot continue to the point where it frees the netdev before wdm_open/wdm_close has completed, because it waits for qmi_wwan_unbind which waits for wdm_disconnect. And wdm_disconnect takes the both read and write mutexes. So I do not think there is any race there. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 23, 2012 at 4:45 PM, Bjørn Mork <bjorn@mork.no> wrote: > usbnet_disconnect() cannot continue to the point where it frees the > netdev before wdm_open/wdm_close has completed, because it waits for > qmi_wwan_unbind which waits for wdm_disconnect. And wdm_disconnect > takes the both read and write mutexes. Thanks for your explanation. I see, and your patch is correct, but it might not be generic enough. So driver_info->unbind will sync with .open/.close of the subdriver, just like unregister_netdev will sync with .open/.close of usbnet interface. IMO, the general solution is to keep intfdata's lifetime after ->unbind, which can guarantee that intfdata can be accessed safely in .open/.close of usbnet interface and the other subdrivers' device. Suppose there will be another usbnet driver which has its own subdriver too, the same trick of checking need to be added again if not taking the general way of simply removing 'usb_set_intfdata(intf, NULL);' in usbnet_disconnect. Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Sat, Jun 23, 2012 at 4:45 PM, Bjørn Mork <bjorn@mork.no> wrote: > >> usbnet_disconnect() cannot continue to the point where it frees the >> netdev before wdm_open/wdm_close has completed, because it waits for >> qmi_wwan_unbind which waits for wdm_disconnect. And wdm_disconnect >> takes the both read and write mutexes. > > Thanks for your explanation. > > I see, and your patch is correct, but it might not be generic enough. > > So driver_info->unbind will sync with .open/.close of the subdriver, > just like unregister_netdev will sync with .open/.close of usbnet interface. > > IMO, the general solution is to keep intfdata's lifetime after ->unbind, > which can guarantee that intfdata can be accessed safely in .open/.close > of usbnet interface and the other subdrivers' device. > > Suppose there will be another usbnet driver which has its own subdriver > too, the same trick of checking need to be added again if not taking the > general way of simply removing 'usb_set_intfdata(intf, NULL);' in > usbnet_disconnect. Yes, I guess so. I am just worrying (maybe too much) about the unknown consequences of removing that code in usbnet, not fully understanding why it was there in the first place. And I do not want to take the blame and cleanup work if anything goes wrong :-) Fixing it in qmi_wwan feels much safer. But if you want to remove the 'usb_set_intfdata(intf, NULL)', and the other USB gurus agree, then I'm of course not going to object to that alternative solution to this bug. I still think it's unlikely this will ever hit another driver though. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Samstag, 23. Juni 2012, 17:32:09 schrieb Bjørn Mork: > Ming Lei <tom.leiming@gmail.com> writes: > > On Sat, Jun 23, 2012 at 4:45 PM, Bjørn Mork <bjorn@mork.no> wrote: > > Suppose there will be another usbnet driver which has its own subdriver > > too, the same trick of checking need to be added again if not taking the > > general way of simply removing 'usb_set_intfdata(intf, NULL);' in > > usbnet_disconnect. > > Yes, I guess so. > > I am just worrying (maybe too much) about the unknown consequences of > removing that code in usbnet, not fully understanding why it was there > in the first place. And I do not want to take the blame and cleanup > work if anything goes wrong :-) Fixing it in qmi_wwan feels much safer. void usbnet_cdc_unbind(struct usbnet *dev, struct usb_interface *intf) { struct cdc_state *info = (void *) &dev->data; struct usb_driver *driver = driver_of(intf); /* disconnect master --> disconnect slave */ if (intf == info->control && info->data) { /* ensure immediate exit from usbnet_disconnect */ usb_set_intfdata(info->data, NULL); usb_driver_release_interface(driver, info->data); info->data = NULL; 1. We mirror the minidrivers closely, which reduces errors 2. unbind() is called with the data anyway and after disconnect() the intfdata is not valid anyway, because the interface may have been reprobed. Regarsd Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Neukum <oliver@neukum.org> writes: > Am Samstag, 23. Juni 2012, 17:32:09 schrieb Bjørn Mork: >> Ming Lei <tom.leiming@gmail.com> writes: >> > On Sat, Jun 23, 2012 at 4:45 PM, Bjørn Mork <bjorn@mork.no> wrote: > >> > Suppose there will be another usbnet driver which has its own subdriver >> > too, the same trick of checking need to be added again if not taking the >> > general way of simply removing 'usb_set_intfdata(intf, NULL);' in >> > usbnet_disconnect. >> >> Yes, I guess so. >> >> I am just worrying (maybe too much) about the unknown consequences of >> removing that code in usbnet, not fully understanding why it was there >> in the first place. And I do not want to take the blame and cleanup >> work if anything goes wrong :-) Fixing it in qmi_wwan feels much safer. > > void usbnet_cdc_unbind(struct usbnet *dev, struct usb_interface *intf) > { > struct cdc_state *info = (void *) &dev->data; > struct usb_driver *driver = driver_of(intf); > > /* disconnect master --> disconnect slave */ > if (intf == info->control && info->data) { > /* ensure immediate exit from usbnet_disconnect */ > usb_set_intfdata(info->data, NULL); > usb_driver_release_interface(driver, info->data); > info->data = NULL; > > 1. We mirror the minidrivers closely, which reduces errors > 2. unbind() is called with the data anyway and after disconnect() > the intfdata is not valid anyway, because the interface may have been > reprobed. Sorry, I did not understand what you meant we should do here. The extra usb_set_intfdata(, NULL) in usbnet_disconnect() won't make any difference for that piece of code, will it? And the USB core ensures that intfdata is set to NULL before any reprobing, so that will never be a problem. That's the reason why it seems redundant setting it in usbnet_disconnect(). Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Sonntag, 24. Juni 2012, 11:34:19 schrieb Bjørn Mork: > Oliver Neukum <oliver@neukum.org> writes: > > 1. We mirror the minidrivers closely, which reduces errors > > 2. unbind() is called with the data anyway and after disconnect() > > the intfdata is not valid anyway, because the interface may have been > > reprobed. > > Sorry, I did not understand what you meant we should do here. The extra > usb_set_intfdata(, NULL) in usbnet_disconnect() won't make any > difference for that piece of code, will it? The point is that if it may be set to NULL, we always want it to be set to NULL, so we catch bugs. > And the USB core ensures that intfdata is set to NULL before any > reprobing, so that will never be a problem. That's the reason why it > seems redundant setting it in usbnet_disconnect(). The point is that if there is a problem because intfdata is set to NULL, there is very likely a problem in form of a race condition, if intfdata were not set to NULL in usbnet's disconnect handler. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Neukum <oliver@neukum.org> wrote: >Am Sonntag, 24. Juni 2012, 11:34:19 schrieb Bjørn Mork: > >> Sorry, I did not understand what you meant we should do here. The >extra >> usb_set_intfdata(, NULL) in usbnet_disconnect() won't make any >> difference for that piece of code, will it? > >The point is that if it may be set to NULL, we always want it to be set >to >NULL, so we catch bugs. > >> And the USB core ensures that intfdata is set to NULL before any >> reprobing, so that will never be a problem. That's the reason why it >> seems redundant setting it in usbnet_disconnect(). > >The point is that if there is a problem because intfdata is set to >NULL, >there is very likely a problem in form of a race condition, if intfdata >were not set to NULL in usbnet's disconnect handler. Thanks for explaining. Yes, that makes sense to me as well. So then the original patch against qmi_wwan should go in, and we should leave usbnet as it is. Are everyone comfortable with that? Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 25, 2012 at 1:47 AM, Bjørn Mork <bjorn@mork.no> wrote: > Oliver Neukum <oliver@neukum.org> wrote: >>Am Sonntag, 24. Juni 2012, 11:34:19 schrieb Bjørn Mork: >> >>> Sorry, I did not understand what you meant we should do here. The >>extra >>> usb_set_intfdata(, NULL) in usbnet_disconnect() won't make any >>> difference for that piece of code, will it? >> >>The point is that if it may be set to NULL, we always want it to be set >>to >>NULL, so we catch bugs. >> >>> And the USB core ensures that intfdata is set to NULL before any >>> reprobing, so that will never be a problem. That's the reason why it >>> seems redundant setting it in usbnet_disconnect(). >> >>The point is that if there is a problem because intfdata is set to >>NULL, >>there is very likely a problem in form of a race condition, if intfdata >>were not set to NULL in usbnet's disconnect handler. I don't agree on the assumption. The current problem is caused by the set to NULL without any protection or sync mechanism on it, and it is really a bug. Also we didn't say the set to NULL will be cancelled, just delay the clear until it is safe to do it, eg. after complete of unregister_netdev() and driver_info->unbind, when the previous .open/.close has been completed already or the later ones will notice the disconnection early and won't touch usb_get_intfdata() any more. > Thanks for explaining. Yes, that makes sense to me as well. > > So then the original patch against qmi_wwan should go in, and we should leave usbnet as it is. Are everyone comfortable with that? I don't see any races caused by just removing usb_set_intfdata(, NULL) or moving it after driver_info->unbind simply in usbnet_disconnect. In fact, suppose that .open/.close and .disconnect are run on different CPUs, there are no any guarantee that .open/.close can always see the clear action immediately. Also, the clear of intfdata may not be observed in .manage_power since usb_set_intfdata(, NULL) may be completed after the lock wdm_mutex operation. So it is only the sync mechanism that works on the race even the check is added in the patch. Putting usb_set_intfdata(, NULL) after driver_info->unbind should be OK, and it is a general solution for the problem. Thanks,
Am Montag, 25. Juni 2012, 05:37:20 schrieb Ming Lei: > On Mon, Jun 25, 2012 at 1:47 AM, Bjørn Mork <bjorn@mork.no> wrote: > > Oliver Neukum <oliver@neukum.org> wrote: > >>Am Sonntag, 24. Juni 2012, 11:34:19 schrieb Bjørn Mork: > >> > >>> Sorry, I did not understand what you meant we should do here. The > >>extra > >>> usb_set_intfdata(, NULL) in usbnet_disconnect() won't make any > >>> difference for that piece of code, will it? > >> > >>The point is that if it may be set to NULL, we always want it to be set > >>to > >>NULL, so we catch bugs. > >> > >>> And the USB core ensures that intfdata is set to NULL before any > >>> reprobing, so that will never be a problem. That's the reason why it > >>> seems redundant setting it in usbnet_disconnect(). > >> > >>The point is that if there is a problem because intfdata is set to > >>NULL, > >>there is very likely a problem in form of a race condition, if intfdata > >>were not set to NULL in usbnet's disconnect handler. > > I don't agree on the assumption. > > The current problem is caused by the set to NULL without any > protection or sync mechanism on it, and it is really a bug. Minidrivers can test for NULL. That may not be enough and locking may be needed. > Also we didn't say the set to NULL will be cancelled, just delay > the clear until it is safe to do it, eg. after complete of unregister_netdev() > and driver_info->unbind, when the previous .open/.close has been > completed already or the later ones will notice the disconnection > early and won't touch usb_get_intfdata() any more. We can move to after unregister_netdev() I am unhappy with it going after unbind. > > Thanks for explaining. Yes, that makes sense to me as well. > > > > So then the original patch against qmi_wwan should go in, and we should leave usbnet as it is. Are everyone comfortable with that? > > I don't see any races caused by just removing usb_set_intfdata(, NULL) > or moving it after driver_info->unbind simply in usbnet_disconnect. They are not caused. They are harder to detect. > In fact, suppose that .open/.close and .disconnect are run on different CPUs, > there are no any guarantee that .open/.close can always see the clear action > immediately. Also, the clear of intfdata may not be observed in .manage_power > since usb_set_intfdata(, NULL) may be completed after the lock wdm_mutex > operation. Sure, it is a debugging aid. It has the drawback that minidrivers have to be able to deal with intfdata being NULL. That is not hard to do. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 25, 2012 at 2:15 PM, Oliver Neukum <oliver@neukum.org> wrote: > Am Montag, 25. Juni 2012, 05:37:20 schrieb Ming Lei: >> The current problem is caused by the set to NULL without any >> protection or sync mechanism on it, and it is really a bug. > > Minidrivers can test for NULL. > That may not be enough and locking may be needed. Any locking isn't needed if the set to NULL is put after driver_info->unbind, since ->unbind will call subdriver->disconnect, which will hold the open/disconnect lock of wdm_mutex. > >> Also we didn't say the set to NULL will be cancelled, just delay >> the clear until it is safe to do it, eg. after complete of unregister_netdev() >> and driver_info->unbind, when the previous .open/.close has been >> completed already or the later ones will notice the disconnection >> early and won't touch usb_get_intfdata() any more. > > We can move to after unregister_netdev() > I am unhappy with it going after unbind. > Could you let us know the reason? I think it may let the patch not necessary. >> I don't see any races caused by just removing usb_set_intfdata(, NULL) >> or moving it after driver_info->unbind simply in usbnet_disconnect. > > They are not caused. They are harder to detect. > >> In fact, suppose that .open/.close and .disconnect are run on different CPUs, >> there are no any guarantee that .open/.close can always see the clear action >> immediately. Also, the clear of intfdata may not be observed in .manage_power >> since usb_set_intfdata(, NULL) may be completed after the lock wdm_mutex >> operation. > > Sure, it is a debugging aid. It has the drawback that minidrivers have > to be able to deal with intfdata being NULL. That is not hard to do. The check isn't needed if the set to NULL is put after driver_info->unbind in usbnet_disconnect. I think we can add the comment on it to avoid the unnecessary check. Thanks,
[I don't have anything more to add to the generic usbnet discussion, but want to comment on the qmi_wwan issues] Ming Lei <tom.leiming@gmail.com> writes: > Also, the clear of intfdata may not be observed in .manage_power > since usb_set_intfdata(, NULL) may be completed after the lock wdm_mutex > operation. True, but irrelevant. The pointer is either valid or NULL. We don't need to care about synchronizing the exact time it is set to NULL. The locking in cdc-wdm will ensure that the pointer is valid while it is in use by .manage_power, because usbnet_disconnect is prevented from continuing with free_netdev() while any caller of .manage_power is running. > So it is only the sync mechanism that works on the race even the check is > added in the patch. Putting usb_set_intfdata(, NULL) after driver_info->unbind > should be OK, and it is a general solution for the problem. There is no problem wrt qmi_wwan and intfdata as long as the NULL test is added to .manage_power. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 25, 2012 at 3:24 PM, Bjørn Mork <bjorn@mork.no> wrote: > > True, but irrelevant. The pointer is either valid or NULL. We don't > need to care about synchronizing the exact time it is set to NULL. > > The locking in cdc-wdm will ensure that the pointer is valid while it is > in use by .manage_power, because usbnet_disconnect is prevented from > continuing with free_netdev() while any caller of .manage_power is > running. What I mean is that the situation is just what moving the set to NULL is doing. >> So it is only the sync mechanism that works on the race even the check is >> added in the patch. Putting usb_set_intfdata(, NULL) after driver_info->unbind >> should be OK, and it is a general solution for the problem. > > There is no problem wrt qmi_wwan and intfdata as long as the NULL test > is added to .manage_power. It depends on the ARCH or compiler. Considered there is not any locking/memory barrier between the set to NULL and read the pointer, also no ACCESS_ONCE on read or store the pointer, reading in .manage_power may see a invalid pointer if the CPU doesn't support Store Atomicity or the compiler does a byte-at-a-time optimization on the store[1]. So why not take the correct way in theory? also it is a general solution, and we can document its usage. [1], Paul mentioned it in the previous discussion http://lkml.org/lkml/2012/6/6/280 Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Mon, Jun 25, 2012 at 3:24 PM, Bjørn Mork <bjorn@mork.no> wrote: > >> There is no problem wrt qmi_wwan and intfdata as long as the NULL test >> is added to .manage_power. > > It depends on the ARCH or compiler. > > Considered there is not any locking/memory barrier between the set to NULL > and read the pointer, also no ACCESS_ONCE on read or store the pointer, > reading in .manage_power may see a invalid pointer if the CPU doesn't > support Store Atomicity or the compiler does a byte-at-a-time optimization > on the store[1]. > > So why not take the correct way in theory? also it is a general solution, > and we can document its usage. > > > [1], Paul mentioned it in the previous discussion > http://lkml.org/lkml/2012/6/6/280 I don't believe that is valid for pointers, which hopefully either have native word size or are protected by the compiler. In any case, I'm not prepared to deal with the situation that changing a pointer from valid to NULL can cause intermediate invalid pointers. If such an architecture/compiler exists, then I believe the sanest thing to do is to let it fail randomly. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, 25. Juni 2012, 09:15:21 schrieb Ming Lei: > On Mon, Jun 25, 2012 at 2:15 PM, Oliver Neukum <oliver@neukum.org> wrote: > > Am Montag, 25. Juni 2012, 05:37:20 schrieb Ming Lei: > > >> The current problem is caused by the set to NULL without any > >> protection or sync mechanism on it, and it is really a bug. > > > > Minidrivers can test for NULL. > > That may not be enough and locking may be needed. > > Any locking isn't needed if the set to NULL is put after > driver_info->unbind, since ->unbind will call subdriver->disconnect, > which will hold the open/disconnect lock of wdm_mutex. True for cdc_wdm. But usbnet needs to work well for everything. > > We can move to after unregister_netdev() > > I am unhappy with it going after unbind. > > > > Could you let us know the reason? I think it may let the > patch not necessary. Very well. This is the code: void usbnet_disconnect (struct usb_interface *intf) { struct usbnet *dev; struct usb_device *xdev; struct net_device *net; dev = usb_get_intfdata(intf); usb_set_intfdata(intf, NULL); if (!dev) return; This code needs to check for NULL (cdc_ether and similar drivers) It is cleaner that if we need to check for NULL we also set to NULL. But that is no good reason to keep it if there's real trouble xdev = interface_to_usbdev (intf); netif_info(dev, probe, dev->net, "unregister '%s' usb-%s-%s, %s\n", intf->dev.driver->name, xdev->bus->bus_name, xdev->devpath, dev->driver_info->description); net = dev->net; unregister_netdev (net); Here intfdata is NULL. cancel_work_sync(&dev->kevent); if (dev->driver_info->unbind) dev->driver_info->unbind (dev, intf); At this point a minidriver must not follow the intfdata pointer, because the interface may again be probed. So if here a minidriver still uses intfdata, locking will be needed. We want to catch those casees. usb_kill_urb(dev->interrupt); usb_free_urb(dev->interrupt); free_netdev(net); usb_put_dev (xdev); } > > Sure, it is a debugging aid. It has the drawback that minidrivers have > > to be able to deal with intfdata being NULL. That is not hard to do. > > The check isn't needed if the set to NULL is put after driver_info->unbind > in usbnet_disconnect. True, but we don't catch bugs. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum <oliver@neukum.org> wrote: > Am Montag, 25. Juni 2012, 09:15:21 schrieb Ming Lei: >> Any locking isn't needed if the set to NULL is put after >> driver_info->unbind, since ->unbind will call subdriver->disconnect, >> which will hold the open/disconnect lock of wdm_mutex. > > True for cdc_wdm. But usbnet needs to work well for everything. Suppose there are other usbnet drivers which may have this kind of subdriver, and they have to take one lock to avoid open/disconnect race, there are only two ways to do it: - the lock is held before calling usbnet_disconnect - the lock is held inside driver_info->unbind So putting the set to NULL after driver_info->unbind should work for the both two ways above. Also we can document the usage in comments. > >> > We can move to after unregister_netdev() >> > I am unhappy with it going after unbind. >> > >> >> Could you let us know the reason? I think it may let the >> patch not necessary. > > Very well. This is the code: > > void usbnet_disconnect (struct usb_interface *intf) > { > struct usbnet *dev; > struct usb_device *xdev; > struct net_device *net; > > dev = usb_get_intfdata(intf); > usb_set_intfdata(intf, NULL); > if (!dev) > return; > > This code needs to check for NULL (cdc_ether and similar drivers) > It is cleaner that if we need to check for NULL we also set to NULL. > But that is no good reason to keep it if there's real trouble > > xdev = interface_to_usbdev (intf); > > netif_info(dev, probe, dev->net, "unregister '%s' usb-%s-%s, %s\n", > intf->dev.driver->name, > xdev->bus->bus_name, xdev->devpath, > dev->driver_info->description); > > net = dev->net; > unregister_netdev (net); > > Here intfdata is NULL. > > cancel_work_sync(&dev->kevent); > > if (dev->driver_info->unbind) > dev->driver_info->unbind (dev, intf); > > At this point a minidriver must not follow the intfdata pointer, > because the interface may again be probed. So if here a minidriver IMO, probe is serialized strictly with driver unbind since both the parent lock and its own device lock have been held, so the probe may only be started after driver unbinding is completed. > still uses intfdata, locking will be needed. We want to catch those > casees. Suppose infdata is used here somewhere, it is surely a bug because the usbnet instance pointed by intfdata will be freed soon. So looks putting the set to NULL after driver_info->unbind is good, doesn't it? > > usb_kill_urb(dev->interrupt); > usb_free_urb(dev->interrupt); > > free_netdev(net); > usb_put_dev (xdev); > } > >> > Sure, it is a debugging aid. It has the drawback that minidrivers have >> > to be able to deal with intfdata being NULL. That is not hard to do. >> >> The check isn't needed if the set to NULL is put after driver_info->unbind >> in usbnet_disconnect. > > True, but we don't catch bugs. If the check is added, the bugs may be hided, and no stack will be dumped, :-) Thanks,
Am Dienstag, 26. Juni 2012, 09:23:19 schrieb Ming Lei: > On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum <oliver@neukum.org> wrote: > > At this point a minidriver must not follow the intfdata pointer, > > because the interface may again be probed. So if here a minidriver > > IMO, probe is serialized strictly with driver unbind since both the parent > lock and its own device lock have been held, so the probe may only be > started after driver unbinding is completed. Yes, but if you have a driver which claims multiple interfaces and uses a subdriver, then you will have cases of intfdate being NULL before disconnect() finishes. > > still uses intfdata, locking will be needed. We want to catch those > > casees. > > Suppose infdata is used here somewhere, it is surely a bug because > the usbnet instance pointed by intfdata will be freed soon. Of course. That is the point. > So looks putting the set to NULL after driver_info->unbind is good, > doesn't it? Again, of course. We could drop it (but not the check for NULL in usbnet). It is a debugging aid. > > usb_kill_urb(dev->interrupt); > > usb_free_urb(dev->interrupt); > > > > free_netdev(net); > > usb_put_dev (xdev); > > } > > > >> > Sure, it is a debugging aid. It has the drawback that minidrivers have > >> > to be able to deal with intfdata being NULL. That is not hard to do. > >> > >> The check isn't needed if the set to NULL is put after driver_info->unbind > >> in usbnet_disconnect. > > > > True, but we don't catch bugs. > > If the check is added, the bugs may be hided, and no stack will be > dumped, :-) That is also true. Bjørn, do you use subdrivers with cdc-ether? Regards Oliver
Bjørn Mork <bjorn@mork.no> writes: [..] > Fixes this Oops where a cdc-wdm device was closed while the > USB device was disconnecting, causing wdm_release to call > qmi_wwan_cdc_wdm_manage_power after intfdata was set to > NULL by usbnet_disconnect: > > [41819.087460] BUG: unable to handle kernel NULL pointer dereference at 00000080 > [41819.087815] IP: [<f8640458>] qmi_wwan_manage_power+0x68/0x90 [qmi_wwan] > [41819.088028] *pdpt = 000000000314f001 *pde = 0000000000000000 > [41819.088028] Oops: 0002 [#1] SMP [..] > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c > index 3767a12..b01960f 100644 > --- a/drivers/net/usb/qmi_wwan.c > +++ b/drivers/net/usb/qmi_wwan.c > @@ -197,6 +197,10 @@ err: > static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on) > { > struct usbnet *dev = usb_get_intfdata(intf); > + > + /* can be called while disconnecting */ > + if (!dev) > + return 0; > return qmi_wwan_manage_power(dev, on); > } Hello, I'd like this patch applied to qmi_wwan regardless of the outcome of the (now stalled?) generic usbnet_disconnect discussion. The patch fixes a real Oops in 3.4 and 3.5, and I believe it should be left in qmi_wwan even if the usbnet code is fixed to avoid this specific bug. The additional NULL test won't harm, and it makes the code more robust should someone decide to rearrange usbnet_disconnect again at some later point in time. I really want this fixed in the next 3.4 stable release, if possible. Should I resubmit the patch, or will you pick it up from http://patchwork.ozlabs.org/patch/166542/ ? Thanks, Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 28. Juni 2012, 10:36:49 schrieb Bjørn Mork: > Bjørn Mork <bjorn@mork.no> writes: > > [..] > > Fixes this Oops where a cdc-wdm device was closed while the > > USB device was disconnecting, causing wdm_release to call > > qmi_wwan_cdc_wdm_manage_power after intfdata was set to > > NULL by usbnet_disconnect: > > > > [41819.087460] BUG: unable to handle kernel NULL pointer dereference at 00000080 > > [41819.087815] IP: [<f8640458>] qmi_wwan_manage_power+0x68/0x90 [qmi_wwan] > > [41819.088028] *pdpt = 000000000314f001 *pde = 0000000000000000 > > [41819.088028] Oops: 0002 [#1] SMP > > [..] > > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c > > index 3767a12..b01960f 100644 > > --- a/drivers/net/usb/qmi_wwan.c > > +++ b/drivers/net/usb/qmi_wwan.c > > @@ -197,6 +197,10 @@ err: > > static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on) > > { > > struct usbnet *dev = usb_get_intfdata(intf); > > + > > + /* can be called while disconnecting */ > > + if (!dev) > > + return 0; > > return qmi_wwan_manage_power(dev, on); > > } > > Hello, > > I'd like this patch applied to qmi_wwan regardless of the outcome of the > (now stalled?) generic usbnet_disconnect discussion. > > The patch fixes a real Oops in 3.4 and 3.5, and I believe it should be > left in qmi_wwan even if the usbnet code is fixed to avoid this specific > bug. The additional NULL test won't harm, and it makes the code more > robust should someone decide to rearrange usbnet_disconnect again at > some later point in time. > > I really want this fixed in the next 3.4 stable release, if possible. > Should I resubmit the patch, or will you pick it up from > http://patchwork.ozlabs.org/patch/166542/ ? Yes. David, can you please apply this? It needs to also go into stable. We might remove this in later releases, but for now it is needed. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Neukum <oneukum@suse.de> writes: > Am Dienstag, 26. Juni 2012, 09:23:19 schrieb Ming Lei: >> On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum <oliver@neukum.org> wrote: > >> > At this point a minidriver must not follow the intfdata pointer, >> > because the interface may again be probed. So if here a minidriver >> >> IMO, probe is serialized strictly with driver unbind since both the parent >> lock and its own device lock have been held, so the probe may only be >> started after driver unbinding is completed. > > Yes, but if you have a driver which claims multiple interfaces and uses > a subdriver, then you will have cases of intfdate being NULL before > disconnect() finishes. This is true regardless of subdriver, isn't it? The subdriver issue is special because it makes this a potentional problem even with a single interface. >> > still uses intfdata, locking will be needed. We want to catch those >> > casees. >> >> Suppose infdata is used here somewhere, it is surely a bug because >> the usbnet instance pointed by intfdata will be freed soon. > > Of course. That is the point. > >> So looks putting the set to NULL after driver_info->unbind is good, >> doesn't it? > > Again, of course. We could drop it (but not the check for NULL in usbnet). > It is a debugging aid. > >> > usb_kill_urb(dev->interrupt); >> > usb_free_urb(dev->interrupt); >> > >> > free_netdev(net); >> > usb_put_dev (xdev); >> > } >> > >> >> > Sure, it is a debugging aid. It has the drawback that minidrivers have >> >> > to be able to deal with intfdata being NULL. That is not hard to do. >> >> >> >> The check isn't needed if the set to NULL is put after driver_info->unbind >> >> in usbnet_disconnect. >> > >> > True, but we don't catch bugs. >> >> If the check is added, the bugs may be hided, and no stack will be >> dumped, :-) > > That is also true. > > Bjørn, > > do you use subdrivers with cdc-ether? No, and I don't think it should ever be added there. It would add complexity to the driver, and IMHO we really don't want that for the generic cdc-ether. Better adding a new minidriver if the need to export some other CDC embedded protocol to userspace shows up. But given that all other embedded protocol examples I've seen are so simple that they've been implemented inside their repective minidrivers (sierra_net, hso, rndis_host), I don't think it's likely that any new need for the cdc-wdm subdriver will show up soon. I could of course be completely wrong, as usual. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 28, 2012 at 4:35 PM, Oliver Neukum <oneukum@suse.de> wrote: > Am Dienstag, 26. Juni 2012, 09:23:19 schrieb Ming Lei: >> IMO, probe is serialized strictly with driver unbind since both the parent >> lock and its own device lock have been held, so the probe may only be >> started after driver unbinding is completed. > > Yes, but if you have a driver which claims multiple interfaces and uses > a subdriver, then you will have cases of intfdate being NULL before We have no such case now. > disconnect() finishes. Suppose cdc-ether will support subdriver, ->unbind will set NULL to the 2nd interface and release the 2nd interface, then subdriver->disconnect() will be called, looks still no any problem. Thanks,
From: Oliver Neukum <oliver@neukum.org> Date: Thu, 28 Jun 2012 10:40:55 +0200 > Am Donnerstag, 28. Juni 2012, 10:36:49 schrieb Bjørn Mork: >> I'd like this patch applied to qmi_wwan regardless of the outcome of the >> (now stalled?) generic usbnet_disconnect discussion. >> >> The patch fixes a real Oops in 3.4 and 3.5, and I believe it should be >> left in qmi_wwan even if the usbnet code is fixed to avoid this specific >> bug. The additional NULL test won't harm, and it makes the code more >> robust should someone decide to rearrange usbnet_disconnect again at >> some later point in time. >> >> I really want this fixed in the next 3.4 stable release, if possible. >> Should I resubmit the patch, or will you pick it up from >> http://patchwork.ozlabs.org/patch/166542/ ? > > Yes. > > David, can you please apply this? It needs to also go into stable. > We might remove this in later releases, but for now it is needed. Done. I left the stable CC: in there so Greg will pick this up automatically once it hits Linus's tree. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c index 3767a12..b01960f 100644 --- a/drivers/net/usb/qmi_wwan.c +++ b/drivers/net/usb/qmi_wwan.c @@ -197,6 +197,10 @@ err: static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on) { struct usbnet *dev = usb_get_intfdata(intf); + + /* can be called while disconnecting */ + if (!dev) + return 0; return qmi_wwan_manage_power(dev, on); }
usbnet_disconnect() will set intfdata to NULL before calling the minidriver unbind function. The cdc_wdm subdriver cannot know that it is disconnecting until the qmi_wwan unbind function has called its disconnect function. This means that we must be able to support the cdc_wdm subdriver operating normally while usbnet_disconnect() is running, and in particular that intfdata may be NULL. The only place this matters is in qmi_wwan_cdc_wdm_manage_power which is called from cdc_wdm. Simply testing for NULL intfdata there is sufficient to allow it to continue working at all times. Fixes this Oops where a cdc-wdm device was closed while the USB device was disconnecting, causing wdm_release to call qmi_wwan_cdc_wdm_manage_power after intfdata was set to NULL by usbnet_disconnect: [41819.087460] BUG: unable to handle kernel NULL pointer dereference at 00000080 [41819.087815] IP: [<f8640458>] qmi_wwan_manage_power+0x68/0x90 [qmi_wwan] [41819.088028] *pdpt = 000000000314f001 *pde = 0000000000000000 [41819.088028] Oops: 0002 [#1] SMP [41819.088028] Modules linked in: qmi_wwan option usb_wwan usbserial usbnet cdc_wdm nls_iso8859_1 nls_cp437 vfat fat usb_storage bnep rfcomm bluetooth parport_pc ppdev binfmt_misc iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 iptable_mangle iptable_filter ip_tables x_tables dm_crypt uvcvideo snd_hda_codec_realtek snd_hda_intel videobuf2_core snd_hda_codec joydev videodev videobuf2_vmalloc hid_multitouch snd_hwdep arc4 videobuf2_memops snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event ath9k mac80211 snd_seq ath9k_common ath9k_hw ath snd_timer snd_seq_device sparse_keymap dm_multipath scsi_dh coretemp mac_hid snd soundcore cfg80211 snd_page_alloc psmouse serio_raw microcode lp parport dm_mirror dm_region_hash dm_log usbhid hid i915 drm_kms_helper drm r8169 i2c_algo_bit wmi video [last unloaded: qmi_wwan] [41819.088028] [41819.088028] Pid: 23292, comm: qmicli Not tainted 3.4.0-5-generic #11-Ubuntu GIGABYTE T1005/T1005 [41819.088028] EIP: 0060:[<f8640458>] EFLAGS: 00010246 CPU: 1 [41819.088028] EIP is at qmi_wwan_manage_power+0x68/0x90 [qmi_wwan] [41819.088028] EAX: 00000000 EBX: 00000000 ECX: 000000c3 EDX: 00000000 [41819.088028] ESI: c3b27658 EDI: 00000000 EBP: c298bea4 ESP: c298be98 [41819.088028] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 [41819.088028] CR0: 8005003b CR2: 00000080 CR3: 3605e000 CR4: 000007f0 [41819.088028] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 [41819.088028] DR6: ffff0ff0 DR7: 00000400 [41819.088028] Process qmicli (pid: 23292, ti=c298a000 task=f343b280 task.ti=c298a000) [41819.088028] Stack: [41819.088028] 00000000 c3b27658 e2a80d00 c298beb0 f864051a c3b27600 c298bec0 f9027099 [41819.088028] c2fd6000 00000008 c298bef0 c1147f96 00000001 00000000 00000000 f4e54790 [41819.088028] ecf43a00 ecf43a00 c2fd6008 c2fd6000 ebbd7600 ffffffb9 c298bf08 c1144474 [41819.088028] Call Trace: [41819.088028] [<f864051a>] qmi_wwan_cdc_wdm_manage_power+0x1a/0x20 [qmi_wwan] [41819.088028] [<f9027099>] wdm_release+0x69/0x70 [cdc_wdm] [41819.088028] [<c1147f96>] fput+0xe6/0x210 [41819.088028] [<c1144474>] filp_close+0x54/0x80 [41819.088028] [<c1046a65>] put_files_struct+0x75/0xc0 [41819.088028] [<c1046b56>] exit_files+0x46/0x60 [41819.088028] [<c1046f81>] do_exit+0x141/0x780 [41819.088028] [<c107248f>] ? wake_up_state+0xf/0x20 [41819.088028] [<c1053f48>] ? signal_wake_up+0x28/0x40 [41819.088028] [<c1054f3b>] ? zap_other_threads+0x6b/0x80 [41819.088028] [<c1047864>] do_group_exit+0x34/0xa0 [41819.088028] [<c10478e8>] sys_exit_group+0x18/0x20 [41819.088028] [<c15bb7df>] sysenter_do_call+0x12/0x28 [41819.088028] Code: 04 83 e7 01 c1 e7 03 0f b6 42 18 83 e0 f7 09 f8 88 42 18 8b 43 04 e8 48 9a dd c8 89 f0 8b 5d f4 8b 75 f8 8b 7d fc 89 ec 5d c3 90 <f0> ff 88 80 00 00 00 0f 94 c0 84 c0 75 b7 31 f6 8b 5d f4 89 f0 [41819.088028] EIP: [<f8640458>] qmi_wwan_manage_power+0x68/0x90 [qmi_wwan] SS:ESP 0068:c298be98 [41819.088028] CR2: 0000000000000080 [41819.149492] ---[ end trace 0944479ff8257f55 ]--- Reported-by: Marius Bjørnstad Kotsbak <marius.kotsbak@gmail.com> Cc: <stable@vger.kernel.org> # v3.4 Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/usb/qmi_wwan.c | 4 ++++ 1 file changed, 4 insertions(+)