Re: [PATCH v2 net] net: hso: check for allocation failure in hso_create_bulk_serial_device()

[Johan Hovold <johan@kernel.org>]

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