Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Wed, 12 Apr 2017, Felipe Balbi wrote:

> >> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> >> disagree.  Hasn't he said, many times in the past, that any dynamically 
> >> allocated device structure _must_ have a real release routine?  
> >> usb_udc_nop_release() doesn't qualify.
> >
> > Aw, I wanted to publically yell at someone like the kernel documentation
> > says I am allowed to do so if anyone does such a foolish thing :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-) Here's what we have for most UDCs (net2280.c included):
> 
>   struct my_udc {
>   struct gadget gadget;
> [...]
>   };
> 
>   probe()
> {
>   struct my_udc *u;
> 
>   u = kzalloc(sizeof(*u), GFP_KERNEL);
> [...]
>   return 0;
>   }

Allow me to point out that the struct device is embedded inside the
struct gadget (actually struct usb_gadget) embedded inside the struct
my_udc, which _is_ dynamically allocated.  Therefore the struct device
is located in dynamically allocated memory.

> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
> this result on a functionally equivalent execution to the patch I
> proposed above?

It would, and it would be equally wrong.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Greg KH  writes:
>> Here's what we have for most UDCs (net2280.c included):
>> 
>>  struct my_udc {
>>  struct gadget gadget;
>> [...]
>>  };
>> 
>>  probe()
>> {
>>  struct my_udc *u;
>> 
>>  u = kzalloc(sizeof(*u), GFP_KERNEL);
>> [...]
>>  return 0;
>>  }
>> 
>> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
>> this result on a functionally equivalent execution to the patch I
>> proposed above?
>> 
>> Iff we change struct gadget to contain a struct device *dev instead of a
>> struct device dev, then sure, we will need to cope with proper
>> ->release() implementations.
>> 
>> As it is, it brings nothing to the table, IMO.
>
> You always have to have a release function for a kobject, no matter
> where it is, as it is being reference counted.  To not do so, is a huge
> indication of a problem in the design.

okay, this goes all the way back to when Dave B wrote the API, it has
always had empty ->release() functions (not all UDCs, though). I'll make
sure to review all of this for v4.13.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Greg KH]

On Wed, Apr 12, 2017 at 09:01:44AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH  writes:
> > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> >> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> >> 
> >> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> >> > > up every time it gets called, in its current form.
> >> > 
> >> > Well, it does :-)
> >> > 
> >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> >> > 
> >> > We're just leaking memory. I guess a patch like below would be best:
> >> > 
> >> > diff --git a/drivers/usb/gadget/udc/net2280.c 
> >> > b/drivers/usb/gadget/udc/net2280.c
> >> > index 3828c2ec8623..4dc04253da61 100644
> >> > --- a/drivers/usb/gadget/udc/net2280.c
> >> > +++ b/drivers/usb/gadget/udc/net2280.c
> >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void 
> >> > *_dev)
> >> >  
> >> >  
> >> > /*-*/
> >> >  
> >> > -static void gadget_release(struct device *_dev)
> >> > -{
> >> > -struct net2280  *dev = dev_get_drvdata(_dev);
> >> > -
> >> > -kfree(dev);
> >> > -}
> >> > -
> >> >  /* tear down the binding between this driver and the pci device */
> >> >  
> >> >  static void net2280_remove(struct pci_dev *pdev)
> >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> >> >  device_remove_file(>dev, _attr_registers);
> >> >  
> >> >  ep_info(dev, "unbind\n");
> >> > +
> >> > +kfree(dev);
> >> >  }
> >> >  
> >> >  /* wrap this driver around the specified device, but
> >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, 
> >> > const struct pci_device_id *id)
> >> >  if (retval)
> >> >  goto done;
> >> >  
> >> > -retval = usb_add_gadget_udc_release(>dev, >gadget,
> >> > -gadget_release);
> >> > +retval = usb_add_gadget_udc(>dev, >gadget);
> >> >  if (retval)
> >> >  goto done;
> >> >  return 0;
> >> 
> >> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> >> disagree.  Hasn't he said, many times in the past, that any dynamically 
> >> allocated device structure _must_ have a real release routine?  
> >> usb_udc_nop_release() doesn't qualify.
> >
> > Aw, I wanted to publically yell at someone like the kernel documentation
> > says I am allowed to do so if anyone does such a foolish thing :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-)

Please don't say that, that's even worse :(

> Here's what we have for most UDCs (net2280.c included):
> 
>   struct my_udc {
>   struct gadget gadget;
> [...]
>   };
> 
>   probe()
> {
>   struct my_udc *u;
> 
>   u = kzalloc(sizeof(*u), GFP_KERNEL);
> [...]
>   return 0;
>   }
> 
> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
> this result on a functionally equivalent execution to the patch I
> proposed above?
> 
> Iff we change struct gadget to contain a struct device *dev instead of a
> struct device dev, then sure, we will need to cope with proper
> ->release() implementations.
> 
> As it is, it brings nothing to the table, IMO.

You always have to have a release function for a kobject, no matter
where it is, as it is being reference counted.  To not do so, is a huge
indication of a problem in the design.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Greg KH  writes:
> On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
>> On Tue, 11 Apr 2017, Felipe Balbi wrote:
>> 
>> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
>> > > up every time it gets called, in its current form.
>> > 
>> > Well, it does :-)
>> > 
>> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
>> > 
>> > We're just leaking memory. I guess a patch like below would be best:
>> > 
>> > diff --git a/drivers/usb/gadget/udc/net2280.c 
>> > b/drivers/usb/gadget/udc/net2280.c
>> > index 3828c2ec8623..4dc04253da61 100644
>> > --- a/drivers/usb/gadget/udc/net2280.c
>> > +++ b/drivers/usb/gadget/udc/net2280.c
>> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>> >  
>> >  
>> > /*-*/
>> >  
>> > -static void gadget_release(struct device *_dev)
>> > -{
>> > -  struct net2280  *dev = dev_get_drvdata(_dev);
>> > -
>> > -  kfree(dev);
>> > -}
>> > -
>> >  /* tear down the binding between this driver and the pci device */
>> >  
>> >  static void net2280_remove(struct pci_dev *pdev)
>> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>> >device_remove_file(>dev, _attr_registers);
>> >  
>> >ep_info(dev, "unbind\n");
>> > +
>> > +  kfree(dev);
>> >  }
>> >  
>> >  /* wrap this driver around the specified device, but
>> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const 
>> > struct pci_device_id *id)
>> >if (retval)
>> >goto done;
>> >  
>> > -  retval = usb_add_gadget_udc_release(>dev, >gadget,
>> > -  gadget_release);
>> > +  retval = usb_add_gadget_udc(>dev, >gadget);
>> >if (retval)
>> >goto done;
>> >return 0;
>> 
>> Maybe...  But I can't shake the feeling that Greg KH would strongly 
>> disagree.  Hasn't he said, many times in the past, that any dynamically 
>> allocated device structure _must_ have a real release routine?  
>> usb_udc_nop_release() doesn't qualify.
>
> Aw, I wanted to publically yell at someone like the kernel documentation
> says I am allowed to do so if anyone does such a foolish thing :)

heh, except that we're not dynamically allocating struct device at all
:-) Here's what we have for most UDCs (net2280.c included):

struct my_udc {
struct gadget gadget;
[...]
};

probe()
{
struct my_udc *u;

u = kzalloc(sizeof(*u), GFP_KERNEL);
[...]
return 0;
}

Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
this result on a functionally equivalent execution to the patch I
proposed above?

Iff we change struct gadget to contain a struct device *dev instead of a
struct device dev, then sure, we will need to cope with proper
->release() implementations.

As it is, it brings nothing to the table, IMO.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Greg KH]

On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> 
> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> > > up every time it gets called, in its current form.
> > 
> > Well, it does :-)
> > 
> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> > 
> > We're just leaking memory. I guess a patch like below would be best:
> > 
> > diff --git a/drivers/usb/gadget/udc/net2280.c 
> > b/drivers/usb/gadget/udc/net2280.c
> > index 3828c2ec8623..4dc04253da61 100644
> > --- a/drivers/usb/gadget/udc/net2280.c
> > +++ b/drivers/usb/gadget/udc/net2280.c
> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
> >  
> >  
> > /*-*/
> >  
> > -static void gadget_release(struct device *_dev)
> > -{
> > -   struct net2280  *dev = dev_get_drvdata(_dev);
> > -
> > -   kfree(dev);
> > -}
> > -
> >  /* tear down the binding between this driver and the pci device */
> >  
> >  static void net2280_remove(struct pci_dev *pdev)
> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> > device_remove_file(>dev, _attr_registers);
> >  
> > ep_info(dev, "unbind\n");
> > +
> > +   kfree(dev);
> >  }
> >  
> >  /* wrap this driver around the specified device, but
> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *id)
> > if (retval)
> > goto done;
> >  
> > -   retval = usb_add_gadget_udc_release(>dev, >gadget,
> > -   gadget_release);
> > +   retval = usb_add_gadget_udc(>dev, >gadget);
> > if (retval)
> > goto done;
> > return 0;
> 
> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> disagree.  Hasn't he said, many times in the past, that any dynamically 
> allocated device structure _must_ have a real release routine?  
> usb_udc_nop_release() doesn't qualify.

Aw, I wanted to publically yell at someone like the kernel documentation
says I am allowed to do so if anyone does such a foolish thing :)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Tue, 11 Apr 2017, Felipe Balbi wrote:

> > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> > up every time it gets called, in its current form.
> 
> Well, it does :-)
> 
> dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> 
> We're just leaking memory. I guess a patch like below would be best:
> 
> diff --git a/drivers/usb/gadget/udc/net2280.c 
> b/drivers/usb/gadget/udc/net2280.c
> index 3828c2ec8623..4dc04253da61 100644
> --- a/drivers/usb/gadget/udc/net2280.c
> +++ b/drivers/usb/gadget/udc/net2280.c
> @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  /*-*/
>  
> -static void gadget_release(struct device *_dev)
> -{
> - struct net2280  *dev = dev_get_drvdata(_dev);
> -
> - kfree(dev);
> -}
> -
>  /* tear down the binding between this driver and the pci device */
>  
>  static void net2280_remove(struct pci_dev *pdev)
> @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>   device_remove_file(>dev, _attr_registers);
>  
>   ep_info(dev, "unbind\n");
> +
> + kfree(dev);
>  }
>  
>  /* wrap this driver around the specified device, but
> @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const 
> struct pci_device_id *id)
>   if (retval)
>   goto done;
>  
> - retval = usb_add_gadget_udc_release(>dev, >gadget,
> - gadget_release);
> + retval = usb_add_gadget_udc(>dev, >gadget);
>   if (retval)
>   goto done;
>   return 0;

Maybe...  But I can't shake the feeling that Greg KH would strongly 
disagree.  Hasn't he said, many times in the past, that any dynamically 
allocated device structure _must_ have a real release routine?  
usb_udc_nop_release() doesn't qualify.

The issue is outstanding references to gadget.dev.  The driver core
does not guarantee that all references will have been dropped by the
time device_unregister() returns.  So what if some other part of the
kernel still has a reference to gadget.dev when we reach the end of
net2280_remove() and deallocate the private structure?

Unless you can be certain that there are no outstanding references when
the gadget is unregistered, this approach isn't safe.  (And even if you
can be certain, how can you know that future changes to the kernel
won't affect the situation?)

> > And it doesn't help with the fact that net2280_remove() continues to 
> > access the private data structure after calling usb_del_gadget_udc().  
> > Strictly speaking, that routine should do
> >
> > get_device(>gadget.dev);
> >
> > at the start, with a corresponding put_device() at the end.
> >
> > There's another problem.  Suppose a call to 
> > usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> > error pathway does put_device(>dev).  This will invoke the 
> > release callback, deallocating the private data structure without 
> > giving the caller (i.e., the UDC driver) a chance to clean up.
> 
> it won't deallocate anything :-) dev_set_drvdata() was never called,
> we will endup with kfree(NULL) which is safe and just silently returns.

But if you change gadget_release() to use the parent's drvdata field, 
like you proposed earlier, and fix up the refcounting in 
net2280_remove() so that the structure doesn't get deallocated too 
quickly, then this does become a real problem.

And it can affect other UDC drivers, not just net2280.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Alan Stern  writes:
>> >> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget 
>> >> >> >> *gadget)
>> >> >> >> flush_work(>work);
>> >> >> >> device_unregister(>dev);
>> >> >> >> device_unregister(>dev);
>> >> >> >> +   memset(>dev, 0x00, sizeof(gadget->dev));
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >> >
>> >> >> > Isn't this dangerous?  It's quite possible that the 
>> >> >> > device_unregister() 
>> >> >> 
>> >> >> not on the gadget API, no.
>> >> >> 
>> >> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> >> > which might deallocate gadget.  If that happens, your new memset 
>> >> >> > will 
>> >> >> > oops.
>> >> >> 
>> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> >> structure, like this:
>> >> >> 
>> >> >> struct dwc3 {
>> >> >>[...]
>> >> >>struct usb_gadget   gadget;
>> >> >>struct usb_gadget_driver *gadget_driver;
>> >> >>[...]
>> >> >> };
>> >> >
>> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> >> > usb_gadget to control the lifetime of its private structure?
>> >> 
>> >> nope, not being used. At least not yet.
>> >
>> > I'm not convinced (yet)...
>> >
>> >> > (By the way, can you tell what's going on in net2280.c?  I must be
>> >> > missing something; it looks like gadget_release() would quickly run
>> >> > into problems because it calls dev_get_drvdata() for >dev, but
>> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> >> > final put.)
>> >> 
>> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id 
>> >> *id)
>> >> {
>> >>   struct net2280  *dev;
>> >>   unsigned long   resource, len;
>> >>   void__iomem *base = NULL;
>> >>   int retval, i;
>> >> 
>> >>   /* alloc, and start init */
>> >>   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> >>   if (dev == NULL) {
>> >>   retval = -ENOMEM;
>> >>   goto done;
>> >>   }
>> >> 
>> >>   pci_set_drvdata(pdev, dev);
>> >>   ^^^
>> >
>> > That sets the driver data in the struct pci_dev, not in
>> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
>> > driver data in dev->gadget.dev.
>> 
>> hmmm, indeed. The same is happening with other callers of
>> usb_add_gadget_udc_release().
>> 
>> I guess this should be enough?
>> 
>> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>>  
>>  static void gadget_release(struct device *_dev)
>>  {
>> -struct net2280  *dev = dev_get_drvdata(_dev);
>> +struct net2280  *dev = dev_get_drvdata(_dev->parent);
>>  
>>  kfree(dev);
>>  }
>
> Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> up every time it gets called, in its current form.

