[bug report] net: Prepare gro for packet consuming gro callbacks

public inbox for kernel-janitors@vger.kernel.org
 help / color / mirror / Atom feed

* [bug report] net: Prepare gro for packet consuming gro callbacks
@ 2017-02-16 20:11 Dan Carpenter
  2017-02-17  8:18 ` Steffen Klassert
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-02-16 20:11 UTC (permalink / raw)
  To: kernel-janitors

Hello Steffen Klassert,

The patch 25393d3fc055: "net: Prepare gro for packet consuming gro
callbacks" from Feb 15, 2017, leads to the following Smatch
warning:

	net/core/dev.c:4522 dev_gro_receive()
	error: 'pp' dereferencing possible ERR_PTR()

net/core/dev.c
  4510          if (&ptype->list = head)
  4511                  goto normal;
  4512  
  4513          if (IS_ERR(pp) && PTR_ERR(pp) = -EINPROGRESS) {


Smatch sees this and assumes that it's possible for "pp" to be an error
pointer that's not -EINPROGRESS.


  4514                  ret = GRO_CONSUMED;
  4515                  goto ok;
  4516          }
  4517  
  4518          same_flow = NAPI_GRO_CB(skb)->same_flow;
  4519          ret = NAPI_GRO_CB(skb)->free ? GRO_MERGED_FREE : GRO_MERGED;
  4520  
  4521          if (pp) {
  4522                  struct sk_buff *nskb = *pp;
                                               ^^^
Leading to an oops.

  4523  
  4524                  *pp = nskb->next;
  4525                  nskb->next = NULL;
  4526                  napi_gro_complete(nskb);
  4527                  napi->gro_count--;
  4528          }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] net: Prepare gro for packet consuming gro callbacks
  2017-02-16 20:11 [bug report] net: Prepare gro for packet consuming gro callbacks Dan Carpenter
@ 2017-02-17  8:18 ` Steffen Klassert
  2017-02-17 10:13 ` Dan Carpenter
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2017-02-17  8:18 UTC (permalink / raw)
  To: kernel-janitors

Hi Dan.

On Thu, Feb 16, 2017 at 11:11:27PM +0300, Dan Carpenter wrote:
> Hello Steffen Klassert,
> 
> The patch 25393d3fc055: "net: Prepare gro for packet consuming gro
> callbacks" from Feb 15, 2017, leads to the following Smatch
> warning:
> 
> 	net/core/dev.c:4522 dev_gro_receive()
> 	error: 'pp' dereferencing possible ERR_PTR()
> 
> net/core/dev.c
>   4510          if (&ptype->list = head)
>   4511                  goto normal;
>   4512  
>   4513          if (IS_ERR(pp) && PTR_ERR(pp) = -EINPROGRESS) {
> 
> 
> Smatch sees this and assumes that it's possible for "pp" to be an error
> pointer that's not -EINPROGRESS.

pp can be either a vaild pointer, NULL or -EINPROGRESS,
nothing else. So I guess this is ok.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] net: Prepare gro for packet consuming gro callbacks
  2017-02-16 20:11 [bug report] net: Prepare gro for packet consuming gro callbacks Dan Carpenter
  2017-02-17  8:18 ` Steffen Klassert
@ 2017-02-17 10:13 ` Dan Carpenter
  2017-02-17 10:22 ` Steffen Klassert
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-02-17 10:13 UTC (permalink / raw)
  To: kernel-janitors

