Skip to contentSkip to conversation

net: Prepare gro for packet consuming gro callbacks

bug report Dan Carpenter 2 participants 6 messages Feb 16, 2017 – Feb 17, 2017 Kernel janitors

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

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

#1 lore raw

Hi Dan.

Quoted 16 lines · Dan Carpenter

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) {

Quoted 2 lines

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.

author
#2 lore raw
In reply to Dan Carpenter
Quoted 19 lines · Steffen Klassert

Hi Dan.

Quoted 16 lines · Dan Carpenter

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) {

Quoted 5 lines
Quoted 2 lines

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

#3 lore raw
In reply to Dan Carpenter
Quoted 23 lines · Dan Carpenter
Quoted 22 lines · Steffen Klassert

Hi Dan.

Quoted 18 lines · Dan Carpenter

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.

Quoted 8 lines
Quoted 2 lines

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.

author
#4 lore raw
In reply to Dan Carpenter
Quoted 29 lines · Steffen Klassert
Quoted 28 lines · Dan Carpenter
Quoted 19 lines · Steffen Klassert

Hi Dan.

Quoted 16 lines · Dan Carpenter

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) {

Quoted 5 lines
Quoted 2 lines

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:

Quoted 9 lines
Quoted 3 lines
	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

author
#5 lore raw
In reply to Dan Carpenter

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