Well, it does :-)

dev_get_drvdata(_dev) -> NULL -> kfree(NULL)

We're just leaking memory. I guess a patch like below would be best:

diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 3828c2ec8623..4dc04253da61 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 /*-*/
 
-static void gadget_release(struct device *_dev)
-{
-   struct net2280  *dev = dev_get_drvdata(_dev);
-
-   kfree(dev);
-}
-
 /* tear down the binding between this driver and the pci device */
 
 static void net2280_remove(struct pci_dev *pdev)
@@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
device_remove_file(>dev, _attr_registers);
 
ep_info(dev, "unbind\n");
+
+   kfree(dev);
 }
 
 /* wrap this driver around the specified device, but
@@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const 
struct pci_device_id *id)
if (retval)
goto done;
 
-   retval = usb_add_gadget_udc_release(>dev, >gadget,
-   gadget_release);
+   retval = usb_add_gadget_udc(>dev, >gadget);
if (retval)
goto done;
return 0;

> And it doesn't help with the fact that net2280_remove() continues to 
> access the private data structure after calling usb_del_gadget_udc().  
> Strictly speaking, that routine should do
>
>   get_device(>gadget.dev);
>
> at the start, with a corresponding put_device() at the end.
>
> There's another problem.  Suppose a call to 
> usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> error pathway does put_device(>dev).  This will invoke the 
> 

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Mon, 10 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern  writes:
> > On Wed, 5 Apr 2017, Felipe Balbi wrote:
> >
> >> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget 
> >> >> >> *gadget)
> >> >> >>  flush_work(>work);
> >> >> >>  device_unregister(>dev);
> >> >> >>  device_unregister(>dev);
> >> >> >> +memset(>dev, 0x00, sizeof(gadget->dev));
> >> >> >>  }
> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >> >
> >> >> > Isn't this dangerous?  It's quite possible that the 
> >> >> > device_unregister() 
> >> >> 
> >> >> not on the gadget API, no.
> >> >> 
> >> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> >> > oops.
> >> >> 
> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> >> structure, like this:
> >> >> 
> >> >> struct dwc3 {
> >> >> [...]
> >> >> struct usb_gadget   gadget;
> >> >> struct usb_gadget_driver *gadget_driver;
> >> >> [...]
> >> >> };
> >> >
> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> >> > usb_gadget to control the lifetime of its private structure?
> >> 
> >> nope, not being used. At least not yet.
> >
> > I'm not convinced (yet)...
> >
> >> > (By the way, can you tell what's going on in net2280.c?  I must be
> >> > missing something; it looks like gadget_release() would quickly run
> >> > into problems because it calls dev_get_drvdata() for >dev, but
> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
> >> > final put.)
> >> 
> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id 
> >> *id)
> >> {
> >>struct net2280  *dev;
> >>unsigned long   resource, len;
> >>void__iomem *base = NULL;
> >>int retval, i;
> >> 
> >>/* alloc, and start init */
> >>dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >>if (dev == NULL) {
> >>retval = -ENOMEM;
> >>goto done;
> >>}
> >> 
> >>pci_set_drvdata(pdev, dev);
> >>^^^
> >
> > That sets the driver data in the struct pci_dev, not in
> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> > driver data in dev->gadget.dev.
> 
> hmmm, indeed. The same is happening with other callers of
> usb_add_gadget_udc_release().
> 
> I guess this should be enough?
> 
> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  static void gadget_release(struct device *_dev)
>  {
> - struct net2280  *dev = dev_get_drvdata(_dev);
> + struct net2280  *dev = dev_get_drvdata(_dev->parent);
>  
>   kfree(dev);
>  }

Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
up every time it gets called, in its current form.

And it doesn't help with the fact that net2280_remove() continues to 
access the private data structure after calling usb_del_gadget_udc().  
Strictly speaking, that routine should do

get_device(>gadget.dev);

at the start, with a corresponding put_device() at the end.

There's another problem.  Suppose a call to 
usb_add_gadget_udc_release() fails.  At the end of that routine, the 
error pathway does put_device(>dev).  This will invoke the 
release callback, deallocating the private data structure without 
giving the caller (i.e., the UDC driver) a chance to clean up.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Wed, 5 Apr 2017, Felipe Balbi wrote:
>
>> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget 
>> >> >> *gadget)
>> >> >>flush_work(>work);
>> >> >>device_unregister(>dev);
>> >> >>device_unregister(>dev);
>> >> >> +  memset(>dev, 0x00, sizeof(gadget->dev));
>> >> >>  }
>> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >
>> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> >> 
>> >> not on the gadget API, no.
>> >> 
>> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> > which might deallocate gadget.  If that happens, your new memset will 
>> >> > oops.
>> >> 
>> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> structure, like this:
>> >> 
>> >> struct dwc3 {
>> >>   [...]
>> >>   struct usb_gadget   gadget;
>> >>   struct usb_gadget_driver *gadget_driver;
>> >>   [...]
>> >> };
>> >
>> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> > usb_gadget to control the lifetime of its private structure?
>> 
>> nope, not being used. At least not yet.
>
> I'm not convinced (yet)...
>
>> > (By the way, can you tell what's going on in net2280.c?  I must be
>> > missing something; it looks like gadget_release() would quickly run
>> > into problems because it calls dev_get_drvdata() for >dev, but
>> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> > final put.)
>> 
>> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id 
>> *id)
>> {
>>  struct net2280  *dev;
>>  unsigned long   resource, len;
>>  void__iomem *base = NULL;
>>  int retval, i;
>> 
>>  /* alloc, and start init */
>>  dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>  if (dev == NULL) {
>>  retval = -ENOMEM;
>>  goto done;
>>  }
>> 
>>  pci_set_drvdata(pdev, dev);
>>  ^^^
>
> That sets the driver data in the struct pci_dev, not in
> dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> driver data in dev->gadget.dev.

hmmm, indeed. The same is happening with other callers of
usb_add_gadget_udc_release().

I guess this should be enough?

@@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 static void gadget_release(struct device *_dev)
 {
-   struct net2280  *dev = dev_get_drvdata(_dev);
+   struct net2280  *dev = dev_get_drvdata(_dev->parent);
 
kfree(dev);
 }


> (Even after all these years, I still get bothered by the way Dave 
> Brownell used to call everything "dev"...  IIRC, at one time he had a 
> line of code that went something like:  dev->dev.dev = >dev !)

:-)

>> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> >> device at all. Just a pointer to a device, that would solve all these
>> >> issues.
>> >
>> > A pointer to which device?  The UDC?  That would change the directory 
>> > layout in sysfs.
>> 
>> indeed. Would that be a problem?
>
> Possibly for some userspace tool.

yeah, we can do dynamic allocation of the device pointer, no issue.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Wed, 5 Apr 2017, Felipe Balbi wrote:

> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >> >> flush_work(>work);
> >> >> device_unregister(>dev);
> >> >> device_unregister(>dev);
> >> >> +   memset(>dev, 0x00, sizeof(gadget->dev));
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >
> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> >> 
> >> not on the gadget API, no.
> >> 
> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> > oops.
> >> 
> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> structure, like this:
> >> 
> >> struct dwc3 {
> >>[...]
> >>struct usb_gadget   gadget;
> >>struct usb_gadget_driver *gadget_driver;
> >>[...]
> >> };
> >
> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> > usb_gadget to control the lifetime of its private structure?
> 
> nope, not being used. At least not yet.

I'm not convinced (yet)...

> > (By the way, can you tell what's going on in net2280.c?  I must be
> > missing something; it looks like gadget_release() would quickly run
> > into problems because it calls dev_get_drvdata() for >dev, but
> > net2280_probe() never calls dev_set_drvdata() for that device.  
> > Furthermore, net2280_remove() continues to reference the net2280 struct
> > after calling usb_del_gadget_udc(), and it never does seem to do a
> > final put.)
> 
> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
>   struct net2280  *dev;
>   unsigned long   resource, len;
>   void__iomem *base = NULL;
>   int retval, i;
> 
>   /* alloc, and start init */
>   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>   if (dev == NULL) {
>   retval = -ENOMEM;
>   goto done;
>   }
> 
>   pci_set_drvdata(pdev, dev);
>   ^^^

