linux-kernel - Re: [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered

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:   Tue, 8 Dec 2020 15:31:39 +0100
From:   "Ahmed S. Darwish" <a.darwish@...utronix.de>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Jonathan Corbet <corbet@....net>,
        John Hubbard <jhubbard@...dia.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "Sebastian A. Siewior" <bigeasy@...utronix.de>
Subject: Re: [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption
 is automatically altered

Hi Jason,

On Mon, Dec 07, 2020 at 04:43:16PM -0400, Jason Gunthorpe wrote:
...
>
> The thing that was confusing is if it was appropriate to use a
> seqcount in case where write side preemption was not disabled - which
> is safe only if the read side doesn't spin.
>

No, that's not correct.

What was confusing was that *everyone* in that mm/ thread, including
yourself, thought that write_seqcount_begin() will automatically disable
preemption for plain seqcount_t:

  https://lkml.kernel.org/r/20201030235121.GQ2620339@nvidia.com

Quoting Peter Xu: "My understanding is that we used
raw_write_seqcount_t_begin() because we're with spin lock so assuming we
disabled preemption already."

Quoting you: "write_seqcount_begin - Enforces preemption off".

And that's why this patch explicitly adds to the kernel-doc: "Preemption
will be automatically disabled if and only if the seqcount write
serialization lock is associated, and preemptible."

Honestly, I should've added that statement anyway in my original
"sequence counters with associated locks" submission. I take the blame
for the resulting confusion, and I'm sorry...

~~~~~~~~~~~~~~~~~~~~

Now regarding the other point, where the seqcount_t write side does not
need to disable preemption if the reader does not spin. We've already
discussed that (at length) last time.

There comes a point where you decide what level of documentation to add,
and what level to skip.

Because in the end, you don't want to confuse "Joe, the generic driver
developer" with too much details that's not relevant to their task at
hand.

You want to keep the general case simple, but the special case do-able.
And you also want to encourage people to use the standard write side
critical section entry and exit functions, as much as possible.

Because, oh, look at that:

  [PATCH v2 0/6] seqlock: seqcount_t call sites bugfixes
  https://lkml.kernel.org/r/20200603144949.1122421-1-a.darwish@linutronix.de

Sequence counters are already hard to get right. And driver developers
get things wrong, a lot -- you don't want to confuse them even further.

For developers who're advanced enough to know the difference, they don't
need the kernel-doc anyway. And that's why I've kindly asked to add the
following to your mm/ patch (which you did, thanks):

	/*
	 * Disabling preemption is not needed for the write side, as
	 * the read side does not spin, but goes to mmap_lock.
	 * ...
	 */

And IMHO, that should be enough. Developers of such special cases are
already assumed to know what they're doing.

Thanks a lot,

--
Ahmed S. Darwish
Linutronix GmbH