Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

Am Montag, 25. Juni 2012, 09:15:21 schrieb Ming Lei:

On Mon, Jun 25, 2012 at 2:15 PM, Oliver Neukum [off-list ref] wrote:

quoted

Am Montag, 25. Juni 2012, 05:37:20 schrieb Ming Lei:

quoted

quoted

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.

quoted

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);
}

quoted

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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum [off-list ref] wrote:

Am Montag, 25. Juni 2012, 09:15:21 schrieb Ming Lei:

quoted

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.

quoted

quoted

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);
}

quoted

quoted

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,
-- 
Ming Lei

Am Dienstag, 26. Juni 2012, 09:23:19 schrieb Ming Lei:

On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum [off-list ref] wrote:

quoted

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.

quoted

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. 

quoted

       usb_kill_urb(dev->interrupt);
       usb_free_urb(dev->interrupt);

       free_netdev(net);
       usb_put_dev (xdev);
}

quoted

quoted

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

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 

On Thu, Jun 28, 2012 at 4:35 PM, Oliver Neukum [off-list ref] wrote:

Am Dienstag, 26. Juni 2012, 09:23:19 schrieb Ming Lei:

quoted

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,
-- 
Ming Lei