[PATCH] net: core: dev: Initialise napi state correctly

[PATCH] net: core: dev: Initialise napi state correctly

[Dave McKay]

The state member of the napi_struct is not initialised correctly, it
sets the SCHED bit without initialising the state to zero first. This
results in peculiar behaviour if the original napi_struct didn't come
from a zero initialised region to start with.

This patch just sets it directly using the appropriate bitfield
constant.

Signed-off-by: Dave McKay <mckay.david@gmail.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 82f20022259d..250f97bf1973 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6276,7 +6276,7 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
 #ifdef CONFIG_NETPOLL
 	napi->poll_owner = -1;
 #endif
-	set_bit(NAPI_STATE_SCHED, &napi->state);
+	napi->state = NAPIF_STATE_SCHED;
 	napi_hash_add(napi);
 }
 EXPORT_SYMBOL(netif_napi_add);
-- 
2.20.1

Re: [PATCH] net: core: dev: Initialise napi state correctly

[Eric Dumazet]

On 01/18/2019 04:46 AM, Dave McKay wrote:
> The state member of the napi_struct is not initialised correctly, it
> sets the SCHED bit without initialising the state to zero first. This
> results in peculiar behaviour if the original napi_struct didn't come
> from a zero initialised region to start with.
> 
> This patch just sets it directly using the appropriate bitfield
> constant.
> 
> Signed-off-by: Dave McKay <mckay.david@gmail.com>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 82f20022259d..250f97bf1973 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6276,7 +6276,7 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
>  #ifdef CONFIG_NETPOLL
>  	napi->poll_owner = -1;
>  #endif
> -	set_bit(NAPI_STATE_SCHED, &napi->state);
> +	napi->state = NAPIF_STATE_SCHED;
>  	napi_hash_add(napi);
>  }
>  EXPORT_SYMBOL(netif_napi_add);
> 

I am curious, which driver has exhibit any issue with current code ?

Thanks.

Re: [PATCH] net: core: dev: Initialise napi state correctly

[David McKay]

On Fri, 18 Jan 2019 at 17:15, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 01/18/2019 04:46 AM, Dave McKay wrote:
> > The state member of the napi_struct is not initialised correctly, it
> > sets the SCHED bit without initialising the state to zero first. This
> > results in peculiar behaviour if the original napi_struct didn't come
> > from a zero initialised region to start with.
> >
> > This patch just sets it directly using the appropriate bitfield
> > constant.
> >
> > Signed-off-by: Dave McKay <mckay.david@gmail.com>
> > ---
> >  net/core/dev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 82f20022259d..250f97bf1973 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6276,7 +6276,7 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
> >  #ifdef CONFIG_NETPOLL
> >       napi->poll_owner = -1;
> >  #endif
> > -     set_bit(NAPI_STATE_SCHED, &napi->state);
> > +     napi->state = NAPIF_STATE_SCHED;
> >       napi_hash_add(napi);
> >  }
> >  EXPORT_SYMBOL(netif_napi_add);
> >
>
> I am curious, which driver has exhibit any issue with current code ?
>

Hi Eric,
    As far as I know, none! I suspect all the napi_adds() are called on areas
that come from kzalloc(). I sampled a few and all of them would
not have hit this problem as the memory was allocated  in zero initialised
memory anyway.

I was working on an experimental out of tree driver that allocates the
napi struct through kzalloc().
I could not understand why kzalloc() worked but kmalloc() sometimes
didn't. I tracked it down to this.

Am I wrong here? Seemed to fix it for me!

Cheers!

Re: [PATCH] net: core: dev: Initialise napi state correctly

[Eric Dumazet]

On 01/23/2019 10:49 AM, David McKay wrote:
> On Fri, 18 Jan 2019 at 17:15, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 01/18/2019 04:46 AM, Dave McKay wrote:
>>> The state member of the napi_struct is not initialised correctly, it
>>> sets the SCHED bit without initialising the state to zero first. This
>>> results in peculiar behaviour if the original napi_struct didn't come
>>> from a zero initialised region to start with.
>>>
>>> This patch just sets it directly using the appropriate bitfield
>>> constant.
>>>
>>> Signed-off-by: Dave McKay <mckay.david@gmail.com>
>>> ---
>>>  net/core/dev.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 82f20022259d..250f97bf1973 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -6276,7 +6276,7 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
>>>  #ifdef CONFIG_NETPOLL
>>>       napi->poll_owner = -1;
>>>  #endif
>>> -     set_bit(NAPI_STATE_SCHED, &napi->state);
>>> +     napi->state = NAPIF_STATE_SCHED;
>>>       napi_hash_add(napi);
>>>  }
>>>  EXPORT_SYMBOL(netif_napi_add);
>>>
>>
>> I am curious, which driver has exhibit any issue with current code ?
>>
> 
> gro_cell_init() maybe?
> 

Perfect example of something that would break with your patch :/

alloc_percpu() clears all memory.


gro_cells_init() does :

set_bit(NAPI_STATE_NO_BUSY_POLL, &cell->napi.state);
netif_napi_add(dev, &cell->napi, gro_cell_poll, ...)

So clearing napi->state in netif_napi_add() would remove the NAPI_STATE_NO_BUSY_POLL setting.

Re: [PATCH] net: core: dev: Initialise napi state correctly

[David Miller]

From: Dave McKay <mckay.david@gmail.com>
Date: Fri, 18 Jan 2019 12:46:32 +0000

> The state member of the napi_struct is not initialised correctly, it
> sets the SCHED bit without initialising the state to zero first. This
> results in peculiar behaviour if the original napi_struct didn't come
> from a zero initialised region to start with.
> 
> This patch just sets it directly using the appropriate bitfield
> constant.
> 
> Signed-off-by: Dave McKay <mckay.david@gmail.com>

Caller needs to pass in properly initialized memory, that means
bzero()'d.