bgmac: fix a missing check for build_skb

bgmac: fix a missing check for build_skb

[Weidong Wang]

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

Re: bgmac: fix a missing check for build_skb

[David Miller]

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.

Re: bgmac: fix a missing check for build_skb

[Paolo Abeni]

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 +

Re: bgmac: fix a missing check for build_skb

[Weidong Wang]

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

Re: bgmac: fix a missing check for build_skb

[Felix Fietkau]

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

Re: bgmac: fix a missing check for build_skb

[Weidong Wang]

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

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

[Weidong Wang]

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

Re: bgmac: fix a missing check for build_skb

[Sergei Shtylyov]

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

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

[Eric Dumazet]

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.

Re: bgmac: fix a missing check for build_skb

[David Miller]

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.

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

[Weidong Wang]

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

> 
>