Message ID | 20240410-smatch-v1-1-785d009a852b@chromium.org |
---|---|
State | New |
Headers | show |
Series | [1/6] media: usb: siano: Fix allocation of urbs | expand |
On 10/04/2024 23:54, Ricardo Ribalda wrote: > USB urbs must be allocated with usb_alloc_urb. Quoting the manual > > Only use this function (usb_init_urb) if you _really_ understand what you > are doing. > > Fix the following smatch error: > > drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c > index 723510520d092..d85308e0785db 100644 > --- a/drivers/media/usb/siano/smsusb.c > +++ b/drivers/media/usb/siano/smsusb.c > @@ -40,7 +40,7 @@ struct smsusb_urb_t { > struct smscore_buffer_t *cb; > struct smsusb_device_t *dev; > > - struct urb urb; > + struct urb *urb; > > /* For the bottom half */ > struct work_struct wq; > @@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, > } > > usb_fill_bulk_urb( > - &surb->urb, > + surb->urb, > dev->udev, > usb_rcvbulkpipe(dev->udev, dev->in_ep), > surb->cb->p, > @@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, > smsusb_onresponse, > surb > ); > - surb->urb.transfer_flags |= URB_FREE_BUFFER; > + surb->urb->transfer_flags |= URB_FREE_BUFFER; > > - return usb_submit_urb(&surb->urb, GFP_ATOMIC); > + return usb_submit_urb(surb->urb, GFP_ATOMIC); > } > > static void smsusb_stop_streaming(struct smsusb_device_t *dev) > @@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev) > int i; > > for (i = 0; i < MAX_URBS; i++) { > - usb_kill_urb(&dev->surbs[i].urb); > + usb_kill_urb(dev->surbs[i].urb); > if (dev->surbs[i].wq.func) > cancel_work_sync(&dev->surbs[i].wq); > > @@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf) > struct smsusb_device_t *dev = usb_get_intfdata(intf); > > if (dev) { > + int i; > + > dev->state = SMSUSB_DISCONNECTED; > > smsusb_stop_streaming(dev); > @@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf) > if (dev->coredev) > smscore_unregister_device(dev->coredev); > > + for (i = 0; i < MAX_URBS; i++) > + usb_free_urb(dev->surbs[i].urb); > + > pr_debug("device 0x%p destroyed\n", dev); > kfree(dev); > } > @@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > void *mdev; > int i, rc; > int align = 0; > + int n_urb = 0; > > /* create device object */ > dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); > @@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > dev->coredev->is_usb_device = true; > > /* initialize urbs */ > - for (i = 0; i < MAX_URBS; i++) { > - dev->surbs[i].dev = dev; > - usb_init_urb(&dev->surbs[i].urb); > + for (n_urb = 0; n_urb < MAX_URBS; n_urb++) { > + dev->surbs[n_urb].dev = dev; > + dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL); > + if (!dev->surbs[n_urb].urb) > + goto free_urbs; > } After allocating the URBs there are a few more error paths that do 'goto err_unregister_device;' instead of 'goto free_urbs;'. From what I can see, those need to go through 'free_urbs' as well. > > pr_debug("smsusb_start_streaming(...).\n"); > @@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > > return rc; > > +free_urbs: > + for (i = 0; i < n_urb; i++) > + usb_free_urb(dev->surbs[n_urb].urb); Would it be better to also assign NULL to dev->surbs[n_urb].urb? That way there are no invalid pointers that can mess up things. Regards, Hans > + > err_unregister_device: > smsusb_term_device(intf); > #ifdef CONFIG_MEDIA_CONTROLLER_DVB >
On 15/04/2024 12:08, Hans Verkuil wrote: > On 10/04/2024 23:54, Ricardo Ribalda wrote: >> USB urbs must be allocated with usb_alloc_urb. Quoting the manual >> >> Only use this function (usb_init_urb) if you _really_ understand what you >> are doing. >> >> Fix the following smatch error: >> >> drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures >> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >> --- >> drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++-------- >> 1 file changed, 20 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c >> index 723510520d092..d85308e0785db 100644 >> --- a/drivers/media/usb/siano/smsusb.c >> +++ b/drivers/media/usb/siano/smsusb.c >> @@ -40,7 +40,7 @@ struct smsusb_urb_t { >> struct smscore_buffer_t *cb; >> struct smsusb_device_t *dev; >> >> - struct urb urb; >> + struct urb *urb; >> >> /* For the bottom half */ >> struct work_struct wq; >> @@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, >> } >> >> usb_fill_bulk_urb( >> - &surb->urb, >> + surb->urb, >> dev->udev, >> usb_rcvbulkpipe(dev->udev, dev->in_ep), >> surb->cb->p, >> @@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, >> smsusb_onresponse, >> surb >> ); >> - surb->urb.transfer_flags |= URB_FREE_BUFFER; >> + surb->urb->transfer_flags |= URB_FREE_BUFFER; >> >> - return usb_submit_urb(&surb->urb, GFP_ATOMIC); >> + return usb_submit_urb(surb->urb, GFP_ATOMIC); >> } >> >> static void smsusb_stop_streaming(struct smsusb_device_t *dev) >> @@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev) >> int i; >> >> for (i = 0; i < MAX_URBS; i++) { >> - usb_kill_urb(&dev->surbs[i].urb); >> + usb_kill_urb(dev->surbs[i].urb); >> if (dev->surbs[i].wq.func) >> cancel_work_sync(&dev->surbs[i].wq); >> >> @@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf) >> struct smsusb_device_t *dev = usb_get_intfdata(intf); >> >> if (dev) { >> + int i; >> + >> dev->state = SMSUSB_DISCONNECTED; >> >> smsusb_stop_streaming(dev); >> @@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf) >> if (dev->coredev) >> smscore_unregister_device(dev->coredev); >> >> + for (i = 0; i < MAX_URBS; i++) >> + usb_free_urb(dev->surbs[i].urb); >> + >> pr_debug("device 0x%p destroyed\n", dev); >> kfree(dev); >> } >> @@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) >> void *mdev; >> int i, rc; >> int align = 0; >> + int n_urb = 0; >> >> /* create device object */ >> dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); >> @@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) >> dev->coredev->is_usb_device = true; >> >> /* initialize urbs */ >> - for (i = 0; i < MAX_URBS; i++) { >> - dev->surbs[i].dev = dev; >> - usb_init_urb(&dev->surbs[i].urb); >> + for (n_urb = 0; n_urb < MAX_URBS; n_urb++) { >> + dev->surbs[n_urb].dev = dev; >> + dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL); >> + if (!dev->surbs[n_urb].urb) >> + goto free_urbs; >> } > > After allocating the URBs there are a few more error paths that do > 'goto err_unregister_device;' instead of 'goto free_urbs;'. From what > I can see, those need to go through 'free_urbs' as well. > >> >> pr_debug("smsusb_start_streaming(...).\n"); >> @@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) >> >> return rc; >> >> +free_urbs: >> + for (i = 0; i < n_urb; i++) >> + usb_free_urb(dev->surbs[n_urb].urb); > > Would it be better to also assign NULL to dev->surbs[n_urb].urb? > That way there are no invalid pointers that can mess up things. Ricardo, just post a v2 of this patch. I posted a PR for the other patches. Nice work, BTW! Regards, Hans > > Regards, > > Hans > >> + >> err_unregister_device: >> smsusb_term_device(intf); >> #ifdef CONFIG_MEDIA_CONTROLLER_DVB >> > >
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c index 723510520d092..d85308e0785db 100644 --- a/drivers/media/usb/siano/smsusb.c +++ b/drivers/media/usb/siano/smsusb.c @@ -40,7 +40,7 @@ struct smsusb_urb_t { struct smscore_buffer_t *cb; struct smsusb_device_t *dev; - struct urb urb; + struct urb *urb; /* For the bottom half */ struct work_struct wq; @@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, } usb_fill_bulk_urb( - &surb->urb, + surb->urb, dev->udev, usb_rcvbulkpipe(dev->udev, dev->in_ep), surb->cb->p, @@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, smsusb_onresponse, surb ); - surb->urb.transfer_flags |= URB_FREE_BUFFER; + surb->urb->transfer_flags |= URB_FREE_BUFFER; - return usb_submit_urb(&surb->urb, GFP_ATOMIC); + return usb_submit_urb(surb->urb, GFP_ATOMIC); } static void smsusb_stop_streaming(struct smsusb_device_t *dev) @@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev) int i; for (i = 0; i < MAX_URBS; i++) { - usb_kill_urb(&dev->surbs[i].urb); + usb_kill_urb(dev->surbs[i].urb); if (dev->surbs[i].wq.func) cancel_work_sync(&dev->surbs[i].wq); @@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf) struct smsusb_device_t *dev = usb_get_intfdata(intf); if (dev) { + int i; + dev->state = SMSUSB_DISCONNECTED; smsusb_stop_streaming(dev); @@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf) if (dev->coredev) smscore_unregister_device(dev->coredev); + for (i = 0; i < MAX_URBS; i++) + usb_free_urb(dev->surbs[i].urb); + pr_debug("device 0x%p destroyed\n", dev); kfree(dev); } @@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) void *mdev; int i, rc; int align = 0; + int n_urb = 0; /* create device object */ dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); @@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) dev->coredev->is_usb_device = true; /* initialize urbs */ - for (i = 0; i < MAX_URBS; i++) { - dev->surbs[i].dev = dev; - usb_init_urb(&dev->surbs[i].urb); + for (n_urb = 0; n_urb < MAX_URBS; n_urb++) { + dev->surbs[n_urb].dev = dev; + dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL); + if (!dev->surbs[n_urb].urb) + goto free_urbs; } pr_debug("smsusb_start_streaming(...).\n"); @@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) return rc; +free_urbs: + for (i = 0; i < n_urb; i++) + usb_free_urb(dev->surbs[n_urb].urb); + err_unregister_device: smsusb_term_device(intf); #ifdef CONFIG_MEDIA_CONTROLLER_DVB
USB urbs must be allocated with usb_alloc_urb. Quoting the manual Only use this function (usb_init_urb) if you _really_ understand what you are doing. Fix the following smatch error: drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)