netdev - Re: [PATCH net-next v6 3/3] net: add sysfs attribute to control napi threaded mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Jan 2021 15:08:02 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Wei Wang <weiwan@...gle.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Felix Fietkau <nbd@....name>
Subject: Re: [PATCH net-next v6 3/3] net: add sysfs attribute to control napi
 threaded mode

On Fri, Jan 15, 2021 at 1:54 PM Wei Wang <weiwan@...gle.com> wrote:
>
> On Thu, Jan 14, 2021 at 7:34 PM Alexander Duyck
> <alexander.duyck@...il.com> wrote:
> >
> > On Thu, Jan 14, 2021 at 4:34 PM Wei Wang <weiwan@...gle.com> wrote:
> > >
> > > This patch adds a new sysfs attribute to the network device class.
> > > Said attribute provides a per-device control to enable/disable the
> > > threaded mode for all the napi instances of the given network device.
> > >
> > > Co-developed-by: Paolo Abeni <pabeni@...hat.com>
> > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > Co-developed-by: Hannes Frederic Sowa <hannes@...essinduktion.org>
> > > Signed-off-by: Hannes Frederic Sowa <hannes@...essinduktion.org>
> > > Co-developed-by: Felix Fietkau <nbd@....name>
> > > Signed-off-by: Felix Fietkau <nbd@....name>
> > > Signed-off-by: Wei Wang <weiwan@...gle.com>
> > > ---
> > >  include/linux/netdevice.h |  2 ++
> > >  net/core/dev.c            | 28 +++++++++++++++++
> > >  net/core/net-sysfs.c      | 63 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 93 insertions(+)
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index c24ed232c746..11ae0c9b9350 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -497,6 +497,8 @@ static inline bool napi_complete(struct napi_struct *n)
> > >         return napi_complete_done(n, 0);
> > >  }
> > >
> > > +int dev_set_threaded(struct net_device *dev, bool threaded);
> > > +
> > >  /**
> > >   *     napi_disable - prevent NAPI from scheduling
> > >   *     @n: NAPI context
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index edcfec1361e9..d5fb95316ea8 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -6754,6 +6754,34 @@ static int napi_set_threaded(struct napi_struct *n, bool threaded)
> > >         return err;
> > >  }
> > >
> > > +static void dev_disable_threaded_all(struct net_device *dev)
> > > +{
> > > +       struct napi_struct *napi;
> > > +
> > > +       list_for_each_entry(napi, &dev->napi_list, dev_list)
> > > +               napi_set_threaded(napi, false);
> > > +}
> > > +
> > > +int dev_set_threaded(struct net_device *dev, bool threaded)
> > > +{
> > > +       struct napi_struct *napi;
> > > +       int ret;
> > > +
> > > +       dev->threaded = threaded;
> > > +       list_for_each_entry(napi, &dev->napi_list, dev_list) {
> > > +               ret = napi_set_threaded(napi, threaded);
> > > +               if (ret) {
> > > +                       /* Error occurred on one of the napi,
> > > +                        * reset threaded mode on all napi.
> > > +                        */
> > > +                       dev_disable_threaded_all(dev);
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       return ret;
> >
> > This doesn't seem right. The NAPI instances can be active while this
> > is occuring can they not? I would think at a minimum you need to go
> > through a napi_disable/napi_enable in order to toggle this value for
> > each NAPI instance. Otherwise aren't you at risk for racing and having
> > a napi_schedule attempt to wake up the thread before it has been
> > allocated?
>
>
> Yes. The napi instance could be active when this occurs. And I think
> it is OK. It is cause napi_set_threaded() only sets
> NAPI_STATE_THREADED bit after successfully created the kthread. And
> ___napi_schedule() only tries to wake up the kthread after testing the
> THREADED bit.

But what do you have guaranteeing that the kthread has been written to
memory? That is what I was getting at. Just because you have written
the value doesn't mean it is in memory yet so you would probably need
an smb_mb__before_atomic() barrier call before you set the bit.

Also I am not sure it is entirely safe to have the instance polling
while you are doing this. That is why I am thinking if the instance is
enabled then a napi_disable/napi_enable would be preferable.