That sets the driver data in the struct pci_dev, not in
dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
driver data in dev->gadget.dev.

(Even after all these years, I still get bothered by the way Dave 
Brownell used to call everything "dev"...  IIRC, at one time he had a 
line of code that went something like:  dev->dev.dev = >dev !)

> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
> >> device at all. Just a pointer to a device, that would solve all these
> >> issues.
> >
> > A pointer to which device?  The UDC?  That would change the directory 
> > layout in sysfs.
> 
> indeed. Would that be a problem?

Possibly for some userspace tool.

> > Or a pointer to a separate dynamically allocated device (the way struct 
> > usb_hcd contains a pointer to the root hub device)?  That would work.  
> > If the UDC driver wanted to re-register the gadget, it would have to 
> > allocate a new device.
> 
> That could be done as well, if maintaining the directory structure is a
> must.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Alan Stern  writes:
>> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> >> repeatedly on the same gadget->dev structure.
>> >> 
>> >> We need to clear the gadget->dev structure so that kobject_init()
>> >> doesn't complain about already initialized object.
>> >> 
>> >> Signed-off-by: Roger Quadros 
>> >> ---
>> >>  drivers/usb/gadget/udc/core.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >> 
>> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> >> index d685d82..efce68e 100644
>> >> --- a/drivers/usb/gadget/udc/core.c
>> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >>   flush_work(>work);
>> >>   device_unregister(>dev);
>> >>   device_unregister(>dev);
>> >> + memset(>dev, 0x00, sizeof(gadget->dev));
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >
>> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> 
>> not on the gadget API, no.
>> 
>> > call on the previous line invokes the gadget->dev.release callback, 
>> > which might deallocate gadget.  If that happens, your new memset will 
>> > oops.
>> 
>> that won't happen. struct usb_gadget is a member of the UDC's private
>> structure, like this:
>> 
>> struct dwc3 {
>>  [...]
>>  struct usb_gadget   gadget;
>>  struct usb_gadget_driver *gadget_driver;
>>  [...]
>> };
>
> Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> usb_gadget to control the lifetime of its private structure?

nope, not being used. At least not yet.

> (By the way, can you tell what's going on in net2280.c?  I must be
> missing something; it looks like gadget_release() would quickly run
> into problems because it calls dev_get_drvdata() for >dev, but
> net2280_probe() never calls dev_set_drvdata() for that device.  
> Furthermore, net2280_remove() continues to reference the net2280 struct
> after calling usb_del_gadget_udc(), and it never does seem to do a
> final put.)

static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct net2280  *dev;
unsigned long   resource, len;
void__iomem *base = NULL;
int retval, i;

/* alloc, and start init */
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (dev == NULL) {
retval = -ENOMEM;
goto done;
}

pci_set_drvdata(pdev, dev);
^^^

>> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> device at all. Just a pointer to a device, that would solve all these
>> issues.
>
> A pointer to which device?  The UDC?  That would change the directory 
> layout in sysfs.

indeed. Would that be a problem?

> Or a pointer to a separate dynamically allocated device (the way struct 
> usb_hcd contains a pointer to the root hub device)?  That would work.  
> If the UDC driver wanted to re-register the gadget, it would have to 
> allocate a new device.

