| Message ID | 20190118124632.17183-1-mckay.david@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: core: dev: Initialise napi state correctly | expand |
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.
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!
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.
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.
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);
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(-)