bgmac: fix a missing check for build_skb

PATCH In review Weidong Wang 6 participants 11 messages Jan 13, 2016 – Jan 14, 2016

Subsystems (3)

Networking Network drivers Broadcom iproc gbit Ethernet driver

lore

Share & tools

lore

Weidong Wang

author

2016-01-13 03:06 UTC #0 lore raw

when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.

Signed-off-by Weidong Wang <[email protected]>

---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/net/ethernet/broadcom/bgmac.c +5 -0

index 21e3c38..d75180a 100644
@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			len -= ETH_FCS_LEN;
 
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(skb)) {
+				bgmac_err(bgmac, "build_skb failed\n");
+				put_page(virt_to_head_page(buf));
+				break;
+			}
 			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
 				BGMAC_RX_BUF_OFFSET + len);
 			skb_pull(skb, BGMAC_RX_FRAME_OFFSET +

Signature · 1 line

1.9.0

David Miller

maintainer Applied

2016-01-13 05:24 UTC #1 lore raw

From: Weidong Wang <[email protected]>
Date: Wed, 13 Jan 2016 11:06:41 +0800

Quoted 4 lines

when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>

Applied, thanks.

Paolo Abeni

maintainer

2016-01-13 08:34 UTC #2 lore raw

In reply to Weidong Wang

Quoted 13 lines · Weidong Wang

when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/broadcom/bgmac.c +0 -0
index 21e3c38..d75180a 100644

drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 469

Quoted 4 lines

drivers/net/ethernet/broadcom/bgmac.c +1 -0

@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			len -= ETH_FCS_LEN;
 
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(skb)) {

Should that be instead:

if (unlikely(!skb)) {

Quoted 7 lines

drivers/net/ethernet/broadcom/bgmac.c +4 -0

+				bgmac_err(bgmac, "build_skb failed\n");
+				put_page(virt_to_head_page(buf));
+				break;
+			}
 			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
 				BGMAC_RX_BUF_OFFSET + len);
 			skb_pull(skb, BGMAC_RX_FRAME_OFFSET +

Weidong Wang

author

2016-01-13 10:25 UTC #3 lore raw

Quoted 16 lines · Paolo Abeni

Quoted 15 lines · Weidong Wang
when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/broadcom/bgmac.c +0 -0
index 21e3c38..d75180a 100644
@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			len -= ETH_FCS_LEN;
 

Quoted 9 lines

drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 469
Quoted 2 lines
drivers/net/ethernet/broadcom/bgmac.c +1 -0
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(skb)) {
Should that be instead:
if (unlikely(!skb)) {
?

What to instead of it?

Regards,
Weidong

Quoted 11 lines

drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 476
Quoted 7 lines
drivers/net/ethernet/broadcom/bgmac.c +4 -0
+				bgmac_err(bgmac, "build_skb failed\n");
+				put_page(virt_to_head_page(buf));
+				break;
+			}
 			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
 				BGMAC_RX_BUF_OFFSET + len);
 			skb_pull(skb, BGMAC_RX_FRAME_OFFSET +
.

Felix Fietkau

2016-01-13 10:57 UTC #4 lore raw

Quoted 22 lines · Weidong Wang

Quoted 21 lines · Paolo Abeni
Quoted 13 lines · Weidong Wang
when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/broadcom/bgmac.c +0 -0
index 21e3c38..d75180a 100644
drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 469
Quoted 4 lines
drivers/net/ethernet/broadcom/bgmac.c +1 -0
@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			len -= ETH_FCS_LEN;
 
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(skb)) {
Should that be instead:

Quoted 6 lines

Quoted 4 lines
if (unlikely(!skb)) {
?
What to instead of it?

Your patch has a logic error (missing the !), and it's breaking the ethernet driver completely instead of fixing anything. Did you even test this?

Dave, why did you apply this patch so fast without any chance of review?

- Felix

Weidong Wang

author

2016-01-13 11:31 UTC #5 lore raw

Quoted 25 lines · Felix Fietkau

Quoted 24 lines · Weidong Wang
Quoted 23 lines · Paolo Abeni
Quoted 13 lines · Weidong Wang
when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/broadcom/bgmac.c +0 -0
index 21e3c38..d75180a 100644
drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 469
Quoted 4 lines
drivers/net/ethernet/broadcom/bgmac.c +1 -0
@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			len -= ETH_FCS_LEN;
 
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(skb)) {
Should that be instead:
if (unlikely(!skb)) {

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
?
What to instead of it?
Your patch has a logic error (missing the !), and it's breaking the ethernet driver completely instead of fixing anything. Did you even test this?

Yep, you are right.
Sorry for that.
I just review the source code without any test.
I should resent it again.

Quoted 6 lines

Dave, why did you apply this patch so fast without any chance of review?
- Felix
.

Regards,
Weidong

David Miller

maintainer

2016-01-13 15:34 UTC #6 lore raw

In reply to Felix Fietkau

From: Felix Fietkau <[email protected]>
Date: Wed, 13 Jan 2016 11:57:51 +0100

Quoted 2 lines

Dave, why did you apply this patch so fast without any chance of review?

Because I wanted to clear my backlog %100 for the merge window initial pull request.

The patch looked simple enough, I did read it carefully, and I did unfortunately miss the logic error.

Weidong Wang

author

2016-01-13 11:53 UTC #7 lore raw

PATCH v2 1/1 bgmac: fix a missing check for build_skb

In reply to Weidong Wang

when build_skb failed, it may occur a NULL pointer. So add a 'NULL check' for it.

Signed-off-by Weidong Wang <[email protected]>

---
change log:
v2:
fix the error logic change which pointed by Paolo Abni and Felix.

---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/net/ethernet/broadcom/bgmac.c +5 -0

index c7798d3..3974152 100644
@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			len -= ETH_FCS_LEN;
 
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(!skb)) {
+				bgmac_err(bgmac, "build_skb failed\n");
+				put_page(virt_to_head_page(buf));
+				break;
+			}
 			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
 				BGMAC_RX_BUF_OFFSET + len);
 			skb_pull(skb, BGMAC_RX_FRAME_OFFSET +

Signature · 1 line

1.9.0

Eric Dumazet

2016-01-13 14:52 UTC #8 lore raw

Quoted 9 lines · Weidong Wang

when build_skb failed, it may occur a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
change log:
v2:
fix the error logic change which pointed by Paolo Abni and Felix.

This is too late, you have to send a relative patch, since the prior one was merged.

Weidong Wang

author

2016-01-14 02:10 UTC #9 lore raw

Quoted 11 lines · Eric Dumazet

Quoted 9 lines · Weidong Wang
when build_skb failed, it may occur a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
change log:
v2:
fix the error logic change which pointed by Paolo Abni and Felix.

Quoted 4 lines

This is too late, you have to send a relative patch, since the prior one was merged.

OK.

Regards,
Weidong

Quoted 2 lines

Sergei Shtylyov

2016-01-13 12:10 UTC #10 lore raw

In reply to Weidong Wang

Hello.

Quoted 13 lines · Weidong Wang

when build_skb failed, it may occure a NULL pointer. So add a 'NULL check' for it.
Signed-off-by Weidong Wang <[email protected]>
---
  drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
  1 file changed, 5 insertions(+)
drivers/net/ethernet/broadcom/bgmac.c +0 -0
index 21e3c38..d75180a 100644

drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 469

Quoted 4 lines

drivers/net/ethernet/broadcom/bgmac.c +1 -0

@@ -466,6 +466,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
  			len -= ETH_FCS_LEN;
 
  			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			if (unlikely(skb)) {

!skb , you mean?

drivers/net/ethernet/broadcom/bgmac.c static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, · line 476

Quoted 7 lines

drivers/net/ethernet/broadcom/bgmac.c +4 -0

+				bgmac_err(bgmac, "build_skb failed\n");
+				put_page(virt_to_head_page(buf));
+				break;
+			}
  			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
  				BGMAC_RX_BUF_OFFSET + len);
  			skb_pull(skb, BGMAC_RX_FRAME_OFFSET +

MBR, Sergei