That could be done as well, if maintaining the directory structure is a
must.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Tue, 4 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern  writes:
> > On Mon, 3 Apr 2017, Roger Quadros wrote:
> >
> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> >> repeatedly on the same gadget->dev structure.
> >> 
> >> We need to clear the gadget->dev structure so that kobject_init()
> >> doesn't complain about already initialized object.
> >> 
> >> Signed-off-by: Roger Quadros 
> >> ---
> >>  drivers/usb/gadget/udc/core.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> >> index d685d82..efce68e 100644
> >> --- a/drivers/usb/gadget/udc/core.c
> >> +++ b/drivers/usb/gadget/udc/core.c
> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >>flush_work(>work);
> >>device_unregister(>dev);
> >>device_unregister(>dev);
> >> +  memset(>dev, 0x00, sizeof(gadget->dev));
> >>  }
> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >
> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> 
> not on the gadget API, no.
> 
> > call on the previous line invokes the gadget->dev.release callback, 
> > which might deallocate gadget.  If that happens, your new memset will 
> > oops.
> 
> that won't happen. struct usb_gadget is a member of the UDC's private
> structure, like this:
> 
> struct dwc3 {
>   [...]
>   struct usb_gadget   gadget;
>   struct usb_gadget_driver *gadget_driver;
>   [...]
> };

Yes.  So what?  Can't the UDC driver use the refcount inside struct 
usb_gadget to control the lifetime of its private structure?

(By the way, can you tell what's going on in net2280.c?  I must be
missing something; it looks like gadget_release() would quickly run
into problems because it calls dev_get_drvdata() for >dev, but
net2280_probe() never calls dev_set_drvdata() for that device.  
Furthermore, net2280_remove() continues to reference the net2280 struct
after calling usb_del_gadget_udc(), and it never does seem to do a
final put.)

> I'm actually thinking that struct usb_gadget shouldn't have a struct
> device at all. Just a pointer to a device, that would solve all these
> issues.

A pointer to which device?  The UDC?  That would change the directory 
layout in sysfs.

Or a pointer to a separate dynamically allocated device (the way struct 
usb_hcd contains a pointer to the root hub device)?  That would work.  
If the UDC driver wanted to re-register the gadget, it would have to 
allocate a new device.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Mon, 3 Apr 2017, Roger Quadros wrote:
>
>> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> repeatedly on the same gadget->dev structure.
>> 
>> We need to clear the gadget->dev structure so that kobject_init()
>> doesn't complain about already initialized object.
>> 
>> Signed-off-by: Roger Quadros 
>> ---
>>  drivers/usb/gadget/udc/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index d685d82..efce68e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>>  flush_work(>work);
>>  device_unregister(>dev);
>>  device_unregister(>dev);
>> +memset(>dev, 0x00, sizeof(gadget->dev));
>>  }
>>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>
> Isn't this dangerous?  It's quite possible that the device_unregister() 

not on the gadget API, no.

> call on the previous line invokes the gadget->dev.release callback, 
> which might deallocate gadget.  If that happens, your new memset will 
> oops.

that won't happen. struct usb_gadget is a member of the UDC's private
structure, like this:

struct dwc3 {
[...]
struct usb_gadget   gadget;
struct usb_gadget_driver *gadget_driver;
[...]
};

I'm actually thinking that struct usb_gadget shouldn't have a struct
device at all. Just a pointer to a device, that would solve all these
issues.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Mon, 3 Apr 2017, Roger Quadros wrote:

> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> repeatedly on the same gadget->dev structure.
> 
> We need to clear the gadget->dev structure so that kobject_init()
> doesn't complain about already initialized object.
> 
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/gadget/udc/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index d685d82..efce68e 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>   flush_work(>work);
>   device_unregister(>dev);
>   device_unregister(>dev);
> + memset(>dev, 0x00, sizeof(gadget->dev));
>  }
>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);

Isn't this dangerous?  It's quite possible that the device_unregister() 
call on the previous line invokes the gadget->dev.release callback, 
which might deallocate gadget.  If that happens, your new memset will 
oops.

In general, if an object relies on reference counting for its lifetime, 
you cannot register and unregister it more than once.  A typical issue 
is that some code retains a reference to the old instance and tries to 
use it after the new instance has been registered, thereby messing up 
the new instance.  I don't know if that is possible in this case, but 
it is something to watch out for.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html