netdev - Re: [PATCH net-next v2] net: stop napi kthreads when THREADED napi is disabled

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]

Message-ID: <CAAywjhR4znr9fsAdBKmYAwcyP8JgoesLkuS8p9D0goJBFFePWg@mail.gmail.com>
Date: Wed, 21 May 2025 10:28:20 -0700
From: Samiullah Khawaja <skhawaja@...gle.com>
To: Wei Wang <weiwan@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, almasrymina@...gle.com, 
	willemb@...gle.com, jdamato@...tly.com, mkarsten@...terloo.ca, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: stop napi kthreads when THREADED napi is disabled

On Wed, May 21, 2025 at 9:41 AM Wei Wang <weiwan@...gle.com> wrote:
>
> On Tue, May 20, 2025 at 7:09 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Mon, 19 May 2025 22:43:25 +0000 Samiullah Khawaja wrote:
> > > -/* Called with irq disabled */
> > > -static inline void ____napi_schedule(struct softnet_data *sd,
> > > -                                  struct napi_struct *napi)
> > > +static inline bool ____try_napi_schedule_threaded(struct softnet_data *sd,
> > > +                                               struct napi_struct *napi)
> > >  {
> > >       struct task_struct *thread;
> > > +     unsigned long new, val;
> > >
> > > -     lockdep_assert_irqs_disabled();
> > > +     do {
> > > +             val = READ_ONCE(napi->state);
> > > +
> > > +             if (!(val & NAPIF_STATE_THREADED))
> > > +                     return false;
> >
> > Do we really need to complicate the fastpath to make the slowpath easy?
> >
> > Plus I'm not sure it works.
> >
> >           CPU 0 (IRQ)             CPU 1 (NAPI thr)          CPU 2 (config)
> >                          if (test_bit(NAPI_STATE_SCHED_THREADED))
> >                                ...
> >
> >   ____napi_schedule()
> >   cmpxchg(...)
> >   wake_up_process(thread);
> >                                                        clear_bit(NAPI_STATE_THREADED)
> >                                                        kthread_stop(thread)
> >
> >                          if (kthread_should_stop())
> >                                 exit
> >
> > Right?
+1

If the kthread checks whether it was not SCHED before it died then
this should not occur.
> >
> Hmm right... I think the main issue is that while dev_set_threaded()
> clears STATE_THREADED, SCHED_THREADED could already be set meaning the
> napi is already scheduled. And napi_thread_wait() does not really
> check STATE_THREADED...
>
> > I think the shutting down thread should do this:
> >
> >         while (true) {
> >                 state = READ_ONCE()
> >
> >                 // safe to clear if thread owns the NAPI,
> >                 // or NAPI is completely idle
> >                 if (state & SCHED_THREADED || !(state & SCHED)) {
This might suffer from the problem you highlighted earlier,
CPU 0 (IRQ)             CPU 1 (NAPI thr)          CPU 2 (config)

  ____napi_schedule()
    if (test_bit(NAPI_STATE_THREADED))
    if (thread) {

 kthread_stop()
                              if (state & SCHED_THREADED || !(state & SCHED)) {
                                   state &= ~THREADED;
                              if (try_cmp_xchg())
                                   break

       set_bit(NAPI_STATE_SCHED_THREADED)
       wake_up_process(thread);

This would happen without the try_cmp_xchg logic that I added in my
patch in the __napi_schedule (in the fast path). __napi_schedule would
have to make sure that the kthread is not stopping while it is trying
to do SCHED. This is similar to the logic we have in
napi_schedule_prep that handles the STATE_DISABLE, STATE_SCHED and
STATE_MISSED scenarios. Also if it falls back to normal softirq, it
needs to make sure that the kthread is not polling at the same time.

> I think we should make sure SCHED_THREADED is cleared as well, or
> otherwise the thread is in the middle of calling napi_threaded_poll()
> and we can't just disable the thread?
+1

We need to make sure that any scheduled polling should be completed.

>
> >                         state &= ~THREADED;
>
> STATE_THREADED to be exact. Right?
>
> >                 } else {
> >                         msleep(1);
> >                         continue;
> >                 }
> >
> >                 if (try_cmpxchg())
> >                         break;
> >         }
> >
> > But that's just an idea, it could also be wrong... :S