Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1308001 > unrolled thread

bgmac: fix a missing check for build_skb

Started byWeidong Wang <wangweidong1@huawei.com>
First post2016-01-13 04:10 +0100
Last post2016-01-13 13:20 +0100
Articles 11 — 6 participants

Back to article view | Back to linux.kernel

Contents

  bgmac: fix a missing check for build_skb Weidong Wang <wangweidong1@huawei.com> - 2016-01-13 04:10 +0100
    Re: bgmac: fix a missing check for build_skb David Miller <davem@davemloft.net> - 2016-01-13 06:30 +0100
    Re: bgmac: fix a missing check for build_skb Paolo Abeni <pabeni@redhat.com> - 2016-01-13 09:40 +0100
      Re: bgmac: fix a missing check for build_skb Weidong Wang <wangweidong1@huawei.com> - 2016-01-13 11:30 +0100
        Re: bgmac: fix a missing check for build_skb Felix Fietkau <nbd@openwrt.org> - 2016-01-13 12:00 +0100
          Re: bgmac: fix a missing check for build_skb Weidong Wang <wangweidong1@huawei.com> - 2016-01-13 12:40 +0100
          Re: bgmac: fix a missing check for build_skb David Miller <davem@davemloft.net> - 2016-01-13 16:40 +0100
    [PATCH net-next v2] bgmac: fix a missing check for build_skb Weidong Wang <wangweidong1@huawei.com> - 2016-01-13 13:00 +0100
      Re: [PATCH net-next v2] bgmac: fix a missing check for build_skb Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-13 16:00 +0100
        Re: [PATCH net-next v2] bgmac: fix a missing check for build_skb Weidong Wang <wangweidong1@huawei.com> - 2016-01-14 03:20 +0100
    Re: bgmac: fix a missing check for build_skb Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-01-13 13:20 +0100

#1308001 — bgmac: fix a missing check for build_skb

FromWeidong Wang <wangweidong1@huawei.com>
Date2016-01-13 04:10 +0100
Subjectbgmac: fix a missing check for build_skb
Message-ID<qQjYR-1Vd-1@gated-at.bofh.it>
when build_skb failed, it may occure a NULL pointer.
So add a 'NULL check' for it.

Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 21e3c38..d75180a 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -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 +
-- 
1.9.0

[toc] | [next] | [standalone]

#1308055

FromDavid Miller <davem@davemloft.net>
Date2016-01-13 06:30 +0100
Message-ID<qQmal-3tP-5@gated-at.bofh.it>
In reply to#1308001
From: Weidong Wang <wangweidong1@huawei.com>
Date: Wed, 13 Jan 2016 11:06:41 +0800

> when build_skb failed, it may occure a NULL pointer.
> So add a 'NULL check' for it.
> 
> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>

Applied, thanks.

[toc] | [prev] | [next] | [standalone]

#1308170

