| 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
| ||
|
Date: Fri, 26 Feb 2021 14:37:56 -0800
From: Wei Wang <weiwan@...gle.com>
To: Martin Zaharinov <micron10@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
Alexander Duyck <alexanderduyck@...com>,
Eric Dumazet <edumazet@...gle.com>,
"David S . Miller" <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>,
Hannes Frederic Sowa <hannes@...essinduktion.org>,
Alexander Duyck <alexander.duyck@...il.com>
Subject: Re: [PATCH net] net: fix race between napi kthread mode and busy poll
On Fri, Feb 26, 2021 at 2:36 PM Martin Zaharinov <micron10@...il.com> wrote:
>
>
>
> On Sat, Feb 27, 2021, 00:24 Wei Wang <weiwan@...gle.com> wrote:
>>
>> On Fri, Feb 26, 2021 at 1:35 PM Jakub Kicinski <kuba@...nel.org> wrote:
>> >
>> > On Fri, 26 Feb 2021 10:28:00 -0800 Wei Wang wrote:
>> > > Hi Martin,
>> > > Could you help try the following new patch on your setup and let me
>> > > know if there are still issues?
>> >
>> > FWIW your email got line wrapped for me.
>> >
>> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> > > index ddf4cfc12615..9ed0f89ccdd5 100644
>> > > --- a/include/linux/netdevice.h
>> > > +++ b/include/linux/netdevice.h
>> > > @@ -357,9 +357,10 @@ enum {
>> > > NAPI_STATE_NPSVC, /* Netpoll - don't dequeue
>> > > from poll_list */
>> > > NAPI_STATE_LISTED, /* NAPI added to system lists */
>> > > NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no
>> > > busy polling */
>> > > - NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */
>> > > + NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() grabs SHED
>> >
>> > nit: SHED -> SCHED
>> Ack.
>>
>> >
>> > > bit and could busy poll */
>> > > NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over
>> > > softirq processing*/
>> > > NAPI_STATE_THREADED, /* The poll is performed
>> > > inside its own thread*/
>> > > + NAPI_STATE_SCHED_BUSY_POLL, /* Napi is currently scheduled
>> > > in busy poll mode */
>> >
>> > nit: Napi -> NAPI ?
>> Ack.
>>
>> >
>> > > };
>> > >
>> > > enum {
>> > > @@ -372,6 +373,7 @@ enum {
>> > > NAPIF_STATE_IN_BUSY_POLL = BIT(NAPI_STATE_IN_BUSY_POLL),
>> > > NAPIF_STATE_PREFER_BUSY_POLL = BIT(NAPI_STATE_PREFER_BUSY_POLL),
>> > > NAPIF_STATE_THREADED = BIT(NAPI_STATE_THREADED),
>> > > + NAPIF_STATE_SCHED_BUSY_POLL = BIT(NAPI_STATE_SCHED_BUSY_POLL),
>> > > };
>> > >
>> > > enum gro_result {
>> > > diff --git a/net/core/dev.c b/net/core/dev.c
>> > > index 6c5967e80132..c717b67ce137 100644
>> > > --- a/net/core/dev.c
>> > > +++ b/net/core/dev.c
>> > > @@ -1501,15 +1501,14 @@ static int napi_kthread_create(struct napi_struct *n)
>> > > {
>> > > int err = 0;
>> > >
>> > > - /* Create and wake up the kthread once to put it in
>> > > - * TASK_INTERRUPTIBLE mode to avoid the blocked task
>> > > - * warning and work with loadavg.
>> > > + /* Avoid using kthread_run() here to prevent race
>> > > + * between softirq and kthread polling.
>> > > */
>> > > - n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
>> > > - n->dev->name, n->napi_id);
>> > > + n->thread = kthread_create(napi_threaded_poll, n, "napi/%s-%d",
>> > > + n->dev->name, n->napi_id);
>> >
>> > I'm not sure this takes care of rapid:
>> >
>> > dev_set_threaded(0)
>> > # NAPI gets sent to sirq
>> > dev_set_threaded(1)
>> >
>> > since subsequent set_threaded(1) doesn't spawn the thread "afresh".
>> >
>>
>> I think the race between softirq and kthread could be purely dependent
>> on the SCHED bit. In napi_schedule_prep(), we check if SCHED bit is
>> set. And we only call ____napi_schedule() when SCHED bit is not set.
>> In ____napi_schedule(), we either wake up kthread, or raise softirq,
>> never both.
>> So as long as we don't wake up the kthread when creating it, there
>> should not be a chance of race between softirq and kthread.
>>
>> > > if (IS_ERR(n->thread)) {
>> > > err = PTR_ERR(n->thread);
>> > > - pr_err("kthread_run failed with err %d\n", err);
>> > > + pr_err("kthread_create failed with err %d\n", err);
>> > > n->thread = NULL;
>> > > }
>> > >
>> > > @@ -6486,6 +6485,7 @@ bool napi_complete_done(struct napi_struct *n,
>> > > int work_done)
>> > > WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));
>> > >
>> > > new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
>> > > + NAPIF_STATE_SCHED_BUSY_POLL |
>> > > NAPIF_STATE_PREFER_BUSY_POLL);
>> > >
>> > > /* If STATE_MISSED was set, leave STATE_SCHED set,
>> > > @@ -6525,6 +6525,7 @@ static struct napi_struct *napi_by_id(unsigned
>> > > int napi_id)
>> > >
>> > > static void __busy_poll_stop(struct napi_struct *napi, bool skip_schedule)
>> > > {
>> > > + clear_bit(NAPI_STATE_SCHED_BUSY_POLL, &napi->state);
>> > > if (!skip_schedule) {
>> > > gro_normal_list(napi);
>> > > __napi_schedule(napi);
>> > > @@ -6624,7 +6625,8 @@ void napi_busy_loop(unsigned int napi_id,
>> > > }
>> > > if (cmpxchg(&napi->state, val,
>> > > val | NAPIF_STATE_IN_BUSY_POLL |
>> > > - NAPIF_STATE_SCHED) != val) {
>> > > + NAPIF_STATE_SCHED |
>> > > + NAPIF_STATE_SCHED_BUSY_POLL) != val) {
>> > > if (prefer_busy_poll)
>> > >
>> > > set_bit(NAPI_STATE_PREFER_BUSY_POLL, &napi->state);
>> > > goto count;
>> > > @@ -6971,7 +6973,10 @@ static int napi_thread_wait(struct napi_struct *napi)
>> > > set_current_state(TASK_INTERRUPTIBLE);
>> > >
>> > > while (!kthread_should_stop() && !napi_disable_pending(napi)) {
>> > > - if (test_bit(NAPI_STATE_SCHED, &napi->state)) {
>> > > + unsigned long val = READ_ONCE(napi->state);
>> > > +
>> > > + if (val & NAPIF_STATE_SCHED &&
>> > > + !(val & NAPIF_STATE_SCHED_BUSY_POLL)) {
>> >
>> > Again, not protected from the napi_disable() case AFAICT.
>>
>> Hmmm..... Yes. I think you are right. I missed that napi_disable()
>> also grabs the SCHED bit. In this case, I think we have to use the
>> SCHED_THREADED bit. The SCHED_BUSY_POLL bit is not enough to protect
>> the race between napi_disable() and napi_threaded_poll(). :(
>> Sorry, I missed this point when evaluating both solutions. I will have
>> to switch to use the SCHED_THREADED bit.
>
>
>
> should I wait with the test
> when you fix this?
>
Yes. Please. Sorry for the confusion.
>>
>>
>> >
>> > > WARN_ON(!list_empty(&napi->poll_list));
>> > > __set_current_state(TASK_RUNNING);
>> > > return 0;
Powered by blists - more mailing lists