diff mbox series

[1/2] usb: hso: fix error handling code of hso_create_net_device

Message ID 20210714081127.675743-1-mudongliangabcd@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [1/2] usb: hso: fix error handling code of hso_create_net_device | expand

Commit Message

Dongliang Mu July 14, 2021, 8:11 a.m. UTC 
The current error handling code of hso_create_net_device is
hso_free_net_device, no matter which errors lead to. For example,
WARNING in hso_free_net_device [1].

Fix this by refactoring the error handling code of
hso_create_net_device by handling different errors by different code.

[1] https://syzkaller.appspot.com/bug?id=66eff8d49af1b28370ad342787413e35bbe76efe

Reported-by: syzbot+44d53c7255bb1aea22d2@syzkaller.appspotmail.com
Fixes: 5fcfb6d0bfcd ("hso: fix bailout in error case of probe")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

Comments

Dongliang Mu July 14, 2021, 8:14 a.m. UTC | #1 
On Wed, Jul 14, 2021 at 4:11 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> The current error handling code of hso_create_net_device is
> hso_free_net_device, no matter which errors lead to. For example,
> WARNING in hso_free_net_device [1].
>
> Fix this by refactoring the error handling code of
> hso_create_net_device by handling different errors by different code.
>

Hi Dan,

Please take a look at this version. I forget about the changelog about
this patch. I will send a version v3 with your further comment if you
have.

> [1] https://syzkaller.appspot.com/bug?id=66eff8d49af1b28370ad342787413e35bbe76efe
>
> Reported-by: syzbot+44d53c7255bb1aea22d2@syzkaller.appspotmail.com
> Fixes: 5fcfb6d0bfcd ("hso: fix bailout in error case of probe")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 54ef8492ca01..39c4e88eab62 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -2495,7 +2495,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
>                            hso_net_init);
>         if (!net) {
>                 dev_err(&interface->dev, "Unable to create ethernet device\n");
> -               goto exit;
> +               goto err_hso_dev;
>         }
>
>         hso_net = netdev_priv(net);
> @@ -2508,13 +2508,13 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
>                                       USB_DIR_IN);
>         if (!hso_net->in_endp) {
>                 dev_err(&interface->dev, "Can't find BULK IN endpoint\n");
> -               goto exit;
> +               goto err_net;
>         }
>         hso_net->out_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
>                                        USB_DIR_OUT);
>         if (!hso_net->out_endp) {
>                 dev_err(&interface->dev, "Can't find BULK OUT endpoint\n");
> -               goto exit;
> +               goto err_net;
>         }
>         SET_NETDEV_DEV(net, &interface->dev);
>         SET_NETDEV_DEVTYPE(net, &hso_type);
> @@ -2523,18 +2523,18 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
>         for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
>                 hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
>                 if (!hso_net->mux_bulk_rx_urb_pool[i])
> -                       goto exit;
> +                       goto err_mux_bulk_rx;
>                 hso_net->mux_bulk_rx_buf_pool[i] = kzalloc(MUX_BULK_RX_BUF_SIZE,
>                                                            GFP_KERNEL);
>                 if (!hso_net->mux_bulk_rx_buf_pool[i])
> -                       goto exit;
> +                       goto err_mux_bulk_rx;
>         }
>         hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
>         if (!hso_net->mux_bulk_tx_urb)
> -               goto exit;
> +               goto err_mux_bulk_rx;
>         hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
>         if (!hso_net->mux_bulk_tx_buf)
> -               goto exit;
> +               goto err_mux_bulk_tx;
>
>         add_net_device(hso_dev);
>
> @@ -2542,7 +2542,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
>         result = register_netdev(net);
>         if (result) {
>                 dev_err(&interface->dev, "Failed to register device\n");
> -               goto exit;
> +               goto err_register;
>         }
>
>         hso_log_port(hso_dev);
> @@ -2550,8 +2550,21 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
>         hso_create_rfkill(hso_dev, interface);
>
>         return hso_dev;
> -exit:
> -       hso_free_net_device(hso_dev, true);
> +
> +err_register:
> +       remove_net_device(hso_dev);
> +       kfree(hso_net->mux_bulk_tx_buf);
> +err_mux_bulk_tx:
> +       usb_free_urb(hso_net->mux_bulk_tx_urb);
> +err_mux_bulk_rx:
> +       for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
> +               usb_free_urb(hso_net->mux_bulk_rx_urb_pool[i]);
> +               kfree(hso_net->mux_bulk_rx_buf_pool[i]);
> +       }
> +err_net:
> +       free_netdev(net);
> +err_hso_dev:
> +       kfree(hso_dev);
>         return NULL;
>  }
>
> --
> 2.25.1
>
Dan Carpenter July 14, 2021, 8:30 a.m. UTC | #2 
On Wed, Jul 14, 2021 at 04:14:18PM +0800, Dongliang Mu wrote:
> > @@ -2523,18 +2523,18 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
> >         for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
> >                 hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
> >                 if (!hso_net->mux_bulk_rx_urb_pool[i])
> > -                       goto exit;
> > +                       goto err_mux_bulk_rx;
> >                 hso_net->mux_bulk_rx_buf_pool[i] = kzalloc(MUX_BULK_RX_BUF_SIZE,
> >                                                            GFP_KERNEL);
> >                 if (!hso_net->mux_bulk_rx_buf_pool[i])
> > -                       goto exit;
> > +                       goto err_mux_bulk_rx;
> >         }
> >         hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
> >         if (!hso_net->mux_bulk_tx_urb)
> > -               goto exit;
> > +               goto err_mux_bulk_rx;
> >         hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
> >         if (!hso_net->mux_bulk_tx_buf)
> > -               goto exit;
> > +               goto err_mux_bulk_tx;