FromPaolo Abeni <pabeni@redhat.com>
Date2016-01-13 09:40 +0100
Message-ID<qQp8e-5ru-11@gated-at.bofh.it>
In reply to#1308001
On Wed, 2016-01-13 at 11:06 +0800, Weidong Wang wrote:
> when build_skb failed, it may occure a NULL pointer.
> So add a 'NULL check' for it.
> 
> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
> ---
>  drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 21e3c38..d75180a 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -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)) {

?

> +				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 +

[toc] | [prev] | [next] | [standalone]

#1308263

FromWeidong Wang <wangweidong1@huawei.com>
Date2016-01-13 11:30 +0100
Message-ID<qQqQG-6Le-25@gated-at.bofh.it>
In reply to#1308170
On 2016/1/13 16:34, Paolo Abeni wrote:
> On Wed, 2016-01-13 at 11:06 +0800, Weidong Wang wrote:
>> when build_skb failed, it may occure a NULL pointer.
>> So add a 'NULL check' for it.
>>
>> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
>> ---
>>  drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 21e3c38..d75180a 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -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)) {
> 
> ?
> 

What to instead of it?

Regards,
Weidong

>> +				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 +
> 
> 
> .
>

[toc] | [prev] | [next] | [standalone]

#1308286

FromFelix Fietkau <nbd@openwrt.org>
Date2016-01-13 12:00 +0100
Message-ID<qQrjI-6WP-15@gated-at.bofh.it>
In reply to#1308263
On 2016-01-13 11:25, Weidong Wang wrote:
> On 2016/1/13 16:34, Paolo Abeni wrote:
>> On Wed, 2016-01-13 at 11:06 +0800, Weidong Wang wrote:
>>> when build_skb failed, it may occure a NULL pointer.
>>> So add a 'NULL check' for it.
>>>
>>> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
>>> ---
>>>  drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>>> index 21e3c38..d75180a 100644
>>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>>> @@ -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)) {
>> 
>> ?
>> 
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1308312

FromWeidong Wang <wangweidong1@huawei.com>
Date2016-01-13 12:40 +0100
Message-ID<qQrWp-7sj-9@gated-at.bofh.it>
In reply to#1308286
On 2016/1/13 18:57, Felix Fietkau wrote:
> On 2016-01-13 11:25, Weidong Wang wrote:
>> On 2016/1/13 16:34, Paolo Abeni wrote:
>>> On Wed, 2016-01-13 at 11:06 +0800, Weidong Wang wrote:
>>>> when build_skb failed, it may occure a NULL pointer.
>>>> So add a 'NULL check' for it.
>>>>
>>>> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
>>>> ---
>>>>  drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>>>> index 21e3c38..d75180a 100644
>>>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>>>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>>>> @@ -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)) {
>>>
>>> ?
>>>
>>
>> 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.

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

Regards,
Weidong

[toc] | [prev] | [next] | [standalone]

#1308510

FromDavid Miller <davem@davemloft.net>
Date2016-01-13 16:40 +0100
Message-ID<qQvGF-1Gf-1@gated-at.bofh.it>
In reply to#1308286
From: Felix Fietkau <nbd@openwrt.org>
Date: Wed, 13 Jan 2016 11:57:51 +0100

> 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.

[toc] | [prev] | [next] | [standalone]

#1308322 — [PATCH net-next v2] bgmac: fix a missing check for build_skb

FromWeidong Wang <wangweidong1@huawei.com>
Date2016-01-13 13:00 +0100
Subject[PATCH net-next v2] bgmac: fix a missing check for build_skb
Message-ID<qQsfM-7Bq-15@gated-at.bofh.it>
In reply to#1308001
when build_skb failed, it may occur a NULL pointer.
So add a 'NULL check' for it.

Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
---
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(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index c7798d3..3974152 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -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 +
-- 
1.9.0

[toc] | [prev] | [next] | [standalone]

#1308476 — Re: [PATCH net-next v2] bgmac: fix a missing check for build_skb

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-01-13 16:00 +0100
SubjectRe: [PATCH net-next v2] bgmac: fix a missing check for build_skb
Message-ID<qQv3Y-19C-5@gated-at.bofh.it>
In reply to#1308322
On Wed, 2016-01-13 at 19:53 +0800, Weidong Wang wrote:
> when build_skb failed, it may occur a NULL pointer.
> So add a 'NULL check' for it.
> 
> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
> ---
> 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.

[toc] | [prev] | [next] | [standalone]

#1308964 — Re: [PATCH net-next v2] bgmac: fix a missing check for build_skb

FromWeidong Wang <wangweidong1@huawei.com>
Date2016-01-14 03:20 +0100
SubjectRe: [PATCH net-next v2] bgmac: fix a missing check for build_skb
Message-ID<qQFG1-gd-1@gated-at.bofh.it>
In reply to#1308476
On 2016/1/13 22:52, Eric Dumazet wrote:
> On Wed, 2016-01-13 at 19:53 +0800, Weidong Wang wrote:
>> when build_skb failed, it may occur a NULL pointer.
>> So add a 'NULL check' for it.
>>
>> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
>> ---
>> 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.
> 
> 
OK.

Regards,
Weidong

> 
>

[toc] | [prev] | [next] | [standalone]

#1308334

FromSergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date2016-01-13 13:20 +0100
Message-ID<qQsz8-7YE-7@gated-at.bofh.it>
In reply to#1308001
Hello.

On 1/13/2016 6:06 AM, Weidong Wang wrote:

> when build_skb failed, it may occure a NULL pointer.
> So add a 'NULL check' for it.
>
> Signed-off-by: Weidong Wang <wangweidong1@huawei.com>
> ---
>   drivers/net/ethernet/broadcom/bgmac.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 21e3c38..d75180a 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -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?

> +				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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web