netdev - Re: [PATCH net] net: fix race between napi kthread mode and busy poll

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: <20210225150048.23ed87c9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 25 Feb 2021 15:00:48 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Wei Wang <weiwan@...gle.com>
Cc:     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>,
        Martin Zaharinov <micron10@...il.com>
Subject: Re: [PATCH net] net: fix race between napi kthread mode and busy
 poll

On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
> Hmm... I don't think the above patch would work. Consider a situation that:
> 1. At first, the kthread is in sleep mode.
> 2. Then someone calls napi_schedule() to schedule work on this napi.
> So ____napi_schedule() is called. But at this moment, the kthread is
> not yet in RUNNING state. So this function does not set SCHED_THREAD
> bit.
> 3. Then wake_up_process() is called to wake up the thread.
> 4. Then napi_threaded_poll() calls napi_thread_wait().

But how is the task not in running state outside of napi_thread_wait()?

My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().

> woken is false
> and SCHED_THREAD bit is not set. So the kthread will go to sleep again
> (in INTERRUPTIBLE mode) when schedule() is called, and waits to be
> woken up by the next napi_schedule().
> That will introduce arbitrary delay for the napi->poll() to be called.
> Isn't it? Please enlighten me if I did not understand it correctly.

Probably just me not understanding the scheduler :)

> I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
> Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
> kthread_create().

Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.