* [PATCH] usb: gadget: net2272: assert for a valid dma request
@ 2020-08-30 18:36 trix
2020-08-30 21:22 ` Alan Stern
2020-08-31 5:31 ` Greg KH
0 siblings, 2 replies; 4+ messages in thread
From: trix @ 2020-08-30 18:36 UTC (permalink / raw)
To: balbi, gregkh, natechancellor, ndesaulniers, christophe.jaillet,
peter.chen, b-liu, chunfeng.yun, novikov, yanaijie
Cc: linux-usb, linux-kernel, Tom Rix
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>
---
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);
/* Ensure DREQ is de-asserted */
net2272_write(ep->dev, DMAREQ,
--
2.18.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: gadget: net2272: assert for a valid dma request
2020-08-30 18:36 [PATCH] usb: gadget: net2272: assert for a valid dma request trix
@ 2020-08-30 21:22 ` Alan Stern
2020-08-30 21:35 ` Tom Rix
2020-08-31 5:31 ` Greg KH
1 sibling, 1 reply; 4+ messages in thread
From: Alan Stern @ 2020-08-30 21:22 UTC (permalink / raw)
To: trix
Cc: balbi, gregkh, natechancellor, ndesaulniers, christophe.jaillet,
peter.chen, b-liu, chunfeng.yun, novikov, yanaijie, linux-usb,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: gadget: net2272: assert for a valid dma request
2020-08-30 21:22 ` Alan Stern
@ 2020-08-30 21:35 ` Tom Rix
0 siblings, 0 replies; 4+ messages in thread
From: Tom Rix @ 2020-08-30 21:35 UTC (permalink / raw)
To: Alan Stern
Cc: balbi, gregkh, natechancellor, ndesaulniers, christophe.jaillet,
peter.chen, b-liu, chunfeng.yun, novikov, yanaijie, linux-usb,
linux-kernel
On 8/30/20 2:22 PM, Alan Stern wrote:
> 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.
Since nothing is really going to change, drop this patch.
Tom
> Alan Stern
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: gadget: net2272: assert for a valid dma request
2020-08-30 18:36 [PATCH] usb: gadget: net2272: assert for a valid dma request trix
2020-08-30 21:22 ` Alan Stern
@ 2020-08-31 5:31 ` Greg KH
1 sibling, 0 replies; 4+ messages in thread
From: Greg KH @ 2020-08-31 5:31 UTC (permalink / raw)
To: trix
Cc: balbi, natechancellor, ndesaulniers, christophe.jaillet,
peter.chen, b-liu, chunfeng.yun, novikov, yanaijie, linux-usb,
linux-kernel
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>
> ---
> 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);
No, fix this properly, don't paper over the issue by crashing (as Alan
said, never add a BUG_ON() for something that you can recover from, and
really, never add a BUG_ON() call at all, that's just being a lazy
programmer.)
If this can cause a NULL dereference later on, than correctly handle the
error here please.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-31 5:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-30 18:36 [PATCH] usb: gadget: net2272: assert for a valid dma request trix
2020-08-30 21:22 ` Alan Stern
2020-08-30 21:35 ` Tom Rix
2020-08-31 5:31 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.