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: <CAHk-=wgsybshMs3KLsyheP8hHhndrRhjo70L1qi+GdBZND8M+A@mail.gmail.com>
Date:   Thu, 12 Oct 2023 12:24:37 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alexey Gladkov <legion@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2 2/2] seqlock: change __seqprop() to return the function pointer

On Thu, 12 Oct 2023 at 11:21, Ingo Molnar <mingo@...nel.org> wrote:
>
> Okay, so dropping 'const' makes sense in terms of staying bug-compatible
> with the  previous API and not build-breaking the world - but could we
> perhaps follow this up with fixups of the type misuse and then a removal
> of the forced type casts from these APIs?

No. The use of 'const' here is *not* a bug.

The thing is, 'const' doesn't mean what you seem to think it means. A
'const' pointer in C in no way means that the target is constant - it
means that *THIS* use of the pointer will not write to the target!

Those may sound similar, but they are very very very different.

In particular, for the sequence

        seq = raw_seqcount_begin(seq_ptr);
        ...
        if (read_seqcount_retry(seq_ptr, seq))
                goto retry;

then 'seq_ptr' really *is* a 'const' pointer. The reader very much
does not write to it, and this very much is part of the fundamental
design.

The above is *literally* what sequence locking is all about: readers
are pure readers.

So no, making it a 'const seqptr_t' is absolutely not a bug. It's very
much a FUNDAMENTAL FEATURE of sequence locks.

Now, I'm not sure how much we actually take advantage of this and
there may not be very many cases of this all, but I really think this
is fundamental to the whole data structure, and there are most
definitely cases where we probably *should* take more advantage of the
fact that a read_seqcount is a read-only op.

For example, I think a function like 'dget_parent()' should actually
take a 'const struct dentry *dentry' as its argument, and the seqcount
is embedded inside that dentry and as such would also be const.

Right now the dentry code doesn't actually do that, because this isn't
one of the areas we have constified, but it's conceptually the right
thing to do. We use the dentry argument in a read-only manner
(although the *parent* that we look up then is written to, and in the
case of a root dentry, the parent may end up being the same as the
original).

Note that the 'const' should only be an issue for the begin/retry
cases, and obviously not for the write ones, but those readers do use
the seqprop_ptr() helper. So those absolutely need to handle the const
case.

So no, the cast wasn't "masking" anything at all. The 'const' is real.

            Linus

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