On Fri, Feb 17, 2017 at 09:18:25AM +0100, Steffen Klassert wrote:
> Hi Dan.
> 
> On Thu, Feb 16, 2017 at 11:11:27PM +0300, Dan Carpenter wrote:
> > Hello Steffen Klassert,
> > 
> > The patch 25393d3fc055: "net: Prepare gro for packet consuming gro
> > callbacks" from Feb 15, 2017, leads to the following Smatch
> > warning:
> > 
> > 	net/core/dev.c:4522 dev_gro_receive()
> > 	error: 'pp' dereferencing possible ERR_PTR()
> > 
> > net/core/dev.c
> >   4510          if (&ptype->list = head)
> >   4511                  goto normal;
> >   4512  
> >   4513          if (IS_ERR(pp) && PTR_ERR(pp) = -EINPROGRESS) {
> > 
> > 
> > Smatch sees this and assumes that it's possible for "pp" to be an error
> > pointer that's not -EINPROGRESS.
> 
> pp can be either a vaild pointer, NULL or -EINPROGRESS,
> nothing else. So I guess this is ok.

Could you change it to:

	if (PTR_ERR(pp) = -EINPROGRESS) {

That would silence the warning and make the code more clear I think.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] net: Prepare gro for packet consuming gro callbacks
  2017-02-16 20:11 [bug report] net: Prepare gro for packet consuming gro callbacks Dan Carpenter
  2017-02-17  8:18 ` Steffen Klassert
  2017-02-17 10:13 ` Dan Carpenter
@ 2017-02-17 10:22 ` Steffen Klassert
  2017-02-17 10:39 ` Dan Carpenter
  2017-02-17 13:39 ` Dan Carpenter
  4 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2017-02-17 10:22 UTC (permalink / raw)
  To: kernel-janitors

On Fri, Feb 17, 2017 at 01:13:05PM +0300, Dan Carpenter wrote:
> On Fri, Feb 17, 2017 at 09:18:25AM +0100, Steffen Klassert wrote:
> > Hi Dan.
> > 
> > On Thu, Feb 16, 2017 at 11:11:27PM +0300, Dan Carpenter wrote:
> > > Hello Steffen Klassert,
> > > 
> > > The patch 25393d3fc055: "net: Prepare gro for packet consuming gro
> > > callbacks" from Feb 15, 2017, leads to the following Smatch
> > > warning:
> > > 
> > > 	net/core/dev.c:4522 dev_gro_receive()
> > > 	error: 'pp' dereferencing possible ERR_PTR()
> > > 
> > > net/core/dev.c
> > >   4510          if (&ptype->list = head)
> > >   4511                  goto normal;
> > >   4512  
> > >   4513          if (IS_ERR(pp) && PTR_ERR(pp) = -EINPROGRESS) {
> > > 
> > > 
> > > Smatch sees this and assumes that it's possible for "pp" to be an error
> > > pointer that's not -EINPROGRESS.
> > 
> > pp can be either a vaild pointer, NULL or -EINPROGRESS,
> > nothing else. So I guess this is ok.
> 
> Could you change it to:
> 
> 	if (PTR_ERR(pp) = -EINPROGRESS) {
> 
> That would silence the warning and make the code more clear I think.

I had it like that, but some other code checking tool
complained that I check -EINPROGRESS without checking
if this is an error pointer. So I changed it to that
what I have now.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] net: Prepare gro for packet consuming gro callbacks
  2017-02-16 20:11 [bug report] net: Prepare gro for packet consuming gro callbacks Dan Carpenter
                   ` (2 preceding siblings ...)
  2017-02-17 10:22 ` Steffen Klassert
@ 2017-02-17 10:39 ` Dan Carpenter
  2017-02-17 13:39 ` Dan Carpenter
  4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-02-17 10:39 UTC (permalink / raw)
  To: kernel-janitors

On Fri, Feb 17, 2017 at 11:22:41AM +0100, Steffen Klassert wrote:
> On Fri, Feb 17, 2017 at 01:13:05PM +0300, Dan Carpenter wrote:
> > On Fri, Feb 17, 2017 at 09:18:25AM +0100, Steffen Klassert wrote:
> > > Hi Dan.
> > > 
> > > On Thu, Feb 16, 2017 at 11:11:27PM +0300, Dan Carpenter wrote:
> > > > Hello Steffen Klassert,
> > > > 
> > > > The patch 25393d3fc055: "net: Prepare gro for packet consuming gro
> > > > callbacks" from Feb 15, 2017, leads to the following Smatch
> > > > warning:
> > > > 
> > > > 	net/core/dev.c:4522 dev_gro_receive()
> > > > 	error: 'pp' dereferencing possible ERR_PTR()
> > > > 
> > > > net/core/dev.c
> > > >   4510          if (&ptype->list = head)
> > > >   4511                  goto normal;
> > > >   4512  
> > > >   4513          if (IS_ERR(pp) && PTR_ERR(pp) = -EINPROGRESS) {
> > > > 
> > > > 
> > > > Smatch sees this and assumes that it's possible for "pp" to be an error
> > > > pointer that's not -EINPROGRESS.
> > > 
> > > pp can be either a vaild pointer, NULL or -EINPROGRESS,
> > > nothing else. So I guess this is ok.
> > 
> > Could you change it to:
> > 
> > 	if (PTR_ERR(pp) = -EINPROGRESS) {
> > 
> > That would silence the warning and make the code more clear I think.
> 
> I had it like that, but some other code checking tool
> complained that I check -EINPROGRESS without checking
> if this is an error pointer. So I changed it to that
> what I have now.
> 

Why would it complain about that???

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] net: Prepare gro for packet consuming gro callbacks
  2017-02-16 20:11 [bug report] net: Prepare gro for packet consuming gro callbacks Dan Carpenter
                   ` (3 preceding siblings ...)
  2017-02-17 10:39 ` Dan Carpenter
@ 2017-02-17 13:39 ` Dan Carpenter
  4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-02-17 13:39 UTC (permalink / raw)
  To: kernel-janitors

The only static checkers which care about ERR_PTR are Smatch and
Coccinelle.  I don't think Coccinelle would care about that so it must
be a Smatch warning...

:/

The only thing I can think of is that this is a Smatch warning that
"pp is not an error pointer".  And that would be because the cross
function database wasn't fully populated...  I'm not positive on that.

Very strange.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-02-17 13:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-16 20:11 [bug report] net: Prepare gro for packet consuming gro callbacks Dan Carpenter
2017-02-17  8:18 ` Steffen Klassert
2017-02-17 10:13 ` Dan Carpenter
2017-02-17 10:22 ` Steffen Klassert
2017-02-17 10:39 ` Dan Carpenter
2017-02-17 13:39 ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox