| Message ID | YJ6IMH7jI9QFdGIX@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 31db0dbd72444abe645d90c20ecb84d668f5af5e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: hso: check for allocation failure in hso_create_bulk_serial_device() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: gregkh@linuxfoundation.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Unbalanced braces around else statement CHECK: braces {} should be used on all arms of this statement |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
On Fri, May 14, 2021 at 05:24:48PM +0300, Dan Carpenter wrote: > In current kernels, small allocations never actually fail so this > patch shouldn't affect runtime. > > Originally this error handling code written with the idea that if > the "serial->tiocmget" allocation failed, then we would continue > operating instead of bailing out early. But in later years we added > an unchecked dereference on the next line. > > serial->tiocmget->serial_state_notification = kzalloc(); > ^^^^^^^^^^^^^^^^^^ > > Since these allocations are never going fail in real life, this is > mostly a philosophical debate, but I think bailing out early is the > correct behavior that the user would want. And generally it's safer to > bail as soon an error happens. > > Fixes: af0de1303c4e ("usb: hso: obey DMA rules in tiocmget") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: Do more extensive clean up. As Johan pointed out the comments and > later NULL checks can be removed. > > drivers/net/usb/hso.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c > index 3ef4b2841402..260f850d69eb 100644 > --- a/drivers/net/usb/hso.c > +++ b/drivers/net/usb/hso.c > @@ -2618,29 +2618,28 @@ static struct hso_device *hso_create_bulk_serial_device( > num_urbs = 2; > serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), > GFP_KERNEL); > + if (!serial->tiocmget) > + goto exit; > serial->tiocmget->serial_state_notification > = kzalloc(sizeof(struct hso_serial_state_notification), > GFP_KERNEL); > - /* it isn't going to break our heart if serial->tiocmget > - * allocation fails don't bother checking this. > - */ > - if (serial->tiocmget && serial->tiocmget->serial_state_notification) { > - tiocmget = serial->tiocmget; > - tiocmget->endp = hso_get_ep(interface, > - USB_ENDPOINT_XFER_INT, > - USB_DIR_IN); > - if (!tiocmget->endp) { > - dev_err(&interface->dev, "Failed to find INT IN ep\n"); > - goto exit; > - } > - > - tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); > - if (tiocmget->urb) { > - mutex_init(&tiocmget->mutex); > - init_waitqueue_head(&tiocmget->waitq); > - } else > - hso_free_tiomget(serial); > + if (!serial->tiocmget->serial_state_notification) > + goto exit; > + tiocmget = serial->tiocmget; > + tiocmget->endp = hso_get_ep(interface, > + USB_ENDPOINT_XFER_INT, > + USB_DIR_IN); > + if (!tiocmget->endp) { > + dev_err(&interface->dev, "Failed to find INT IN ep\n"); > + goto exit; > } > + > + tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); > + if (tiocmget->urb) { > + mutex_init(&tiocmget->mutex); > + init_waitqueue_head(&tiocmget->waitq); > + } else > + hso_free_tiomget(serial); This should probably be changed to bail out on allocation errors as well now but that can be done as a follow-up. Either way: Reviewed-by: Johan Hovold <johan@kernel.org> > } > else > num_urbs = 1; Johan
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 14 May 2021 17:24:48 +0300 you wrote: > In current kernels, small allocations never actually fail so this > patch shouldn't affect runtime. > > Originally this error handling code written with the idea that if > the "serial->tiocmget" allocation failed, then we would continue > operating instead of bailing out early. But in later years we added > an unchecked dereference on the next line. > > [...] Here is the summary with links: - [v2,net] net: hso: check for allocation failure in hso_create_bulk_serial_device() https://git.kernel.org/netdev/net/c/31db0dbd7244 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c index 3ef4b2841402..260f850d69eb 100644 --- a/drivers/net/usb/hso.c +++ b/drivers/net/usb/hso.c @@ -2618,29 +2618,28 @@ static struct hso_device *hso_create_bulk_serial_device( num_urbs = 2; serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), GFP_KERNEL); + if (!serial->tiocmget) + goto exit; serial->tiocmget->serial_state_notification = kzalloc(sizeof(struct hso_serial_state_notification), GFP_KERNEL); - /* it isn't going to break our heart if serial->tiocmget - * allocation fails don't bother checking this. - */ - if (serial->tiocmget && serial->tiocmget->serial_state_notification) { - tiocmget = serial->tiocmget; - tiocmget->endp = hso_get_ep(interface, - USB_ENDPOINT_XFER_INT, - USB_DIR_IN); - if (!tiocmget->endp) { - dev_err(&interface->dev, "Failed to find INT IN ep\n"); - goto exit; - } - - tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); - if (tiocmget->urb) { - mutex_init(&tiocmget->mutex); - init_waitqueue_head(&tiocmget->waitq); - } else - hso_free_tiomget(serial); + if (!serial->tiocmget->serial_state_notification) + goto exit; + tiocmget = serial->tiocmget; + tiocmget->endp = hso_get_ep(interface, + USB_ENDPOINT_XFER_INT, + USB_DIR_IN); + if (!tiocmget->endp) { + dev_err(&interface->dev, "Failed to find INT IN ep\n"); + goto exit; } + + tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); + if (tiocmget->urb) { + mutex_init(&tiocmget->mutex); + init_waitqueue_head(&tiocmget->waitq); + } else + hso_free_tiomget(serial); } else num_urbs = 1;
In current kernels, small allocations never actually fail so this patch shouldn't affect runtime. Originally this error handling code written with the idea that if the "serial->tiocmget" allocation failed, then we would continue operating instead of bailing out early. But in later years we added an unchecked dereference on the next line. serial->tiocmget->serial_state_notification = kzalloc(); ^^^^^^^^^^^^^^^^^^ Since these allocations are never going fail in real life, this is mostly a philosophical debate, but I think bailing out early is the correct behavior that the user would want. And generally it's safer to bail as soon an error happens. Fixes: af0de1303c4e ("usb: hso: obey DMA rules in tiocmget") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: Do more extensive clean up. As Johan pointed out the comments and later NULL checks can be removed. drivers/net/usb/hso.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)