| Message ID | 20200810022510.6516-7-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: UDC: Fix memory leaks by expanding the API | expand |
On Mon, Aug 10, 2020 at 10:25:10AM +0800, Peter Chen wrote: > We have already allocated gadget structure dynamically at UDC (dwc3) > driver, so commit fac323471df6 ("usb: udc: allow adding and removing > the same gadget device")could be reverted. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > Changes from v1: > - No changes. > > drivers/usb/gadget/udc/core.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 473e74088b1f..43351b0af569 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) > { > usb_del_gadget(gadget); > usb_put_gadget(gadget); > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); Shouldn't you do this patch earlier in the series, as the usb_put_gadget() call could have freed the memory that is being cleared here? Otherwise, this series looks good, thanks for doing it. greg k-h
> > > > diff --git a/drivers/usb/gadget/udc/core.c > > b/drivers/usb/gadget/udc/core.c index 473e74088b1f..43351b0af569 > > 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget > > *gadget) { > > usb_del_gadget(gadget); > > usb_put_gadget(gadget); > > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > Shouldn't you do this patch earlier in the series, as the > usb_put_gadget() call could have freed the memory that is being cleared here? > If I did it earlier, it would cause dwc3 break if people do 'git bisect', dwc3 issue is fixed at patch 5. Peter > Otherwise, this series looks good, thanks for doing it. > > greg k-h
On Tue, Aug 18, 2020 at 10:05:51AM +0000, Peter Chen wrote: > > > > > > > diff --git a/drivers/usb/gadget/udc/core.c > > > b/drivers/usb/gadget/udc/core.c index 473e74088b1f..43351b0af569 > > > 100644 > > > --- a/drivers/usb/gadget/udc/core.c > > > +++ b/drivers/usb/gadget/udc/core.c > > > @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget > > > *gadget) { > > > usb_del_gadget(gadget); > > > usb_put_gadget(gadget); > > > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > > > Shouldn't you do this patch earlier in the series, as the > > usb_put_gadget() call could have freed the memory that is being cleared here? > > > > If I did it earlier, it would cause dwc3 break if people do 'git bisect', dwc3 issue is > fixed at patch 5. If you use the patch I posted earlier: https://marc.info/?l=linux-usb&m=159605415210351&w=2 instead of this one then dwc3 would continue to work correctly during the intermediate stages of the series. Alan Stern
On 20-08-18 10:46:55, stern@rowland.harvard.edu wrote: > On Tue, Aug 18, 2020 at 10:05:51AM +0000, Peter Chen wrote: > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/core.c > > > > b/drivers/usb/gadget/udc/core.c index 473e74088b1f..43351b0af569 > > > > 100644 > > > > --- a/drivers/usb/gadget/udc/core.c > > > > +++ b/drivers/usb/gadget/udc/core.c > > > > @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget > > > > *gadget) { > > > > usb_del_gadget(gadget); > > > > usb_put_gadget(gadget); > > > > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > > > > > Shouldn't you do this patch earlier in the series, as the > > > usb_put_gadget() call could have freed the memory that is being cleared here? > > > > > > > If I did it earlier, it would cause dwc3 break if people do 'git bisect', dwc3 issue is > > fixed at patch 5. > > If you use the patch I posted earlier: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-usb%26m%3D159605415210351%26w%3D2&data=02%7C01%7Cpeter.chen%40nxp.com%7C84c12532be684ba94c1708d843858e86%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637333588196922016&sdata=gOe5kecj38gR9qIbkfjVkNO%2FICp0bHis30Yi2tomrc8%3D&reserved=0 > > instead of this one then dwc3 would continue to work correctly during > the intermediate stages of the series. > But at last, we don't want below at .release function memset(dev, 0, sizeof(*dev)); It still needs another patch to delete it after dwc3 changes, and it changes .release function name to usb_udc_zero_release, this change may also not be needed. Or I only do move memory clear operation at the first patch, and delete it at the last patch, it could let the reader not see the memory clear operation at the usb_del_gadget during the patch series.
On Wed, Aug 19, 2020 at 01:31:14AM +0000, Peter Chen wrote: > On 20-08-18 10:46:55, stern@rowland.harvard.edu wrote: > > On Tue, Aug 18, 2020 at 10:05:51AM +0000, Peter Chen wrote: > > > > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/core.c > > > > > b/drivers/usb/gadget/udc/core.c index 473e74088b1f..43351b0af569 > > > > > 100644 > > > > > --- a/drivers/usb/gadget/udc/core.c > > > > > +++ b/drivers/usb/gadget/udc/core.c > > > > > @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget > > > > > *gadget) { > > > > > usb_del_gadget(gadget); > > > > > usb_put_gadget(gadget); > > > > > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > > > > > > > Shouldn't you do this patch earlier in the series, as the > > > > usb_put_gadget() call could have freed the memory that is being cleared here? > > > > > > > > > > If I did it earlier, it would cause dwc3 break if people do 'git bisect', dwc3 issue is > > > fixed at patch 5. > > > > If you use the patch I posted earlier: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-usb%26m%3D159605415210351%26w%3D2&data=02%7C01%7Cpeter.chen%40nxp.com%7C84c12532be684ba94c1708d843858e86%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637333588196922016&sdata=gOe5kecj38gR9qIbkfjVkNO%2FICp0bHis30Yi2tomrc8%3D&reserved=0 > > > > instead of this one then dwc3 would continue to work correctly during > > the intermediate stages of the series. > > > > But at last, we don't want below at .release function > > memset(dev, 0, sizeof(*dev)); > > It still needs another patch to delete it after dwc3 changes, > and it changes .release function name to usb_udc_zero_release, > this change may also not be needed. > > Or I only do move memory clear operation at the first patch, and > delete it at the last patch, it could let the reader not see > the memory clear operation at the usb_del_gadget during the patch > series. One way or another, the existing code is wrong. I guess the best we can do for now is to let it remain wrong during the patch series, rather than changing it to be wrong in a different way. To put it another way, we already run the risk of clearing memory that has been freed. The series does not make that risk any worse, and it eventually fixes the problem. This means your patch should remain in its position at the end of the series. Alan Stern
On 20-08-19 10:52:36, stern@rowland.harvard.edu wrote: > On Wed, Aug 19, 2020 at 01:31:14AM +0000, Peter Chen wrote: > > On 20-08-18 10:46:55, stern@rowland.harvard.edu wrote: > > > On Tue, Aug 18, 2020 at 10:05:51AM +0000, Peter Chen wrote: > > > > > > > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/core.c > > > > > > b/drivers/usb/gadget/udc/core.c index 473e74088b1f..43351b0af569 > > > > > > 100644 > > > > > > --- a/drivers/usb/gadget/udc/core.c > > > > > > +++ b/drivers/usb/gadget/udc/core.c > > > > > > @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget > > > > > > *gadget) { > > > > > > usb_del_gadget(gadget); > > > > > > usb_put_gadget(gadget); > > > > > > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > > > > > > > > > Shouldn't you do this patch earlier in the series, as the > > > > > usb_put_gadget() call could have freed the memory that is being cleared here? > > > > > > > > > > > > > If I did it earlier, it would cause dwc3 break if people do 'git bisect', dwc3 issue is > > > > fixed at patch 5. > > > > > > If you use the patch I posted earlier: > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-usb%26m%3D159605415210351%26w%3D2&data=02%7C01%7Cpeter.chen%40nxp.com%7Cac0a92404ea34c230dd208d8444f83d3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637334455595715503&sdata=whweZozRiWD%2B4iRFz7zvEahWAqQYkQQ8tHlRSiU%2Fj7I%3D&reserved=0 > > > > > > instead of this one then dwc3 would continue to work correctly during > > > the intermediate stages of the series. > > > > > > > But at last, we don't want below at .release function > > > > memset(dev, 0, sizeof(*dev)); > > > > It still needs another patch to delete it after dwc3 changes, > > and it changes .release function name to usb_udc_zero_release, > > this change may also not be needed. > > > > Or I only do move memory clear operation at the first patch, and > > delete it at the last patch, it could let the reader not see > > the memory clear operation at the usb_del_gadget during the patch > > series. > > One way or another, the existing code is wrong. I guess the best we can > do for now is to let it remain wrong during the patch series, rather > than changing it to be wrong in a different way. > > To put it another way, we already run the risk of clearing memory that > has been freed. The series does not make that risk any worse, and it > eventually fixes the problem. > > This means your patch should remain in its position at the end of the > series. > Thank. If you think my sequence during the patch series is OK, would you please add your reviewed-by below? https://www.spinics.net/lists/linux-usb/msg199291.html
On Thu, Aug 20, 2020 at 03:40:06AM +0000, Peter Chen wrote: > > > > One way or another, the existing code is wrong. I guess the best we can > > do for now is to let it remain wrong during the patch series, rather > > than changing it to be wrong in a different way. > > > > To put it another way, we already run the risk of clearing memory that > > has been freed. The series does not make that risk any worse, and it > > eventually fixes the problem. > > > > This means your patch should remain in its position at the end of the > > series. > > > > Thank. > > If you think my sequence during the patch series is OK, would you > please add your reviewed-by below? > > https://www.spinics.net/lists/linux-usb/msg199291.html That wouldn't make sense; I can't add Reviewed-by: to patches that I wrote myself. They already have my Signed-off-by:. However, it is okay to add Reviewed-by: Alan Stern <stern@rowland.harvard.edu> to the 6/6 patch. Alan Stern
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 473e74088b1f..43351b0af569 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -1386,7 +1386,6 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) { usb_del_gadget(gadget); usb_put_gadget(gadget); - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); } EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
We have already allocated gadget structure dynamically at UDC (dwc3) driver, so commit fac323471df6 ("usb: udc: allow adding and removing the same gadget device")could be reverted. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Peter Chen <peter.chen@nxp.com> --- Changes from v1: - No changes. drivers/usb/gadget/udc/core.c | 1 - 1 file changed, 1 deletion(-)