lkml.org 
[lkml]   [2020]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] usb: gadget: net2272: assert for a valid dma request
On Sun, Aug 30, 2020 at 11:36:46AM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> clang static analysis flags this representive problem
>
> net2272.c:1541:8: warning: Dereference of null pointer
> if ((req->req.length % ep->ep.maxpacket != 0) ||
> ^~~~~~~~~~~~~~~
> This is mostly not a problem.
>
> In net2272_handle_dma(), even though every path through
> the routine dereferences req, it is set to NULL when the
> ep->queue is empty. If the eq->queue was ever empty this
> would have oops.
>
> So the else statement should not be needed. If it is,
> flag it as a BUG.
>
> Signed-off-by: Tom Rix <trix@redhat.com>

This patch really seems to be overkill. In particular, Linus Torvalds
feels very strongly that people should not add BUG or BUG_ON calls
except in extreme situations -- which this isn't.

> ---
> drivers/usb/gadget/udc/net2272.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
> index 44d1ea2307bb..8301723a8b1a 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -1506,17 +1506,16 @@ static int net2272_stop(struct usb_gadget *_gadget)
> static void
> net2272_handle_dma(struct net2272_ep *ep)
> {
> - struct net2272_request *req;
> + struct net2272_request *req = NULL;
> unsigned len;
> int status;
>
> if (!list_empty(&ep->queue))
> req = list_entry(ep->queue.next,
> struct net2272_request, queue);
> - else
> - req = NULL;
>
> dev_vdbg(ep->dev->dev, "handle_dma %s req %p\n", ep->ep.name, req);
> + BUG_ON(!req);

There's no point in adding this. If the function goes on to dereference
a NULL pointer, you'll get the same effect in any case -- an oops.

If you want to make the point that req had better not be NULL, just get
rid of the "if" test entirely. You could even change the list_entry to
list_first_entry.

Alan Stern

\
 
 \ /
  Last update: 2020-08-30 23:23    [W:0.074 / U:1.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site