| 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
| ||
|
Message-ID: <ZSkD4RLAhJaW3VyB@gmail.com>
Date: Fri, 13 Oct 2023 10:46:25 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.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: [PATCH] locking/seqlock: Propagate 'const' pointers within read-only
methods, remove forced type casts
* Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 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!
Yeah, that is absolutely what I too think 'const' means - and sorry, I
didn't expand: I meant something like the patch below, which clearly
separates the 'const' from the non-const pointer uses within
<linux/seqlock.h> and removes the two forced type casts I was unhappy
about.
The 'bug' was that the __seqprop_*ptr() wrapper was used with both const
and non-const pointers, and we forced a type and lost a tiny bit of
potential const propagation. The code was fine and I should not have called
it a 'bug', but I consider the dropping of 'const' a bad pattern, and I
sometimes exaggerate problems to trick^W convince developers to continue
working along a given path...
In hindsight my "break the world" expectation was overblown too: our const
propagation through these methods was almost complete already, and the
fixes stayed within <linux/seqlock.h>.
This patch could probably be split into two patches. Lightly tested only.
Does this work for you?
Thanks,
Ingo
===================>
From: Ingo Molnar <mingo@...nel.org>
Date: Fri, 13 Oct 2023 10:15:46 +0200
Subject: [PATCH] locking/seqlock: Propagate 'const' pointers within read-only methods, remove forced type casts
Currently __seqprop_ptr() is an inline function that must chose to either
use 'const' or non-const seqcount related pointers - but this results in
the undesirable loss of 'const' propagation, via a forced type cast.
The easiest solution would be to turn the pointer wrappers into macros that
pass through whatever type is passed to them - but the clever maze of
seqlock API instantiation macros relies on the GCC CPP '##' macro
extension, which isn't recursive, so inline functions must be used here.
So create two wrapper variants instead: 'ptr' and 'const_ptr', and pick the
right one for the codepaths that are const: read_seqcount_begin() and
read_seqcount_retry().
This cleans up type handling and allows the removal of all type forcing.
No change in functionality.
Signed-off-by: Ingo Molnar <mingo@...nel.org>
Cc: Oleg Nesterov <oleg@...hat.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Waiman Long <longman@...hat.com>
Cc: Will Deacon <will.deacon@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Paul E. McKenney <paulmck@...nel.org>
---
include/linux/seqlock.h | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 4b8dcd3a0d93..80f21d2ca2aa 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -200,9 +200,15 @@ typedef struct seqcount_##lockname { \
} seqcount_##lockname##_t; \
\
static __always_inline seqcount_t * \
-__seqprop_##lockname##_ptr(const seqcount_##lockname##_t *s) \
+__seqprop_##lockname##_ptr(seqcount_##lockname##_t *s) \
{ \
- return (void *)&s->seqcount; /* drop const */ \
+ return &s->seqcount; \
+} \
+ \
+static __always_inline const seqcount_t * \
+__seqprop_##lockname##_const_ptr(const seqcount_##lockname##_t *s) \
+{ \
+ return &s->seqcount; \
} \
\
static __always_inline unsigned \
@@ -247,9 +253,14 @@ __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s) \
* __seqprop() for seqcount_t
*/
-static inline seqcount_t *__seqprop_ptr(const seqcount_t *s)
+static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
{
- return (void *)s; /* drop const */
+ return s;
+}
+
+static inline const seqcount_t *__seqprop_const_ptr(const seqcount_t *s)
+{
+ return s;
}
static inline unsigned __seqprop_sequence(const seqcount_t *s)
@@ -302,6 +313,7 @@ SEQCOUNT_LOCKNAME(mutex, struct mutex, true, mutex)
__seqprop_case((s), mutex, prop))
#define seqprop_ptr(s) __seqprop(s, ptr)(s)
+#define seqprop_const_ptr(s) __seqprop(s, const_ptr)(s)
#define seqprop_sequence(s) __seqprop(s, sequence)(s)
#define seqprop_preemptible(s) __seqprop(s, preemptible)(s)
#define seqprop_assert(s) __seqprop(s, assert)(s)
@@ -353,7 +365,7 @@ SEQCOUNT_LOCKNAME(mutex, struct mutex, true, mutex)
*/
#define read_seqcount_begin(s) \
({ \
- seqcount_lockdep_reader_access(seqprop_ptr(s)); \
+ seqcount_lockdep_reader_access(seqprop_const_ptr(s)); \
raw_read_seqcount_begin(s); \
})
@@ -419,7 +431,7 @@ SEQCOUNT_LOCKNAME(mutex, struct mutex, true, mutex)
* Return: true if a read section retry is required, else false
*/
#define __read_seqcount_retry(s, start) \
- do___read_seqcount_retry(seqprop_ptr(s), start)
+ do___read_seqcount_retry(seqprop_const_ptr(s), start)
static inline int do___read_seqcount_retry(const seqcount_t *s, unsigned start)
{
@@ -439,7 +451,7 @@ static inline int do___read_seqcount_retry(const seqcount_t *s, unsigned start)
* Return: true if a read section retry is required, else false
*/
#define read_seqcount_retry(s, start) \
- do_read_seqcount_retry(seqprop_ptr(s), start)
+ do_read_seqcount_retry(seqprop_const_ptr(s), start)
static inline int do_read_seqcount_retry(const seqcount_t *s, unsigned start)
{
Powered by blists - more mailing lists