| Message ID | 20220407022204.10730-1-hbh25y@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: usbip: fix a refcount leak in stub_probe() | expand |
On 4/6/22 8:22 PM, Hangyu Hua wrote: > usb_get_dev() is called in stub_device_alloc(). When stub_probe() fails > after that, usb_put_dev() needs to be called to release the reference. > > Fix this by moving usb_put_dev() to sdev_free error path handling. > > Fixes: 3ff67445750a ("usbip: fix error handling in stub_probe()") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> > --- > > v2: add more description of this patch. Still missing details on how this problem was found. Please add them to the commit log. thanks, -- Shuah
Hi Shuah, I find this by code review. Do i really need to add this to commit log? This look like a little weird. Thanks On 2022/4/8 00:31, Shuah Khan wrote: > On 4/6/22 8:22 PM, Hangyu Hua wrote: >> usb_get_dev() is called in stub_device_alloc(). When stub_probe() fails >> after that, usb_put_dev() needs to be called to release the reference. >> >> Fix this by moving usb_put_dev() to sdev_free error path handling. >> >> Fixes: 3ff67445750a ("usbip: fix error handling in stub_probe()") >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> >> --- >> >> v2: add more description of this patch. > > Still missing details on how this problem was found. Please add them > to the commit log. > > thanks, > -- Shuah
On 4/7/22 7:59 PM, Hangyu Hua wrote: > Hi Shuah, > > I find this by code review. Do i really need to add this to commit log? This look like a little weird. > Great. Good find. It is important to understand how the problem is found. Please add it the change log. We usually expect dmesg or such info. that revealed refcount leak, since this one is found during code review, we would like to see that information the commit log. Also please remember to avoid top posting. > > On 2022/4/8 00:31, Shuah Khan wrote: >> On 4/6/22 8:22 PM, Hangyu Hua wrote: >>> usb_get_dev() is called in stub_device_alloc(). When stub_probe() fails >>> after that, usb_put_dev() needs to be called to release the reference. >>> >>> Fix this by moving usb_put_dev() to sdev_free error path handling. >>> >>> Fixes: 3ff67445750a ("usbip: fix error handling in stub_probe()") >>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> >>> --- >>> >>> v2: add more description of this patch. >> >> Still missing details on how this problem was found. Please add them >> to the commit log. >> thanks, -- Shuah
On 2022/4/8 23:04, Shuah Khan wrote: > On 4/7/22 7:59 PM, Hangyu Hua wrote: >> Hi Shuah, >> >> I find this by code review. Do i really need to add this to commit >> log? This look like a little weird. >> > > Great. Good find. > > It is important to understand how the problem is found. Please add it > the change log. We usually expect dmesg or such info. that revealed > refcount leak, since this one is found during code review, we would > like to see that information the commit log. > > Also please remember to avoid top posting. I get what you meant now. But i don't know how to get a clear dmesg or any other log. The kernel will not crash because of this. I just used gdb to find that udev->dev->kobj->kref gets bigger and bigger whenever I call stub_probe with busid_priv->status = STUB_BUSID_REMOV. Thanks for telling me the rules. > >> >> On 2022/4/8 00:31, Shuah Khan wrote: >>> On 4/6/22 8:22 PM, Hangyu Hua wrote: >>>> usb_get_dev() is called in stub_device_alloc(). When stub_probe() fails >>>> after that, usb_put_dev() needs to be called to release the reference. >>>> >>>> Fix this by moving usb_put_dev() to sdev_free error path handling. >>>> >>>> Fixes: 3ff67445750a ("usbip: fix error handling in stub_probe()") >>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >>>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> >>>> --- >>>> >>>> v2: add more description of this patch. >>> >>> Still missing details on how this problem was found. Please add them >>> to the commit log. >>> > > thanks, > -- Shuah >
On 4/10/22 11:29 PM, Hangyu Hua wrote: > On 2022/4/8 23:04, Shuah Khan wrote: >> On 4/7/22 7:59 PM, Hangyu Hua wrote: >>> Hi Shuah, >>> >>> I find this by code review. Do i really need to add this to commit log? This look like a little weird. >>> >> >> Great. Good find. >> >> It is important to understand how the problem is found. Please add it >> the change log. We usually expect dmesg or such info. that revealed >> refcount leak, since this one is found during code review, we would >> like to see that information the commit log. >> >> Also please remember to avoid top posting. > > I get what you meant now. But i don't know how to get a clear dmesg or any other log. The kernel will not crash because of this. I just used gdb to find that udev->dev->kobj->kref gets bigger and bigger whenever I call stub_probe with busid_priv->status = STUB_BUSID_REMOV. > > Thanks for telling me the rules. > There is no need to gather dmesg etc. Just add a note that you found the problem during code review. Having complete information about why a change is made will be helpful for future changes to this code and somebody new trying understand the changes made to this file/routine and why. thanks, -- Shuah
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c index d8d3892e5a69..3c6d452e3bf4 100644 --- a/drivers/usb/usbip/stub_dev.c +++ b/drivers/usb/usbip/stub_dev.c @@ -393,7 +393,6 @@ static int stub_probe(struct usb_device *udev) err_port: dev_set_drvdata(&udev->dev, NULL); - usb_put_dev(udev); /* we already have busid_priv, just lock busid_lock */ spin_lock(&busid_priv->busid_lock); @@ -408,6 +407,7 @@ static int stub_probe(struct usb_device *udev) put_busid_priv(busid_priv); sdev_free: + usb_put_dev(udev); stub_device_free(sdev); return rc;
usb_get_dev() is called in stub_device_alloc(). When stub_probe() fails after that, usb_put_dev() needs to be called to release the reference. Fix this by moving usb_put_dev() to sdev_free error path handling. Fixes: 3ff67445750a ("usbip: fix error handling in stub_probe()") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> --- v2: add more description of this patch. drivers/usb/usbip/stub_dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)