I would probably have called this err free_tx_urb or something like
that.

> >
> >         add_net_device(hso_dev);
> >
> > @@ -2542,7 +2542,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
> >         result = register_netdev(net);
> >         if (result) {
> >                 dev_err(&interface->dev, "Failed to register device\n");
> > -               goto exit;
> > +               goto err_register;

This is still Come From style.  I wouldn't have commented except that
you said you are giong to redo the patch for other reasons.

It looks good.  Straight forward to review now.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 54ef8492ca01..39c4e88eab62 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2495,7 +2495,7 @@  static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 			   hso_net_init);
 	if (!net) {
 		dev_err(&interface->dev, "Unable to create ethernet device\n");
-		goto exit;
+		goto err_hso_dev;
 	}
 
 	hso_net = netdev_priv(net);
@@ -2508,13 +2508,13 @@  static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 				      USB_DIR_IN);
 	if (!hso_net->in_endp) {
 		dev_err(&interface->dev, "Can't find BULK IN endpoint\n");
-		goto exit;
+		goto err_net;
 	}
 	hso_net->out_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
 				       USB_DIR_OUT);
 	if (!hso_net->out_endp) {
 		dev_err(&interface->dev, "Can't find BULK OUT endpoint\n");
-		goto exit;
+		goto err_net;
 	}
 	SET_NETDEV_DEV(net, &interface->dev);
 	SET_NETDEV_DEVTYPE(net, &hso_type);
@@ -2523,18 +2523,18 @@  static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
 		hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!hso_net->mux_bulk_rx_urb_pool[i])
-			goto exit;
+			goto err_mux_bulk_rx;
 		hso_net->mux_bulk_rx_buf_pool[i] = kzalloc(MUX_BULK_RX_BUF_SIZE,
 							   GFP_KERNEL);
 		if (!hso_net->mux_bulk_rx_buf_pool[i])
-			goto exit;
+			goto err_mux_bulk_rx;
 	}
 	hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_urb)
-		goto exit;
+		goto err_mux_bulk_rx;
 	hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_buf)
-		goto exit;
+		goto err_mux_bulk_tx;
 
 	add_net_device(hso_dev);
 
@@ -2542,7 +2542,7 @@  static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	result = register_netdev(net);
 	if (result) {
 		dev_err(&interface->dev, "Failed to register device\n");
-		goto exit;
+		goto err_register;
 	}
 
 	hso_log_port(hso_dev);
@@ -2550,8 +2550,21 @@  static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	hso_create_rfkill(hso_dev, interface);
 
 	return hso_dev;
-exit:
-	hso_free_net_device(hso_dev, true);
+
+err_register:
+	remove_net_device(hso_dev);
+	kfree(hso_net->mux_bulk_tx_buf);
+err_mux_bulk_tx:
+	usb_free_urb(hso_net->mux_bulk_tx_urb);
+err_mux_bulk_rx:
+	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
+		usb_free_urb(hso_net->mux_bulk_rx_urb_pool[i]);
+		kfree(hso_net->mux_bulk_rx_buf_pool[i]);
+	}
+err_net:
+	free_netdev(net);
+err_hso_dev:
+	kfree(hso_dev);
 	return NULL;
 }