| Message ID | 20220126094126.923798-1-xy521521@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-next] xhci: fix two places when dealing with return value of function xhci_check_args | expand |
On Wed, Jan 26, 2022 at 05:41:26PM +0800, Hongyu Xie wrote: > From: Hongyu Xie <xiehongyu1@kylinos.cn> > > xhci_check_args returns 4 types of value, -ENODEV, -EINVAL, 1 and 0. > xhci_urb_enqueue and xhci_check_streams_endpoint return -EINVAL if > the return value of xhci_check_args <= 0. > This will cause a problem. What problem? > For example, r8152_submit_rx calling usb_submit_urb in > drivers/net/usb/r8152.c. > r8152_submit_rx will never get -ENODEV after submiting an urb > when xHC is halted, > because xhci_urb_enqueue returns -EINVAL in the very beginning. > > Fixes: 203a86613fb3 ("xhci: Avoid NULL pointer deref when host dies.") > Cc: stable@vger.kernel.org > Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn> > --- > drivers/usb/host/xhci.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index dc357cabb265..a7a55dd206fe 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1604,9 +1604,12 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag > struct urb_priv *urb_priv; > int num_tds; > > - if (!urb || xhci_check_args(hcd, urb->dev, urb->ep, > - true, true, __func__) <= 0) > + if (!urb) > return -EINVAL; > + ret = xhci_check_args(hcd, urb->dev, urb->ep, > + true, true, __func__); > + if (ret <= 0) > + return ret; So if 0 is returned, you will now return that here, is that ok? That is a change in functionality. But this can only ever be the case for a root hub, is that ok? > > slot_id = urb->dev->slot_id; > ep_index = xhci_get_endpoint_index(&urb->ep->desc); > @@ -3323,7 +3326,7 @@ static int xhci_check_streams_endpoint(struct xhci_hcd *xhci, > return -EINVAL; > ret = xhci_check_args(xhci_to_hcd(xhci), udev, ep, 1, true, __func__); > if (ret <= 0) > - return -EINVAL; > + return ret; Again, this means all is good? Why is this being called for a root hub? thanks, greg k-h
1."What problem? r8152_submit_rx needs to detach netdev if -ENODEV happened, but -ENODEV will never happen because xhci_urb_enqueue only returns -EINVAL if the return value of xhci_check_args <= 0. So r8152_submit_rx will will call napi_schedule to re-submit that urb, and this will cause infinite urb submission. The whole point is, if xhci_check_args returns value A, xhci_urb_enqueque shouldn't return any other value, because that will change some driver's behavior(like r8152.c). 2."So if 0 is returned, you will now return that here, is that ok? That is a change in functionality. But this can only ever be the case for a root hub, is that ok?" It's the same logic, but now xhci_urb_enqueue can return -ENODEV if xHC is halted. If it happens on a root hub, xhci_urb_enqueue won't be called. 3."Again, this means all is good? Why is this being called for a root hub?" It is the same logic with the old one, but now xhci_check_streams_endpoint can return -ENODEV if xHC is halted. thanks Hongyu Xie On Tue, 25 Jan 2022 at 22:02, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Jan 26, 2022 at 05:41:26PM +0800, Hongyu Xie wrote: >> From: Hongyu Xie <xiehongyu1@kylinos.cn> >> >> xhci_check_args returns 4 types of value, -ENODEV, -EINVAL, 1 and 0. >> xhci_urb_enqueue and xhci_check_streams_endpoint return -EINVAL if >> the return value of xhci_check_args <= 0. >> This will cause a problem. > What problem? > >> For example, r8152_submit_rx calling usb_submit_urb in >> drivers/net/usb/r8152.c. >> r8152_submit_rx will never get -ENODEV after submiting an urb >> when xHC is halted, >> because xhci_urb_enqueue returns -EINVAL in the very beginning. >> >> Fixes: 203a86613fb3 ("xhci: Avoid NULL pointer deref when host dies.") >> Cc: stable@vger.kernel.org >> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn> >> --- >> drivers/usb/host/xhci.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index dc357cabb265..a7a55dd206fe 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -1604,9 +1604,12 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag >> struct urb_priv *urb_priv; >> int num_tds; >> >> - if (!urb || xhci_check_args(hcd, urb->dev, urb->ep, >> - true, true, __func__) <= 0) >> + if (!urb) >> return -EINVAL; >> + ret = xhci_check_args(hcd, urb->dev, urb->ep, >> + true, true, __func__); >> + if (ret <= 0) >> + return ret; > So if 0 is returned, you will now return that here, is that ok? > That is a change in functionality. > > But this can only ever be the case for a root hub, is that ok? > >> >> slot_id = urb->dev->slot_id; >> ep_index = xhci_get_endpoint_index(&urb->ep->desc); >> @@ -3323,7 +3326,7 @@ static int xhci_check_streams_endpoint(struct xhci_hcd *xhci, >> return -EINVAL; >> ret = xhci_check_args(xhci_to_hcd(xhci), udev, ep, 1, true, __func__); >> if (ret <= 0) >> - return -EINVAL; >> + return ret; > Again, this means all is good? Why is this being called for a root hub? > > thanks, > > greg k-h
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Wed, Jan 26, 2022 at 06:22:45PM +0800, 谢泓宇 wrote: > 1."What problem? > r8152_submit_rx needs to detach netdev if -ENODEV happened, but -ENODEV will > never happen > because xhci_urb_enqueue only returns -EINVAL if the return value of > xhci_check_args <= 0. So > r8152_submit_rx will will call napi_schedule to re-submit that urb, and this > will cause infinite urb > submission. Odd line-wrapping... Anyway, why is this unique to this one driver? Why does it not show up for any other driver? > The whole point is, if xhci_check_args returns value A, xhci_urb_enqueque > shouldn't return any > other value, because that will change some driver's behavior(like r8152.c). But you are changing how the code currently works. Are you sure you want to have this "succeed" if this is on a root hub? > 2."So if 0 is returned, you will now return that here, is that ok? > That is a change in functionality. > But this can only ever be the case for a root hub, is that ok?" > > It's the same logic, but now xhci_urb_enqueue can return -ENODEV if xHC is > halted. > If it happens on a root hub, xhci_urb_enqueue won't be called. > > 3."Again, this means all is good? Why is this being called for a root hub?" > > It is the same logic with the old one, but now xhci_check_streams_endpoint > can return -ENODEV if xHC is halted. This still feels wrong to me, but I'll let the maintainer decide, as I don't understand why a root hub is special here. thanks, greg k-h
Hi Greg, On 2022/1/26 18:50, Greg KH wrote: > A: http://en.wikipedia.org/wiki/Top_post > Q: Were do I find info about this thing called top-posting? > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > http://daringfireball.net/2007/07/on_top > > On Wed, Jan 26, 2022 at 06:22:45PM +0800, 谢泓宇 wrote: >> 1."What problem? >> r8152_submit_rx needs to detach netdev if -ENODEV happened, but -ENODEV will >> never happen >> because xhci_urb_enqueue only returns -EINVAL if the return value of >> xhci_check_args <= 0. So >> r8152_submit_rx will will call napi_schedule to re-submit that urb, and this >> will cause infinite urb >> submission. > Odd line-wrapping... Sorry about my last reply. > > Anyway, why is this unique to this one driver? Why does it not show up > for any other driver? The whole thing is not about a particular driver. The thing is xhci_urb_enqueue shouldn't change the return value of xhci_check_args from -ENODEV to -EINVAL. Many other drivers only check if the return value of xchi_check_args is <= 0. > >> The whole point is, if xhci_check_args returns value A, xhci_urb_enqueque >> shouldn't return any >> other value, because that will change some driver's behavior(like r8152.c). > But you are changing how the code currently works. Are you sure you > want to have this "succeed" if this is on a root hub? Yes, I'm changing how the code currently works but not on a root hub. > >> 2."So if 0 is returned, you will now return that here, is that ok? >> That is a change in functionality. >> But this can only ever be the case for a root hub, is that ok?" >> >> It's the same logic, but now xhci_urb_enqueue can return -ENODEV if xHC is >> halted. >> If it happens on a root hub, xhci_urb_enqueue won't be called. >> >> 3."Again, this means all is good? Why is this being called for a root hub?" >> >> It is the same logic with the old one, but now xhci_check_streams_endpoint >> can return -ENODEV if xHC is halted. > This still feels wrong to me, but I'll let the maintainer decide, as I > don't understand why a root hub is special here. Thanks please. usb_submit_urb will call usb_hcd_submit_urb. And usb_hcd_submit_urb will call rh_urb_enqueue if it's on a root hub instead of calling hcd->driver->urb_enqueue(which is xhci_urb_enqueue in this case). > > thanks, > > greg k-h thanks, Hongyu Xie
On 26.1.2022 14.49, Hongyu Xie wrote: >> Anyway, why is this unique to this one driver? Why does it not show up >> for any other driver? > The whole thing is not about a particular driver. The thing is xhci_urb_enqueue shouldn't change the return value of xhci_check_args from -ENODEV to -EINVAL. Many other drivers only check if the return value of xchi_check_args is <= 0. Agree, lets return -ENODEV when appropriate. >> >>> The whole point is, if xhci_check_args returns value A, xhci_urb_enqueque >>> shouldn't return any >>> other value, because that will change some driver's behavior(like r8152.c). >> But you are changing how the code currently works. Are you sure you >> want to have this "succeed" if this is on a root hub? > Yes, I'm changing how the code currently works but not on a root hub. >> >>> 2."So if 0 is returned, you will now return that here, is that ok? >>> That is a change in functionality. >>> But this can only ever be the case for a root hub, is that ok?" >>> >>> It's the same logic, but now xhci_urb_enqueue can return -ENODEV if xHC is >>> halted. >>> If it happens on a root hub, xhci_urb_enqueue won't be called. >>> >>> 3."Again, this means all is good? Why is this being called for a root hub?" >>> >>> It is the same logic with the old one, but now xhci_check_streams_endpoint >>> can return -ENODEV if xHC is halted. >> This still feels wrong to me, but I'll let the maintainer decide, as I >> don't understand why a root hub is special here. > > Thanks please. usb_submit_urb will call usb_hcd_submit_urb. And usb_hcd_submit_urb will call rh_urb_enqueue if it's on a root hub instead of calling hcd->driver->urb_enqueue(which is xhci_urb_enqueue in this case). xhci_urb_enqueue() shouldn't be called for roothub urbs, but if it is then we should continue to return -EINVAL xhci_check_args() should be rewritten later, but first we want a targeted fix that can go to stable. Your original patch would be ok after following modification: if (ret <= 0) return ret ? ret : -EINVAL; Thanks -Mathias
Hi Mathias, On 2022/1/27 17:43, Mathias Nyman wrote: > On 26.1.2022 14.49, Hongyu Xie wrote: > >>> Anyway, why is this unique to this one driver? Why does it not show up >>> for any other driver? >> The whole thing is not about a particular driver. The thing is xhci_urb_enqueue shouldn't change the return value of xhci_check_args from -ENODEV to -EINVAL. Many other drivers only check if the return value of xchi_check_args is <= 0. > Agree, lets return -ENODEV when appropriate. > >>>> The whole point is, if xhci_check_args returns value A, xhci_urb_enqueque >>>> shouldn't return any >>>> other value, because that will change some driver's behavior(like r8152.c). >>> But you are changing how the code currently works. Are you sure you >>> want to have this "succeed" if this is on a root hub? >> Yes, I'm changing how the code currently works but not on a root hub. >>>> 2."So if 0 is returned, you will now return that here, is that ok? >>>> That is a change in functionality. >>>> But this can only ever be the case for a root hub, is that ok?" >>>> >>>> It's the same logic, but now xhci_urb_enqueue can return -ENODEV if xHC is >>>> halted. >>>> If it happens on a root hub, xhci_urb_enqueue won't be called. >>>> >>>> 3."Again, this means all is good? Why is this being called for a root hub?" >>>> >>>> It is the same logic with the old one, but now xhci_check_streams_endpoint >>>> can return -ENODEV if xHC is halted. >>> This still feels wrong to me, but I'll let the maintainer decide, as I >>> don't understand why a root hub is special here. >> Thanks please. usb_submit_urb will call usb_hcd_submit_urb. And usb_hcd_submit_urb will call rh_urb_enqueue if it's on a root hub instead of calling hcd->driver->urb_enqueue(which is xhci_urb_enqueue in this case). > xhci_urb_enqueue() shouldn't be called for roothub urbs, but if it is then we > should continue to return -EINVAL xhci_urb_enqueue() won't be called for roothub urbs, only for none roothub urbs(see usb_hcd_submit_urb()). So xhci_urb_enqueue() will not get 0 from xhci_check_args(). Still return -EINVAL if xhci_check_args() returns 0 in xhci_urb_enqueue()? > > xhci_check_args() should be rewritten later, but first we want a targeted fix > that can go to stable. > > Your original patch would be ok after following modification: > if (ret <= 0) > return ret ? ret : -EINVAL; I have two questions: 1) Why return -EINVAL for roothub urbs? 2) Should I change all the return statements about xhci_check_args() in drivers/usb/host/xhci.c? There are 6 of them. > > Thanks > -Mathias
Hi On 28.1.2022 5.48, 谢泓宇 wrote: > Hi Mathias, > >> xhci_urb_enqueue() shouldn't be called for roothub urbs, but if it is then we >> should continue to return -EINVAL > > xhci_urb_enqueue() won't be called for roothub urbs, only for none roothub urbs(see usb_hcd_submit_urb()).> > So xhci_urb_enqueue() will not get 0 from xhci_check_args(). > > Still return -EINVAL if xhci_check_args() returns 0 in xhci_urb_enqueue()? > Yes. That is what it used to return. This is more about code maintaining practice than this specific patch. Only make the necessary change to fix a bug, especially if the patch is going to stable kernels. The change to return success ("0") instead of -EINVAL in xhci_urb_enqueue() for roothub URBs is irrelevant in fixing your issue. Debugging future issues is a lot harder when there are small undocumented unrelated functional changes scattered around bugfixing patches. Other reason is that even if you can be almost certain xhci_urb_enqueue() won't be called for roothub urbs for this kernel version, it's possible some old stable kernel code looks very different, and this change can break that stable version. Seemingly odd checks in code can indicate the old original code was flawed, and quickly worked around by adding the odd check. That kernel version might still depend on this odd check even if newer versions are fixed properly. >> >> xhci_check_args() should be rewritten later, but first we want a targeted fix >> that can go to stable. >> >> Your original patch would be ok after following modification: >> if (ret <= 0) >> return ret ? ret : -EINVAL; > > I have two questions: > > 1) Why return -EINVAL for roothub urbs? - For all reasons stated above - Because it used to, and changing it doesn't fix anything - Because urbs sent to xhci_urb_enqueue() should have a valid urb->dev->parent, if they don't have it then they are INVALID > > 2) Should I change all the return statements about xhci_check_args() in drivers/usb/host/xhci.c? > > There are 6 of them. Only make sure your patch doesn't change the functionality unnecessarily. There are two places where we return -EINVAL if xhci_check_args() returns 0: xhci_urb_enqueue() and xhci_check_streams_endpoint() Keep that functionality. Thanks Mathias
Hi Mathias, On 2022/1/28 17:48, Mathias Nyman wrote: > Hi > > On 28.1.2022 5.48, 谢泓宇 wrote: >> Hi Mathias, >> >>> xhci_urb_enqueue() shouldn't be called for roothub urbs, but if it is then we >>> should continue to return -EINVAL >> xhci_urb_enqueue() won't be called for roothub urbs, only for none roothub urbs(see usb_hcd_submit_urb()).> >> So xhci_urb_enqueue() will not get 0 from xhci_check_args(). >> >> Still return -EINVAL if xhci_check_args() returns 0 in xhci_urb_enqueue()? >> > Yes. That is what it used to return. > This is more about code maintaining practice than this specific patch. > > Only make the necessary change to fix a bug, especially if the patch is going > to stable kernels. > The change to return success ("0") instead of -EINVAL in xhci_urb_enqueue() for > roothub URBs is irrelevant in fixing your issue. > > Debugging future issues is a lot harder when there are small undocumented > unrelated functional changes scattered around bugfixing patches. > > Other reason is that even if you can be almost certain xhci_urb_enqueue() won't > be called for roothub urbs for this kernel version, it's possible some old stable > kernel code looks very different, and this change can break that stable version. > > Seemingly odd checks in code can indicate the old original code was flawed, and > quickly worked around by adding the odd check. > That kernel version might still depend on this odd check even if newer versions > are fixed properly. > >>> xhci_check_args() should be rewritten later, but first we want a targeted fix >>> that can go to stable. >>> >>> Your original patch would be ok after following modification: >>> if (ret <= 0) >>> return ret ? ret : -EINVAL; >> I have two questions: >> >> 1) Why return -EINVAL for roothub urbs? > - For all reasons stated above > - Because it used to, and changing it doesn't fix anything > - Because urbs sent to xhci_urb_enqueue() should have a valid urb->dev->parent, > if they don't have it then they are INVALID > >> 2) Should I change all the return statements about xhci_check_args() in drivers/usb/host/xhci.c? >> >> There are 6 of them. > Only make sure your patch doesn't change the functionality unnecessarily. > There are two places where we return -EINVAL if xhci_check_args() returns 0: > xhci_urb_enqueue() and xhci_check_streams_endpoint() > Keep that functionality. > > Thanks > Mathias I'll fix these in v2. Thanks Hongyu Xie
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index dc357cabb265..a7a55dd206fe 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1604,9 +1604,12 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag struct urb_priv *urb_priv; int num_tds; - if (!urb || xhci_check_args(hcd, urb->dev, urb->ep, - true, true, __func__) <= 0) + if (!urb) return -EINVAL; + ret = xhci_check_args(hcd, urb->dev, urb->ep, + true, true, __func__); + if (ret <= 0) + return ret; slot_id = urb->dev->slot_id; ep_index = xhci_get_endpoint_index(&urb->ep->desc); @@ -3323,7 +3326,7 @@ static int xhci_check_streams_endpoint(struct xhci_hcd *xhci, return -EINVAL; ret = xhci_check_args(xhci_to_hcd(xhci), udev, ep, 1, true, __func__); if (ret <= 0) - return -EINVAL; + return ret; if (usb_ss_max_streams(&ep->ss_ep_comp) == 0) { xhci_warn(xhci, "WARN: SuperSpeed Endpoint Companion" " descriptor for ep 0x%x does not support streams\n",