* Re: bgmac: fix a missing check for build_skb
2016-01-13 3:06 bgmac: fix a missing check for build_skb Weidong Wang
@ 2016-01-13 5:24 ` David Miller
2016-01-13 8:34 ` Paolo Abeni
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2016-01-13 5:24 UTC (permalink / raw)
To: wangweidong1; +Cc: nbd, zajec5, hauke, joe, peter.senna, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: bgmac: fix a missing check for build_skb
2016-01-13 3:06 bgmac: fix a missing check for build_skb Weidong Wang
2016-01-13 5:24 ` David Miller
@ 2016-01-13 8:34 ` Paolo Abeni
2016-01-13 10:25 ` Weidong Wang
2016-01-13 11:53 ` [PATCH net-next v2] " Weidong Wang
2016-01-13 12:10 ` Sergei Shtylyov
3 siblings, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2016-01-13 8:34 UTC (permalink / raw)
To: Weidong Wang
Cc: David Miller, nbd, zajec5, hauke, Joe Perches, peter.senna,
netdev, linux-kernel
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 +
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: bgmac: fix a missing check for build_skb
2016-01-13 8:34 ` Paolo Abeni
@ 2016-01-13 10:25 ` Weidong Wang
2016-01-13 10:57 ` Felix Fietkau
0 siblings, 1 reply; 11+ messages in thread
From: Weidong Wang @ 2016-01-13 10:25 UTC (permalink / raw)
To: Paolo Abeni
Cc: David Miller, nbd, zajec5, hauke, Joe Perches, peter.senna,
netdev, linux-kernel
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 +
>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: bgmac: fix a missing check for build_skb
2016-01-13 10:25 ` Weidong Wang
@ 2016-01-13 10:57 ` Felix Fietkau
2016-01-13 11:31 ` Weidong Wang
2016-01-13 15:34 ` David Miller
0 siblings, 2 replies; 11+ messages in thread
From: Felix Fietkau @ 2016-01-13 10:57 UTC (permalink / raw)
To: Weidong Wang, Paolo Abeni
Cc: David Miller, zajec5, hauke, Joe Perches, peter.senna, netdev,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: bgmac: fix a missing check for build_skb
2016-01-13 10:57 ` Felix Fietkau
@ 2016-01-13 11:31 ` Weidong Wang
2016-01-13 15:34 ` David Miller
1 sibling, 0 replies; 11+ messages in thread
From: Weidong Wang @ 2016-01-13 11:31 UTC (permalink / raw)
To: Felix Fietkau
Cc: Paolo Abeni, David Miller, zajec5, hauke, Joe Perches,
peter.senna, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: bgmac: fix a missing check for build_skb
2016-01-13 10:57 ` Felix Fietkau
2016-01-13 11:31 ` Weidong Wang
@ 2016-01-13 15:34 ` David Miller
1 sibling, 0 replies; 11+ messages in thread
From: David Miller @ 2016-01-13 15:34 UTC (permalink / raw)
To: nbd
Cc: wangweidong1, pabeni, zajec5, hauke, joe, peter.senna, netdev,
linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v2] bgmac: fix a missing check for build_skb
2016-01-13 3:06 bgmac: fix a missing check for build_skb Weidong Wang
2016-01-13 5:24 ` David Miller
2016-01-13 8:34 ` Paolo Abeni
@ 2016-01-13 11:53 ` Weidong Wang
2016-01-13 14:52 ` Eric Dumazet
2016-01-13 12:10 ` Sergei Shtylyov
3 siblings, 1 reply; 11+ messages in thread
From: Weidong Wang @ 2016-01-13 11:53 UTC (permalink / raw)
To: David Miller, nbd, zajec5, hauke, Joe Perches, peter.senna
Cc: netdev, linux-kernel, Paolo Abeni
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH net-next v2] bgmac: fix a missing check for build_skb
2016-01-13 11:53 ` [PATCH net-next v2] " Weidong Wang
@ 2016-01-13 14:52 ` Eric Dumazet
2016-01-14 2:10 ` Weidong Wang
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2016-01-13 14:52 UTC (permalink / raw)
To: Weidong Wang
Cc: David Miller, nbd, zajec5, hauke, Joe Perches, peter.senna,
netdev, linux-kernel, Paolo Abeni
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2] bgmac: fix a missing check for build_skb
2016-01-13 14:52 ` Eric Dumazet
@ 2016-01-14 2:10 ` Weidong Wang
0 siblings, 0 replies; 11+ messages in thread
From: Weidong Wang @ 2016-01-14 2:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, nbd, zajec5, hauke, Joe Perches, peter.senna,
netdev, linux-kernel, Paolo Abeni
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
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bgmac: fix a missing check for build_skb
2016-01-13 3:06 bgmac: fix a missing check for build_skb Weidong Wang
` (2 preceding siblings ...)
2016-01-13 11:53 ` [PATCH net-next v2] " Weidong Wang
@ 2016-01-13 12:10 ` Sergei Shtylyov
3 siblings, 0 replies; 11+ messages in thread
From: Sergei Shtylyov @ 2016-01-13 12:10 UTC (permalink / raw)
To: Weidong Wang, David Miller, nbd, zajec5, hauke, Joe Perches, peter.senna
Cc: netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread