Proposal for speculative safe list iterator

---
 arch/x86/include/asm/barrier.h | 12 ++++++++++++
 include/linux/list.h           |  3 ++-
 include/linux/nospec.h         | 16 ++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

arch/x86/include/asm/barrier.h +12 -0

index 35389b2af88e..722797ad74e2 100644
@@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 /* Override the default implementation from linux/nospec.h. */
 #define array_index_mask_nospec array_index_mask_nospec
 
+/* Override the default implementation from linux/nospec.h. */
+#define select_nospec(cond, exptrue, expfalse)				\
+({									\
+	typeof(exptrue) _out = (exptrue);				\
+									\
+	asm volatile("test %1, %1\n\t"					\
+	    "cmove %2, %0"						\
+	    : "+r" (_out)						\
+	    : "r" (cond), "r" (expfalse));				\
+	_out;								\
+})
+
 /* Prevent speculative execution past this barrier. */
 #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)

include/linux/list.h +2 -1

index dd6c2041d09c..1a1b39fdd122 100644
@@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_entry(pos, head, member)				\
 	for (pos = list_first_entry(head, typeof(*pos), member);	\
-	     !list_entry_is_head(pos, head, member);			\
+	    ({ bool _cond = !list_entry_is_head(pos, head, member);	\
+	     pos = select_nospec(_cond, pos, NULL); _cond; }); \
 	     pos = list_next_entry(pos, member))
 
 /**

include/linux/nospec.h +16 -0

index c1e79f72cd89..ca8ed81e4f9e 100644
@@ -67,4 +67,20 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 /* Speculation control for seccomp enforced mitigation */
 void arch_seccomp_spec_mitigate(struct task_struct *task);
 
+/**
+ * select_nospec - select a value without using a branch; equivalent to:
+ * cond ? exptrue : expfalse;
+ */
+#ifndef select_nospec
+#define select_nospec(cond, exptrue, expfalse)				\
+({									\
+	unsigned long _t = (unsigned long) (exptrue);			\
+	unsigned long _f = (unsigned long) (expfalse);			\
+	unsigned long _c = (unsigned long) (cond);			\
+	OPTIMIZER_HIDE_VAR(_c);						\
+	unsigned long _m = -((_c | -_c) >> (BITS_PER_LONG - 1));	\
+	(typeof(exptrue)) ((_t & _m) | (_f & ~_m));			\
+})
+#endif
+
 #endif /* _LINUX_NOSPEC_H */

Signature · 1 line

2.25.1

Greg Kroah-Hartman

maintainer

2022-02-17 19:29 UTC #2 lore raw

Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()

Quoted 50 lines · Jakob Koschel

list_for_each_entry() selects either the correct value (pos) or a safe value for the additional mispredicted iteration (NULL) for the list iterator.
list_for_each_entry() calls select_nospec(), which performs a branch-less select.
On x86, this select is performed via a cmov. Otherwise, it's performed via various shift/mask/etc. operations.
Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU Amsterdam.
Co-developed-by Brian Johannesmeyer <[email protected]>
Signed-off-by Brian Johannesmeyer <[email protected]>
Signed-off-by Jakob Koschel <[email protected]>
---
 arch/x86/include/asm/barrier.h | 12 ++++++++++++
 include/linux/list.h           |  3 ++-
 include/linux/nospec.h         | 16 ++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)
arch/x86/include/asm/barrier.h +12 -0
index 35389b2af88e..722797ad74e2 100644
@@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 /* Override the default implementation from linux/nospec.h. */
 #define array_index_mask_nospec array_index_mask_nospec
 
+/* Override the default implementation from linux/nospec.h. */
+#define select_nospec(cond, exptrue, expfalse)				\
+({									\
+	typeof(exptrue) _out = (exptrue);				\
+									\
+	asm volatile("test %1, %1\n\t"					\
+	    "cmove %2, %0"						\
+	    : "+r" (_out)						\
+	    : "r" (cond), "r" (expfalse));				\
+	_out;								\
+})
+
 /* Prevent speculative execution past this barrier. */
 #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
 
include/linux/list.h +0 -0
index dd6c2041d09c..1a1b39fdd122 100644

include/linux/list.h static inline void list_splice_tail_init(struct list_head *list, · line 642

Quoted 8 lines

include/linux/list.h +2 -1

@@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_entry(pos, head, member)				\
 	for (pos = list_first_entry(head, typeof(*pos), member);	\
-	     !list_entry_is_head(pos, head, member);			\
+	    ({ bool _cond = !list_entry_is_head(pos, head, member);	\
+	     pos = select_nospec(_cond, pos, NULL); _cond; }); \
 	     pos = list_next_entry(pos, member))

You are not "introducing" a new macro for this, you are modifying the existing one such that all users of it now have the select_nospec() call in it.

Is that intentional? This is going to hit a _lot_ of existing entries that probably do not need it at all.

Why not just create list_for_each_entry_nospec()?

thanks,

greg k-h

Jann Horn

2022-02-18 16:30 UTC #3 lore raw

Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()

Quoted 6 lines · Greg Kroah-Hartman

Quoted 5 lines · Jakob Koschel
list_for_each_entry() selects either the correct value (pos) or a safe value for the additional mispredicted iteration (NULL) for the list iterator.
list_for_each_entry() calls select_nospec(), which performs a branch-less select.

[...]

Quoted 8 lines

include/linux/list.h static inline void list_splice_tail_init(struct list_head *list, · line 641

Quoted 7 lines

 #define list_for_each_entry(pos, head, member)                               \
      for (pos = list_first_entry(head, typeof(*pos), member);        \

include/linux/list.h +2 -1

-          !list_entry_is_head(pos, head, member);                    \
+         ({ bool _cond = !list_entry_is_head(pos, head, member);     \
+          pos = select_nospec(_cond, pos, NULL); _cond; }); \
           pos = list_next_entry(pos, member))

Quoted 8 lines

You are not "introducing" a new macro for this, you are modifying the existing one such that all users of it now have the select_nospec() call in it.
Is that intentional? This is going to hit a _lot_ of existing entries that probably do not need it at all.
Why not just create list_for_each_entry_nospec()?

My understanding is that almost all uses of list_for_each_entry() currently create type-confused "pos" pointers when they terminate.

(As a sidenote, I've actually seen this lead to a bug in some out-of-tree code in the past, where someone had a construct like this:

list_for_each_entry(element, ...) {
if (...)
break; /* found the element we were looking for */
}
/* use element here */

and then got a "real" type confusion bug from that when no matching element was found.)

*Every time* you have a list_for_each_entry() iteration over some list where the list_head that you start from is not embedded in the same struct as the element list_heads (which is the normal case), and you don't break from the iteration early, a bogus type-confused pointer (which might not even be part of the same object as the real list head, but instead some random out-of-bounds memory in front of it) is assigned to "pos" (which I think is probably already a violation of the C standard, but whatever), and this means that almost every list_for_each_entry() loop ends with a branch that, when misspeculated, leads to speculative accesses to a type-confused pointer.

And once you're speculatively accessing type-confused pointers, and especially if you start writing to them or loading more pointers from them, it's really hard to reason about what might happen, just like with "normal" type confusion bugs.

If we don't want to keep this performance hit, then in the long term it might be a good idea to refactor away the (hideous) idea that the head of a list and its elements are exactly the same type and everything's just one big circular thing. Then we could change the data structures so that this speculative confusion can't happen anymore and avoid this explicit speculation avoidance on list iteration.

But for now, I think we probably need this.

Jann Horn

2022-02-18 16:30 UTC #4 lore raw

Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()

Quoted 9 lines · Jakob Koschel

list_for_each_entry() selects either the correct value (pos) or a safe value for the additional mispredicted iteration (NULL) for the list iterator.
list_for_each_entry() calls select_nospec(), which performs a branch-less select.
On x86, this select is performed via a cmov. Otherwise, it's performed via various shift/mask/etc. operations.

Quoted 7 lines

Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU Amsterdam.
Co-developed-by Brian Johannesmeyer <[email protected]>
Signed-off-by Brian Johannesmeyer <[email protected]>
Signed-off-by Jakob Koschel <[email protected]>

Yeah, I think this is the best way to do this without deeply intrusive changes to how lists are represented in memory.

Some notes on the specific implementation:

Quoted 10 lines

 arch/x86/include/asm/barrier.h | 12 ++++++++++++
 include/linux/list.h           |  3 ++-
 include/linux/nospec.h         | 16 ++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

arch/x86/include/asm/barrier.h +0 -0

index 35389b2af88e..722797ad74e2 100644

arch/x86/include/asm/barrier.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 56

Quoted 9 lines

arch/x86/include/asm/barrier.h +6 -0

@@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 /* Override the default implementation from linux/nospec.h. */
 #define array_index_mask_nospec array_index_mask_nospec
 
+/* Override the default implementation from linux/nospec.h. */
+#define select_nospec(cond, exptrue, expfalse)                         \
+({                                                                     \
+       typeof(exptrue) _out = (exptrue);                               \
+                                                                       \
+       asm volatile("test %1, %1\n\t"                                  \

This shouldn't need "volatile", because it is only necessary if _out is actually used. Using "volatile" here could prevent optimizing out useless code. OPTIMIZER_HIDE_VAR() also doesn't use "volatile".

arch/x86/include/asm/barrier.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 61

Quoted 5 lines

arch/x86/include/asm/barrier.h +5 -0

+           "cmove %2, %0"                                              \
+           : "+r" (_out)                                               \
+           : "r" (cond), "r" (expfalse));                              \
+       _out;                                                           \
+})

I guess the idea is probably to also add code like this for other important architectures, in particular arm64?

It might also be a good idea to rename the arch-overridable macro to something like "arch_select_nospec" and then have a wrapper macro in include/linux/nospec.h that takes care of type safety issues.

The current definition of the macro doesn't warn if you pass in incompatible pointer types, like this:

int *bogus_pointer_mix(int cond, int *a, long *b) {
return select_nospec(cond, a, b);
}

and if you pass in integers of different sizes, it may silently truncate to the size of the smaller one - this C code:

long wrong_int_conversion(int cond, int a, long b) {
return select_nospec(cond, a, b);
}

generates this assembly:

wrong_int_conversion:
test %edi, %edi
cmove %rdx, %esi
movslq %esi, %rax
ret

It might be a good idea to add something like a static_assert(__same_type(...), ...) to protect against that.

Quoted 8 lines

arch/x86/include/asm/barrier.h +0 -0

 /* Prevent speculative execution past this barrier. */
 #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)

include/linux/list.h +0 -0

index dd6c2041d09c..1a1b39fdd122 100644

include/linux/list.h static inline void list_splice_tail_init(struct list_head *list, · line 641

Quoted 7 lines

include/linux/list.h +2 -1

@@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_entry(pos, head, member)                         \
        for (pos = list_first_entry(head, typeof(*pos), member);        \
-            !list_entry_is_head(pos, head, member);                    \
+           ({ bool _cond = !list_entry_is_head(pos, head, member);     \
+            pos = select_nospec(_cond, pos, NULL); _cond; }); \
             pos = list_next_entry(pos, member))

I wonder if it'd look nicer to write it roughly like this:

#define NOSPEC_TYPE_CHECK(_guarded_var, _cond)                  \
({                                                              \
  bool __cond = (_cond);                                        \
  typeof(_guarded_var) *__guarded_var = &(_guarded_var);        \

*__guarded_var = select_nospec(__cond, *__guarded_var, NULL); \
__cond; \
})

#define list_for_each_entry(pos, head, member)                                \
        for (pos = list_first_entry(head, typeof(*pos), member);              \
             NOSPEC_TYPE_CHECK(head, !list_entry_is_head(pos, head, member)); \
             pos = list_next_entry(pos, member))

I think having a NOSPEC_TYPE_CHECK() like this makes it semantically clearer, and easier to add in other places? But I don't know if the others agree...

Quoted 14 lines

include/linux/list.h +0 -0

/**

include/linux/nospec.h +5 -0

index c1e79f72cd89..ca8ed81e4f9e 100644
@@ -67,4 +67,20 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 /* Speculation control for seccomp enforced mitigation */
 void arch_seccomp_spec_mitigate(struct task_struct *task);
 
+/**
+ * select_nospec - select a value without using a branch; equivalent to:
+ * cond ? exptrue : expfalse;
+ */
+#ifndef select_nospec

include/linux/nospec.h int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which, · line 84

Quoted 10 lines

include/linux/nospec.h +10 -0

+#define select_nospec(cond, exptrue, expfalse)                         \
+({                                                                     \
+       unsigned long _t = (unsigned long) (exptrue);                   \
+       unsigned long _f = (unsigned long) (expfalse);                  \
+       unsigned long _c = (unsigned long) (cond);                      \
+       OPTIMIZER_HIDE_VAR(_c);                                         \
+       unsigned long _m = -((_c | -_c) >> (BITS_PER_LONG - 1));        \
+       (typeof(exptrue)) ((_t & _m) | (_f & ~_m));                     \
+})
+#endif

(As a sidenote, it might be easier to implement a conditional zeroing primitive than a generic conditional select primitive if that's all you need, something like:

#define cond_nullptr_nospec(_cond, _exp) \
({ \
unsigned long __exp = (unsigned long)(_exp); \
unsigned long _mask = 0UL - !(_cond); \
OPTIMIZER_HIDE_VAR(_mask); \
(typeof(_exp)) (_mask & __exp); \
})

)

Jakob

author

2022-02-23 14:32 UTC #5 lore raw

Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()

Quoted 51 lines · Jann Horn

Quoted 9 lines · Jakob Koschel
list_for_each_entry() selects either the correct value (pos) or a safe value for the additional mispredicted iteration (NULL) for the list iterator.
list_for_each_entry() calls select_nospec(), which performs a branch-less select.
On x86, this select is performed via a cmov. Otherwise, it's performed via various shift/mask/etc. operations.
Quoted 7 lines
Kasper Acknowledgements: Jakob Koschel, Brian Johannesmeyer, Kaveh Razavi, Herbert Bos, Cristiano Giuffrida from the VUSec group at VU Amsterdam.
Co-developed-by Brian Johannesmeyer <[email protected]>
Signed-off-by Brian Johannesmeyer <[email protected]>
Signed-off-by Jakob Koschel <[email protected]>
Yeah, I think this is the best way to do this without deeply intrusive changes to how lists are represented in memory.
Some notes on the specific implementation:
Quoted 10 lines
arch/x86/include/asm/barrier.h | 12 ++++++++++++
include/linux/list.h           |  3 ++-
include/linux/nospec.h         | 16 ++++++++++++++++
3 files changed, 30 insertions(+), 1 deletion(-)
arch/x86/include/asm/barrier.h +0 -0
index 35389b2af88e..722797ad74e2 100644
arch/x86/include/asm/barrier.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 56
Quoted 9 lines
arch/x86/include/asm/barrier.h +6 -0
@@ -48,6 +48,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 /* Override the default implementation from linux/nospec.h. */
 #define array_index_mask_nospec array_index_mask_nospec
 
+/* Override the default implementation from linux/nospec.h. */
+#define select_nospec(cond, exptrue, expfalse)                         \
+({                                                                     \
+       typeof(exptrue) _out = (exptrue);                               \
+                                                                       \
+       asm volatile("test %1, %1\n\t"                                  \
This shouldn't need "volatile", because it is only necessary if _out is actually used. Using "volatile" here could prevent optimizing out useless code. OPTIMIZER_HIDE_VAR() also doesn't use "volatile".

Quoted 8 lines

arch/x86/include/asm/barrier.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 61
Quoted 5 lines
arch/x86/include/asm/barrier.h +5 -0
+           "cmove %2, %0"                                              \
+           : "+r" (_out)                                               \
+           : "r" (cond), "r" (expfalse));                              \
+       _out;                                                           \
+})
I guess the idea is probably to also add code like this for other important architectures, in particular arm64?

yes indeed, with a fallback of using the shifting/masking mechanism for other archs.

Quoted 22 lines

It might also be a good idea to rename the arch-overridable macro to something like "arch_select_nospec" and then have a wrapper macro in include/linux/nospec.h that takes care of type safety issues.
The current definition of the macro doesn't warn if you pass in incompatible pointer types, like this:
int *bogus_pointer_mix(int cond, int *a, long *b) {
return select_nospec(cond, a, b);
}
and if you pass in integers of different sizes, it may silently truncate to the size of the smaller one - this C code:
long wrong_int_conversion(int cond, int a, long b) {
return select_nospec(cond, a, b);
}
generates this assembly:

Quoted 8 lines

wrong_int_conversion:
test %edi, %edi
cmove %rdx, %esi
movslq %esi, %rax
ret
It might be a good idea to add something like a static_assert(__same_type(...), ...) to protect against that.

These are good points, thank you for your input. Will be good to incorporate.

Quoted 27 lines

Quoted 9 lines
arch/x86/include/asm/barrier.h +0 -0
 /* Prevent speculative execution past this barrier. */
 #define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
 
include/linux/list.h +0 -0
index dd6c2041d09c..1a1b39fdd122 100644
include/linux/list.h static inline void list_splice_tail_init(struct list_head *list, · line 641
Quoted 7 lines
include/linux/list.h +2 -1
@@ -636,7 +636,8 @@ static inline void list_splice_tail_init(struct list_head *list,
 */
 #define list_for_each_entry(pos, head, member)                         \
       for (pos = list_first_entry(head, typeof(*pos), member);        \
-            !list_entry_is_head(pos, head, member);                    \
+           ({ bool _cond = !list_entry_is_head(pos, head, member);     \
+            pos = select_nospec(_cond, pos, NULL); _cond; }); \
            pos = list_next_entry(pos, member))
I wonder if it'd look nicer to write it roughly like this:
#define NOSPEC_TYPE_CHECK(_guarded_var, _cond) \
({ \
bool __cond = (_cond); \
typeof(_guarded_var) *__guarded_var = &(_guarded_var); \
*__guarded_var = select_nospec(__cond, *__guarded_var, NULL); \
__cond; \
})

Quoted 8 lines

#define list_for_each_entry(pos, head, member)                                \
       for (pos = list_first_entry(head, typeof(*pos), member);              \
            NOSPEC_TYPE_CHECK(head, !list_entry_is_head(pos, head, member)); \
            pos = list_next_entry(pos, member))
I think having a NOSPEC_TYPE_CHECK() like this makes it semantically clearer, and easier to add in other places? But I don't know if the others agree...

That sounds like a good idea. I wonder if the pointer and dereference in NOSPEC_TYPE_CHECK() simply get optimized away. Or why you can't simply use _guarded_var directly instead of a pointer to it.

Quoted 38 lines

Quoted 15 lines
include/linux/list.h +0 -0
 /**
include/linux/nospec.h +5 -0
index c1e79f72cd89..ca8ed81e4f9e 100644
@@ -67,4 +67,20 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 /* Speculation control for seccomp enforced mitigation */
 void arch_seccomp_spec_mitigate(struct task_struct *task);
 
+/**
+ * select_nospec - select a value without using a branch; equivalent to:
+ * cond ? exptrue : expfalse;
+ */
+#ifndef select_nospec
include/linux/nospec.h int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which, · line 84
Quoted 10 lines
include/linux/nospec.h +10 -0
+#define select_nospec(cond, exptrue, expfalse)                         \
+({                                                                     \
+       unsigned long _t = (unsigned long) (exptrue);                   \
+       unsigned long _f = (unsigned long) (expfalse);                  \
+       unsigned long _c = (unsigned long) (cond);                      \
+       OPTIMIZER_HIDE_VAR(_c);                                         \
+       unsigned long _m = -((_c | -_c) >> (BITS_PER_LONG - 1));        \
+       (typeof(exptrue)) ((_t & _m) | (_f & ~_m));                     \
+})
+#endif
(As a sidenote, it might be easier to implement a conditional zeroing primitive than a generic conditional select primitive if that's all you need, something like:
#define cond_nullptr_nospec(_cond, _exp) \
({ \
unsigned long __exp = (unsigned long)(_exp); \
unsigned long _mask = 0UL - !(_cond); \
OPTIMIZER_HIDE_VAR(_mask); \
(typeof(_exp)) (_mask & __exp); \
})

Quoted 1 line

)

Ah yes, if NULL is actually the value to choose, this might be good enough.

Jann Horn

2022-02-19 19:44 UTC #6 lore raw

Re: [RFC PATCH 01/13] list: introduce speculative safe list_for_each_entry()

Quoted 8 lines · Jakob Koschel

list_for_each_entry() selects either the correct value (pos) or a safe value for the additional mispredicted iteration (NULL) for the list iterator.
list_for_each_entry() calls select_nospec(), which performs a branch-less select.
On x86, this select is performed via a cmov. Otherwise, it's performed via various shift/mask/etc. operations.

[...]

include/linux/list.h static inline void list_splice_tail_init(struct list_head *list, · line 641

Quoted 6 lines

 #define list_for_each_entry(pos, head, member)                         \
        for (pos = list_first_entry(head, typeof(*pos), member);        \

include/linux/list.h +2 -1

-            !list_entry_is_head(pos, head, member);                    \
+           ({ bool _cond = !list_entry_is_head(pos, head, member);     \
+            pos = select_nospec(_cond, pos, NULL); _cond; }); \
             pos = list_next_entry(pos, member))

Actually I do have one ugly question about this:

Is NULL a good/safe choice here?

We already know that CPUs try very aggressively to do store-to-load forwarding. Until now, my mental model of store-to-load forwarding was
basically: "The CPU has to guess whether the linear addresses will be the same, and once it knows the linear addresses, it can verify whether that guess was correct."

But of course that can't really be the whole mechanism, because many architectures guarantee that if you access the same physical page through multiple linear addresses, everything stays coherent. So I'm
wondering: Are there basically two stages of speculation based on address guesses? A first stage where the CPU guesses whether the linear addresses are the same, and a second stage where it assumes that different linear addresses also map to different physical addresses, or something like that?

And so, if we don't have a TLB entry for NULL, and we misspeculate through a speculative write to an object of type A at NULL and then a speculative read (at the same offset) from an object of type B at NULL, will we get speculative type confusion through the nonexistent object at NULL that lasts until either the branches are resolved or the page walk for NULL reports back that there is no page at NULL?

(Also, it's been known for a long time that speculative accesses to NULL can be a performance problem, too:
https://lwn.net/Articles/444336/)

So I'm wondering whether, on 64-bit architectures that have canonical address bits, it would be safer and also reduce the amount of useless pagetable walks to try to butcher up the canonical bits of the address somehow so that the CPU can quickly see that the access is bogus, without potentially having to do a pagetable walk first.

Jakob Koschel

author

2022-02-17 18:50 UTC #7 lore raw

RFC PATCH 2/13 scripts: coccinelle: adapt to find list_for_each_entry nospec issues

These changes are just temporary to illustrate how I composed the list of code locations mentioned here.
While for example the usage of &c->member is currently safe to use, it is an issue if c is set to NULL when the terminating condition is met.

---
 .../coccinelle/iterators/use_after_iter.cocci | 24 -------------------
 1 file changed, 24 deletions(-)

scripts/coccinelle/iterators/use_after_iter.cocci +0 ~~-24~~

index 676edd562eef..01563a242f00 100644
@@ -91,43 +91,19 @@ list_for_each_entry(c,...) S
 |
 list_for_each_entry_reverse(c,...) S
 |
-list_for_each_entry_continue(c,...) S
-|
-list_for_each_entry_continue_reverse(c,...) S
-|
-list_for_each_entry_from(c,...) S
-|
 list_for_each_entry_safe(c,...) S
 |
 list_for_each_entry_safe(x,c,...) S
 |
-list_for_each_entry_safe_continue(c,...) S
-|
-list_for_each_entry_safe_continue(x,c,...) S
-|
-list_for_each_entry_safe_from(c,...) S
-|
-list_for_each_entry_safe_from(x,c,...) S
-|
 list_for_each_entry_safe_reverse(c,...) S
 |
 list_for_each_entry_safe_reverse(x,c,...) S
 |
 hlist_for_each_entry(c,...) S
 |
-hlist_for_each_entry_continue(c,...) S
-|
-hlist_for_each_entry_from(c,...) S
-|
 hlist_for_each_entry_safe(c,...) S
 |
-list_remove_head(x,c,...)
-|
-list_entry_is_head(c,...)
-|
 sizeof(<+...c...+>)
-|
- &c->member
 |
 T c;
 |

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #8 lore raw

RFC PATCH 3/13 usb: remove the usage of the list iterator after the loop

It is unsafe to assume that &req->req != _req can only evaluate to false if the break within the list iterator is hit.

When the break is not hit, req is set to an address derived from the head element. If _req would match with that value of req it would allow continuing beyond the safety check even if the _req was never found within the list.

---
 drivers/usb/gadget/udc/gr_udc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/usb/gadget/udc/gr_udc.c +5 -2

index 4b35739d3695..18ae2c7a1656 100644
@@ -1695,6 +1695,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	struct gr_udc *dev;
 	int ret = 0;
 	unsigned long flags;
+	bool found = false;
 
 	ep = container_of(_ep, struct gr_ep, ep);
 	if (!_ep || !_req || (!ep->ep.desc && ep->num != 0))

@@ -1711,10 +1712,12 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 
 	/* Make sure it's actually queued on this endpoint */
 	list_for_each_entry(req, &ep->queue, queue) {
-		if (&req->req == _req)
+		if (&req->req == _req) {
+			found = true;
 			break;
+		}
 	}
-	if (&req->req != _req) {
+	if (!found) {
 		ret = -EINVAL;
 		goto out;
 	}

Signature · 1 line

2.25.1

Linus Torvalds

2022-02-17 19:29 UTC #9 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 3 lines · Jakob Koschel

It is unsafe to assume that &req->req != _req can only evaluate to false if the break within the list iterator is hit.

I don't understand what problem you are trying to fix.

Since "req" absolutely *has* to be stable for any of this code to be valid, then "&req->req" is stable and unambiguous too. The *only* way _req can point to it would be if we finished the iteration properly.

So I don't see the unsafeness.

Note that all this work with "speculative" execution fundamentally CAN NOT affect semantics of the code, yet this patch makes statements about exactly that.

That's not how CPU speculation works.

CPU speculation can expose hidden information that is not "semantically important" (typically through cache access patterns, but that's not the only way). So it might be exposing information it shouldn't.

But if speculation is actually changing semantics, then it's no longer "speculation" - it's just a bug, plain and simple (either a software bug due to insufficient serialization, or an actual hardware bug).

IOW, I don't want to see these kinds of apparently pointless changes to list walking. The patches should explain what that SECONDARY hidden value you try to protect actually is for each case.

This patch is basically not sensible. It just moves code around in a way that the compiler could have done anyway (or the compiler could decide to undo). It doesn't explain what the magic protected value is that shouldn't be leaked, and it leaves the code just looking odd and pointless.

Linus

Jakob

author

2022-02-23 14:13 UTC #10 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 15 lines · Linus Torvalds

Quoted 3 lines · Jakob Koschel
It is unsafe to assume that &req->req != _req can only evaluate to false if the break within the list iterator is hit.
I don't understand what problem you are trying to fix.
Since "req" absolutely *has* to be stable for any of this code to be valid, then "&req->req" is stable and unambiguous too. The *only* way _req can point to it would be if we finished the iteration properly.

Quoted 5 lines

So I don't see the unsafeness.
Note that all this work with "speculative" execution fundamentally CAN NOT affect semantics of the code, yet this patch makes statements about exactly that.

I'm sorry for having created the confusion. I made this patch to support the speculative safe list_for_each_entry() version but it is not actually related to that. I do believe that this an actual bug and *could* *potentially* be misused. I'll follow up with an example to illustrate that.

I agree that this has nothing to do with the speculative execution iterator (apart from making it work) and should best be discussed separately.

I'll attach an example on how I think this code *can* become a problem. Note that this highly depends on the used compiler and how the struct offsets are laid out.

Quoted 23 lines

That's not how CPU speculation works.
CPU speculation can expose hidden information that is not "semantically important" (typically through cache access patterns, but that's not the only way). So it might be exposing information it shouldn't.
But if speculation is actually changing semantics, then it's no longer "speculation" - it's just a bug, plain and simple (either a software bug due to insufficient serialization, or an actual hardware bug).
IOW, I don't want to see these kinds of apparently pointless changes to list walking. The patches should explain what that SECONDARY hidden value you try to protect actually is for each case.
This patch is basically not sensible. It just moves code around in a way that the compiler could have done anyway (or the compiler could decide to undo). It doesn't explain what the magic protected value is that shouldn't be leaked, and it leaves the code just looking odd and pointless.
Linus

Jakob

author

2022-02-23 14:16 UTC #11 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Note that I changed the location of the struct member 'req' in gr_request to make this work. Instead of reshuffling struct members this can also be introduced by simply adding new struct members in certain spots. In other code locations with the same pattern I didn't have to do that.

Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the heap, the check could pass even though the list searched is completely empty.

&req->req for the head element will be an out-of-bounds pointer that by coincidence or heap massaging is where '_req' is located.

Even if the list is empty the list_for_each_entry() macro will do:

	pos = list_first_entry(head, typeof(*pos), member);

resolving all the macros (list_first_entry() etc) it will look like this:

	pos = container_of(head->next, typeof(*pos), member)

Since the list is empty head->next == head and container_of() is called on something that is *not* actually of type gr_request.

Next, the check if the end of the loop is hit is evaluated:

	!list_entry_is_head(pos, head, member);

which will stop the loop directly before executing a single iteration.

then using '&req->req' is some bogus pointer pointing just past the struct gr_ep, where in this case '_req' is located.

The point I'm trying to make: it's probably not safe to rely on the compiler and that everyone is aware of this risk when adding/removing/reordering struct members.

---
drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++ drivers/usb/gadget/udc/gr_udc.h | 2 +-

2 files changed, 26 insertions(+), 1 deletion(-)

drivers/usb/gadget/udc/gr_udc.c +0 -0

index 4b35739d3695..29c662f28428 100644
@@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)

		ret = -EINVAL;
		goto out;
	}

+ printk(KERN_INFO "JKL: This does print, but shouldn't");

	if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
		/* This request is currently being processed */
@@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
	return ret;
}

+static int __init init_test_jkl3(void)
+{
+ struct gr_ep *ep;
+ struct gr_udc *dev;
+ struct usb_request *_req;
+ void *buffer;
+
+ /* assume the usb_request struct is located just after the 'ep' on the heap */
+ buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
+ ep = buffer;
+ _req = buffer+sizeof(struct gr_ep);
+
+ /* setup to call gr_dequeue() */
+ dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+ ep->dev = dev;
+ ep->dev->driver = 1;
+ INIT_LIST_HEAD(&ep->queue); /* list is empty */
+
+ gr_dequeue(&ep->ep, _req);
+
+ return 0;
+}
+__initcall(init_test_jkl3);
+
/* Helper for gr_set_halt and gr_set_wedge */
static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge) {

drivers/usb/gadget/udc/gr_udc.h +0 -0

index 70134239179e..14a18d5b5cf8 100644
@@ -159,7 +159,6 @@ struct gr_ep {

};

struct gr_request {

-	struct usb_request req;
	struct list_head queue;

	/* Chain of dma descriptors */

@@ -171,6 +170,7 @@ struct gr_request {
u16 oddlen; /* Size of odd length tail if buffer length is "odd" */

u8 setup; /* Setup packet */
+ struct usb_request req;
};

enum gr_ep0state {

Signature · 1 line

2.25.1

Greg Kroah-Hartman

maintainer

2022-02-24 10:33 UTC #12 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 107 lines · Jakob

Note that I changed the location of the struct member 'req' in gr_request to make this work. Instead of reshuffling struct members this can also be introduced by simply adding new struct members in certain spots. In other code locations with the same pattern I didn't have to do that.
Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the heap, the check could pass even though the list searched is completely empty.
&req->req for the head element will be an out-of-bounds pointer that by coincidence or heap massaging is where '_req' is located.
Even if the list is empty the list_for_each_entry() macro will do:
	pos = list_first_entry(head, typeof(*pos), member);
resolving all the macros (list_first_entry() etc) it will look like this:
	pos = container_of(head->next, typeof(*pos), member)
Since the list is empty head->next == head and container_of() is called on something that is *not* actually of type gr_request.
Next, the check if the end of the loop is hit is evaluated:
	!list_entry_is_head(pos, head, member);
which will stop the loop directly before executing a single iteration.
then using '&req->req' is some bogus pointer pointing just past the struct gr_ep, where in this case '_req' is located.
The point I'm trying to make: it's probably not safe to rely on the compiler and that everyone is aware of this risk when adding/removing/reordering struct members.
Signed-off-by Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++ drivers/usb/gadget/udc/gr_udc.h | 2 +-
2 files changed, 26 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/gr_udc.c +25 -0
index 4b35739d3695..29c662f28428 100644
@@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 		ret = -EINVAL;
 		goto out;
 	}
+	printk(KERN_INFO "JKL: This does print, but shouldn't");
 
 	if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
 		/* This request is currently being processed */
@@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	return ret;
 }
 
+static int __init init_test_jkl3(void)
+{
+	struct gr_ep *ep;
+	struct gr_udc *dev;
+	struct usb_request *_req;
+	void *buffer;
+
+	/* assume the usb_request struct is located just after the 'ep' on the heap */
+	buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
+	ep = buffer;
+	_req = buffer+sizeof(struct gr_ep);
+
+	/* setup to call gr_dequeue() */
+	dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+	ep->dev = dev;
+	ep->dev->driver = 1;
+	INIT_LIST_HEAD(&ep->queue); /* list is empty */
+
+	gr_dequeue(&ep->ep, _req);
+
+	return 0;
+}
+__initcall(init_test_jkl3);
+
 /* Helper for gr_set_halt and gr_set_wedge */
 static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
 {
drivers/usb/gadget/udc/gr_udc.h +1 -1
index 70134239179e..14a18d5b5cf8 100644
@@ -159,7 +159,6 @@ struct gr_ep {
 };
 
 struct gr_request {
-	struct usb_request req;
 	struct list_head queue;
 
 	/* Chain of dma descriptors */
@@ -171,6 +170,7 @@ struct gr_request {
 	u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
 
 	u8 setup; /* Setup packet */
+	struct usb_request req;
 };
 
 enum gr_ep0state {
Signature · 1 line
2.25.1

So, to follow the proposed solution in this thread, the following change is the "correct" one to make, right?

drivers/usb/gadget/udc/gr_udc.c +7 -4

index 4b35739d3695..5d65d8ad5281 100644
@@ -1690,7 +1690,8 @@ static int gr_queue_ext(struct usb_ep *_ep, struct usb_request *_req,
 /* Dequeue JUST ONE request */
 static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
-	struct gr_request *req;
+	struct gr_request *req = NULL;
+	struct gr_request *tmp;
 	struct gr_ep *ep;
 	struct gr_udc *dev;
 	int ret = 0;

@@ -1710,11 +1711,13 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	spin_lock_irqsave(&dev->lock, flags);
 
 	/* Make sure it's actually queued on this endpoint */
-	list_for_each_entry(req, &ep->queue, queue) {
-		if (&req->req == _req)
+	list_for_each_entry(tmp, &ep->queue, queue) {
+		if (&tmp->req == _req) {
+			req = tmp;
 			break;
+		}
 	}
-	if (&req->req != _req) {
+	if (!req) {
 		ret = -EINVAL;
 		goto out;
 	}

Linus Torvalds

2022-02-24 17:56 UTC #13 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 3 lines · Greg Kroah-Hartman

So, to follow the proposed solution in this thread, the following change is the "correct" one to make, right?

That would indeed be my preference.

If we ever are able to change

        list_for_each_entry(tmp, &ep->queue, queue)...

to simply declare 'tmp' as the right type itself, then the e need to do

        struct gr_request *tmp;

outside the loop goes away, and this kind of "set a variable that is declared in the outside context" becomes the only way to do things.

Linus

A message that is not in the archive (sent off-list or not yet received): [email protected]

Cristiano Giuffrida

2022-02-24 11:02 UTC #15 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

I think the "problem" with this solution is that it doesn't prevent tmp from being used outside the loop still (and people getting it wrong again)? It would be good to have 'struct gr_request *tmp;' being visible only inside the loop (i.e., declared in the macro).

Quoted 145 lines · Greg Kroah-Hartman

Quoted 106 lines · Jakob

Note that I changed the location of the struct member 'req' in gr_request to make this work. Instead of reshuffling struct members this can also be introduced by simply adding new struct members in certain spots. In other code locations with the same pattern I didn't have to do that.
Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the heap, the check could pass even though the list searched is completely empty.
&req->req for the head element will be an out-of-bounds pointer that by coincidence or heap massaging is where '_req' is located.
Even if the list is empty the list_for_each_entry() macro will do:
      pos = list_first_entry(head, typeof(*pos), member);
resolving all the macros (list_first_entry() etc) it will look like this:
      pos = container_of(head->next, typeof(*pos), member)
Since the list is empty head->next == head and container_of() is called on something that is *not* actually of type gr_request.
Next, the check if the end of the loop is hit is evaluated:
      !list_entry_is_head(pos, head, member);
which will stop the loop directly before executing a single iteration.
then using '&req->req' is some bogus pointer pointing just past the struct gr_ep, where in this case '_req' is located.
The point I'm trying to make: it's probably not safe to rely on the compiler and that everyone is aware of this risk when adding/removing/reordering struct members.
Signed-off-by Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++ drivers/usb/gadget/udc/gr_udc.h | 2 +-
2 files changed, 26 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/gr_udc.c +25 -0
index 4b35739d3695..29c662f28428 100644
@@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
              ret = -EINVAL;
              goto out;
      }
+     printk(KERN_INFO "JKL: This does print, but shouldn't");
 
      if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
              /* This request is currently being processed */
@@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
      return ret;
 }
 
+static int __init init_test_jkl3(void)
+{
+     struct gr_ep *ep;
+     struct gr_udc *dev;
+     struct usb_request *_req;
+     void *buffer;
+
+     /* assume the usb_request struct is located just after the 'ep' on the heap */
+     buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
+     ep = buffer;
+     _req = buffer+sizeof(struct gr_ep);
+
+     /* setup to call gr_dequeue() */
+     dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+     ep->dev = dev;
+     ep->dev->driver = 1;
+     INIT_LIST_HEAD(&ep->queue); /* list is empty */
+
+     gr_dequeue(&ep->ep, _req);
+
+     return 0;
+}
+__initcall(init_test_jkl3);
+
 /* Helper for gr_set_halt and gr_set_wedge */
 static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
 {
drivers/usb/gadget/udc/gr_udc.h +1 -2
index 70134239179e..14a18d5b5cf8 100644
@@ -159,7 +159,6 @@ struct gr_ep {
 };
 
 struct gr_request {
-     struct usb_request req;
      struct list_head queue;
 
      /* Chain of dma descriptors */
@@ -171,6 +170,7 @@ struct gr_request {
      u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
 
      u8 setup; /* Setup packet */
+     struct usb_request req;
 };
 
 enum gr_ep0state {
--

Quoted 1 line

2.25.1

So, to follow the proposed solution in this thread, the following change is the "correct" one to make, right?

drivers/usb/gadget/udc/gr_udc.c +7 -4

index 4b35739d3695..5d65d8ad5281 100644
@@ -1690,7 +1690,8 @@ static int gr_queue_ext(struct usb_ep *_ep, struct usb_request *_req,
 /* Dequeue JUST ONE request */
 static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
-       struct gr_request *req;
+       struct gr_request *req = NULL;
+       struct gr_request *tmp;
        struct gr_ep *ep;
        struct gr_udc *dev;
        int ret = 0;

@@ -1710,11 +1711,13 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
        spin_lock_irqsave(&dev->lock, flags);
 
        /* Make sure it's actually queued on this endpoint */
-       list_for_each_entry(req, &ep->queue, queue) {
-               if (&req->req == _req)
+       list_for_each_entry(tmp, &ep->queue, queue) {
+               if (&tmp->req == _req) {
+                       req = tmp;
                        break;
+               }
        }
-       if (&req->req != _req) {
+       if (!req) {
                ret = -EINVAL;
                goto out;
        }

Greg Kroah-Hartman

maintainer

2022-02-24 11:26 UTC #16 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 4 lines · Cristiano Giuffrida

I think the "problem" with this solution is that it doesn't prevent tmp from being used outside the loop still (and people getting it wrong again)? It would be good to have 'struct gr_request *tmp;' being visible only inside the loop (i.e., declared in the macro).

That is a larger change, one that we can hopefully make when changing to introduce the temp variable in the loop_for_each() macro as Linus described elsewhere in the thread.

But for now, it should be pretty obvious to not touch tmp again after the loop is done. That should make it easier to check for stuff like this as well in an automated fashion (i.e. the loop variable is only touched inside the loop.)

thanks,

greg k-h

Linus Torvalds

2022-02-23 18:47 UTC #17 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

In reply to Jakob

[ Arnd was already on the participants, but I moved him from 'Cc:' to 'To:', just because I think this is once again tangentially related to the whole "c99 base" thing ]

Quoted 5 lines · Jakob

I'm sorry for having created the confusion. I made this patch to support the speculative safe list_for_each_entry() version but it is not actually related to that. I do believe that this an actual bug and *could* *potentially* be misused. I'll follow up with an example to illustrate that.

Ok, so this is just a regular bug, plain and simple.

The problem being that the list_for_each_entry() will iterate over each list entry - but at the end of the loop it will not point at any entry at all (it will have a pointer value that is related to the *HEAD* of the list, but that is not necessarily the same kind of entry that the list members are.

Honestly, I think this kind of fix should have been done entirely separately.

In fact, I think the change to list_for_each_entry() should have been done not as "fix type speculation", but as a much more interesting "fix the list iterators".

The whole reason this kind of non-speculative bug can happen is that we historically didn't have C99-style "declare variables in loops". So list_for_each_entry() - and all the other ones - fundamentally always leaks the last HEAD entry out of the loop, simply because we couldn't declare the iterator variable in the loop itself.

(And by "couldn't", I mean "without making for special syntax": we do exactly that in "for_each_thread ()" and friends, but they have an "end_for_each_thread()" thing at the end).

So what I'd personally *really* like to see would be for us to - once again - look at using "-std=gnu99", and fix the whole "leak final invalid pointer outside the loop".

Then the type speculation thing would be an entirely separate patch.

Because honestly, I kind of hate the completely random type speculation patch. It fixes one particular type of loop, and not even one that seems all that special.

But we still don't do "gnu99", because we had some odd problem with some ancient gcc versions that broke documented initializers.

I honestly _thought_ we had gotten over that already. I think the problem cases were gcc-4.9 and older, and now we require gcc-5.1, and we could just use "--std=gnu99" for the kernel, and we could finally start using variable declarations in for-statements.

Arnd - remind me, please.. Was there some other problem than just gcc-4.9?

Linus

Linus Torvalds

2022-02-23 19:24 UTC #18 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 2 lines · Linus Torvalds

Arnd - remind me, please.. Was there some other problem than just gcc-4.9?

Hmm. Interesting. I decided to just try it and see for the compiler I have, and changing the gnu89 to gnu99 I get new warnings (-Werror=shift-negative-value).

Very annoying. Especially since negative values are in many contexts actually *safer* than positive ones when used as a mask, because as long as the top bit is set in 'int', if the end result is then expanded to some wider type, the top bit stays set.

Example:

unsigned long mask(unsigned long x)
{ return x & (~0 << 5); }

unsigned long badmask(unsigned long x)
{ return x & (~0u << 5); }

One does it properly, the other is buggy.

Now, with an explicit "unsigned long" like this, some clueless compiler person might say "just use "~0ul", but that's completely wrong - because quite often the type is *not* this visible, and the signed version works *regardless* of type.

So this Werror=shift-negative-value warning seems to be actively detrimental, and I'm not seeing the reason for it. Can somebody explain the thinking for that stupid warning?

That said, we seem to only have two cases of it in the kernel, at least by a x86-64 allmodconfig build. So we could examine the types there, or we could just add '-Wno-shift-negative-value".

Linus

Linus Torvalds

2022-02-23 19:43 UTC #19 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 3 lines · Linus Torvalds

That said, we seem to only have two cases of it in the kernel, at least by a x86-64 allmodconfig build.

No, there's more of them, it's just that the build broke early enough that I didn't see it.

Doing

        git grep '\(\(~0\)\|\(-1\)\) <<'

finds a number of them. Some of them have casts in front, so they wouldn't necessarily trigger this issue, but it's not an entirely uncommon pattern.

And as mentioned, I think it's a *good* pattern, in that it takes advantage of the sign-extension of the top bit in any widening use, when the type might not be obvious (in macros, or when accessing members of unions or structures, or when using typedefs that hide the actual type).

So I still think that warning is actively detrimental, and I'm wondering why it was added (and why 'gnu99' enables it, but 'gnu89' does not). There's presumably _some_ reason.

Linus

Arnd Bergmann

maintainer

2022-02-23 20:25 UTC #20 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 5 lines · Linus Torvalds

Quoted 2 lines · Linus Torvalds
Arnd - remind me, please.. Was there some other problem than just gcc-4.9?

I'm pretty sure this was the only thing. And not even because gcc-4.9 didn't support later standards, but because it caused some false-postivee warnings like

In file included from ../drivers/usb/core/notify.c:16:0:
../include/linux/notifier.h:117:9: error: initializer element is not constant

  struct blocking_notifier_head name =   \
         ^
../drivers/usb/core/notify.c:21:8: note: in expansion of macro
'BLOCKING_NOTIFIER_HEAD'
 static BLOCKING_NOTIFIER_HEAD(usb_notifier_list);
        ^
../include/linux/notifier.h:117:9: error: (near initialization for
'usb_notifier_list.rwsem.wait_lock')
  struct blocking_notifier_head name =   \
         ^
../drivers/usb/core/notify.c:21:8: note: in expansion of macro
'BLOCKING_NOTIFIER_HEAD'
 static BLOCKING_NOTIFIER_HEAD(usb_notifier_list);
        ^

Quoted 3 lines

Hmm. Interesting. I decided to just try it and see for the compiler I have, and changing the gnu89 to gnu99 I get new warnings (-Werror=shift-negative-value).

FWIW, I think we can go straight to gnu11 in this case even with gcc-5, no need to take gnu99 as an intermediate step. I don't know if there is a practical difference between the two though.

gcc-8 and higher also support --std=gnu18 and --std=gnu2x, which also work for building the kernel but would break gcc-5/6/7 support

Quoted 20 lines

Very annoying. Especially since negative values are in many contexts actually *safer* than positive ones when used as a mask, because as long as the top bit is set in 'int', if the end result is then expanded to some wider type, the top bit stays set.
Example:
unsigned long mask(unsigned long x)
{ return x & (~0 << 5); }
unsigned long badmask(unsigned long x)
{ return x & (~0u << 5); }
One does it properly, the other is buggy.
Now, with an explicit "unsigned long" like this, some clueless compiler person might say "just use "~0ul", but that's completely wrong - because quite often the type is *not* this visible, and the signed version works *regardless* of type.

Quoted 7 lines

So this Werror=shift-negative-value warning seems to be actively detrimental, and I'm not seeing the reason for it. Can somebody explain the thinking for that stupid warning?
That said, we seem to only have two cases of it in the kernel, at least by a x86-64 allmodconfig build. So we could examine the types there, or we could just add '-Wno-shift-negative-value".

I looked at the gcc documentation for this flag, and it tells me that it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89 shows the same warning, so at least it doesn't sound like the actual behavior changed, only the warning output. clang does not warn for this code at all, regardless of the --std= flag.

Arnd

Linus Torvalds

2022-02-23 20:43 UTC #21 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 6 lines · Arnd Bergmann

I looked at the gcc documentation for this flag, and it tells me that it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89 shows the same warning, so at least it doesn't sound like the actual behavior changed, only the warning output. clang does not warn for this code at all, regardless of the --std= flag.

Ok, so we should be able to basically convert '--std=gnu89' into '--std=gnu11 -Wno-shift-negative-value' with no expected change of behavior.

Of course, maybe we need to make -Wno-shift-negative-value be conditional on the compiler supporting it in the first place?

I really would love to finally move forward on this, considering that it's been brewing for many many years.

I think the loop iterators are the biggest user-visible thing, but there might be others.

And some googling seems to show that the reason for -Wshift-negative-value is that with C99 the semantics changed for targets that weren't two's complement. We *really* don't care.

Of course, the C standard being the bunch of incompetents they are, they in the process apparently made left-shifts undefined (rather than implementation-defined). Christ, they keep on making the same mistakes over and over. What was the definition of insanity again?

Linus

Linus Torvalds

2022-02-23 20:55 UTC #22 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 5 lines · Linus Torvalds

Of course, the C standard being the bunch of incompetents they are, they in the process apparently made left-shifts undefined (rather than implementation-defined). Christ, they keep on making the same mistakes over and over. What was the definition of insanity again?

Hey, some more googling on my part seems to say that somebody saw the light, and it's likely getting fixed in newer C standard version.

So it was just a mistake, not actual malice. Maybe we can hope that the tide is turning against the "undefined" crowd that used to rule the roost in the C standards bodies. Maybe the fundamental security issues with undefined behavior finally convinced people how bad it was?

Linus

David Laight

2022-02-23 22:22 UTC #23 lore raw

RE: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

From: Linus Torvalds

Quoted 10 lines

Sent: 23 February 2022 20:55
Quoted 5 lines · Linus Torvalds
Of course, the C standard being the bunch of incompetents they are, they in the process apparently made left-shifts undefined (rather than implementation-defined). Christ, they keep on making the same mistakes over and over. What was the definition of insanity again?

Quoted 8 lines

Hey, some more googling on my part seems to say that somebody saw the light, and it's likely getting fixed in newer C standard version.
So it was just a mistake, not actual malice. Maybe we can hope that the tide is turning against the "undefined" crowd that used to rule the roost in the C standards bodies. Maybe the fundamental security issues with undefined behavior finally convinced people how bad it was?

IIRC UB includes 'fire an ICBM at the writer of the standards document'. There isn't an 'undefined value' or even 'undefined value or program trap' option.

It also seems to me that the compiler people are picking on things in the standard that are there to let 'obscure machines conform' and then using them to break perfectly reasonable programs.

Signed arithmetic is not required to wrap so that cpu (eg DSP) can do saturating maths - not so the compiler can remove some conditionals.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Uecker, Martin

2022-02-25 21:39 UTC #24 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Am Mittwoch, den 23.02.2022, 20:54 +0000 schrieb Linus Torvalds:

Quoted 6 lines

Quoted 4 lines · Linus Torvalds
Of course, the C standard being the bunch of incompetents they are, they in the process apparently made left-shifts undefined (rather than implementation-defined). Christ, they keep on making the same mistakes over and over. What was the definition of insanity again?

Implementation-defined only means that it needs to be documented (and clang does not even do this), so I am not sure what difference this would make.

Quoted 2 lines

Hey, some more googling on my part seems to say that somebody saw the light, and it's likely getting fixed in newer C standard version.

I don't think it is changed. But C23 will require integers to be repreeted using two's complement, so there is a better chance to fix things like this in the future.

Quoted 5 lines

So it was just a mistake, not actual malice. Maybe we can hope that the tide is turning against the "undefined" crowd that used to rule the roost in the C standards bodies. Maybe the fundamental security issues with undefined behavior finally convinced people how bad it was?

The right people to complain to are the compiler vendors, because they decide what UB does in their implementation. In the standard body the same people argue that the standard has to codify existing practice. Even in cases where the standard defines behavior, compilers sometimes simply ignore this (e.g. pointer comparison or pointer-to-integer round trips). So the power is really with the compiler writers.

Martin

Linus Torvalds

2022-02-25 22:03 UTC #25 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 4 lines · Martin

Implementation-defined only means that it needs to be documented (and clang does not even do this), so I am not sure what difference this would make.

No, implementation-defined means something *much* more than "undefined".

Yes, it means that the behavior has to be documented.

But that's not actually the big deal from a user standpoint (although it might be a big annoyance from a compiler writer's standpoint - compiler writers would probably much prefer "let me do the obvious sane thing" over "let me do the obvious sane thing and then have to write documentation about how obviously sane it is").

From a user standpoint, the big thing is that "implementation-defined" means that the behavior has to have *SOME* well-defined behavior. Documentation is irrelevant. But RELIABILITY is relevant.

See?

That's a big big deal. The documentation is almost incidental - the important part is that the code acts the same on the same architecture, regardless of compiler options, and regardless of the phase of the moon. When you upgrade your compiler to a new version, the end result doesn't suddenly change.

In contrast, "undefined" means that the same C code can do two totally different things with different optimization options, or based on just any random thing - like some random register allocation issue.

So "undefined" behavior means that changes to code that isn't even near the code in question can change what the code generation for that code is. And the compiler may not even report it.

That is a complete disaster. It's a disaster from a security standpoint, it's a disaster from a maintenance standpoint, it's just *bad*.

And the C standards committee has traditionally used to think it was a good thing, because they listened to compiler writers that claimed that it allowed them to do wild optimizations and improve code generation.

Example: the completely broken type-based aliasing rules that changed semantics of C for the worse.

Reality: it doesn't actually improve code generation all that much. Instead it just results in a lot of problems, and any sane C code base that cares about security and stability will just turn that off.

Same goes for integer overflow etc.

The only really valid use-case for "undefined" is when you're doing things like accessing past your own allocations. The compiler can't do much about things like that.

But the C standards body has actually actively screwed the pooch over the years, and turned perfectly traditional code into "undefined" code for no good reason. "-1 << 1" is just one particularly simplistic example.

Quoted 7 lines

Quoted 2 lines
Hey, some more googling on my part seems to say that somebody saw the light, and it's likely getting fixed in newer C standard version.
I don't think it is changed. But C23 will require integers to be repreeted using two's complement, so there is a better chance to fix things like this in the future.

If integers are guaranteed to be two's complement, then the only possible explanation for "left shift is undefined" goes away. So presumably the thing goes hand-in-hand with just making (the de-facto) negative shifting well-defined.

(Btw, shifting *by* a negative number - or by amounts bigger than the width of the type - is different. There are at least real reasons to complain about that, although I think "implementation defined" would still be hugely preferred over "undefined")

Quoted 3 lines

The right people to complain to are the compiler vendors, because they decide what
UB does in their implementation.

No. The bug was in the spec to begin with. The people to complain about are the incompetents that literally changed the C standard to be less well-specified.

As far as I know, no actual compiler has ever made integer left-shift done anything bad. You'd literally have to do extra work for it, so there's no reason to do so.

But because the standards committee messed up, the compiler writers *did* do the extra work to warn about the fact (for the trivial "negative constant" case - never mind that technically it's undefined for non-constants: that would have been too much work).

End result: the warning is stupid. The warning only exists for artificially stupid reasons.

And those reasons are literally "broken C standards committee behavior".

Linus

Martin Uecker

2022-02-26 01:30 UTC #26 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Am Freitag, den 25.02.2022, 14:02 -0800 schrieb Linus Torvalds:

Quoted 16 lines

Quoted 3 lines · Martin
Implementation-defined only means that it needs to be documented (and clang does not even do this), so I am not sure what difference this would make.
No, implementation-defined means something *much* more than "undefined".
Yes, it means that the behavior has to be documented.
But that's not actually the big deal from a user standpoint (although it might be a big annoyance from a compiler writer's standpoint - compiler writers would probably much prefer "let me do the obvious sane thing" over "let me do the obvious sane thing and then have to write documentation about how obviously sane it is").

Quoted 5 lines

From a user standpoint, the big thing is that "implementation-defined" means that the behavior has to have *SOME* well-defined behavior. Documentation is irrelevant. But RELIABILITY is relevant.
See?

In principle, a compiler writer could document implementation-defined behavior to do something crazy and promise that something the standard leaves undefined has useful and consistent behavior.

But I think you are right that this still makes a huge difference in practice because the two notions encourage different things.

...

Quoted 7 lines

In contrast, "undefined" means that the same C code can do two totally different things with different optimization options, or based on just any random thing - like some random register allocation issue.
So "undefined" behavior means that changes to code that isn't even near the code in question can change what the code generation for that code is. And the compiler may not even report it.

Yes, it means the compiler is free to do this.

But it does not mean a compiler *has* to do this.

Compiler writers represented in the committee want to have the UB because some customers seem to like those optimizations. At the same time, a programmer who complains about some undesired effect of such an UB-based optimization is typically told that the program has UB according to the C standard so nothing can be done (bug report closed).

You see what is happening here?

Roughly the same group of people / companies that write the compilers also control what goes into the standard. They then like to point to the standard and reject all responsibility for *their* implementation choices. By complaining about the standard, one detracts from the fact that the compiler writers make these choices and also largely control what goes in the standard.

Quoted 3 lines

That is a complete disaster. It's a disaster from a security standpoint, it's a disaster from a maintenance standpoint, it's just *bad*.

It is sometimes pure necessity. An unbounded write has unbounded consequences and there is no realistic way to give it defined behavior (in the context of C). But for many other cases of UB, I completely agree with you.

Quoted 4 lines

And the C standards committee has traditionally used to think it was a good thing, because they listened to compiler writers that claimed that it allowed them to do wild optimizations and improve code generation.

There are many compiler people in the standards committee, so there is no surprise here.

Quoted 2 lines

Example: the completely broken type-based aliasing rules that changed semantics of C for the worse.

I do not really know the history of this, but
I think these rules existed even before compilers really exploited them for optimization.

Quoted 5 lines

Reality: it doesn't actually improve code generation all that much. Instead it just results in a lot of problems, and any sane C code base that cares about security and stability will just turn that off.
Same goes for integer overflow etc.

For signed overflow, I am not entirely sure what the right choice is. Wrapping for signed overflow also seems dangerous. I use UBsan to find such issues in my code, and this would not really work if signed overflow was defined to wrap.

...

Quoted 8 lines

Quoted 7 lines
Quoted 2 lines
Hey, some more googling on my part seems to say that somebody saw the light, and it's likely getting fixed in newer C standard version.
I don't think it is changed. But C23 will require integers to be repreeted using two's complement, so there is a better chance to fix things like this in the future.

Quoted 4 lines

If integers are guaranteed to be two's complement, then the only possible explanation for "left shift is undefined" goes away. So presumably the thing goes hand-in-hand with just making (the de-facto) negative shifting well-defined.

I think there is a fairy good chance to get this changed.

Quoted 9 lines

(Btw, shifting *by* a negative number - or by amounts bigger than the width of the type - is different. There are at least real reasons to complain about that, although I think "implementation defined" would still be hugely preferred over "undefined")
Quoted 3 lines
The right people to complain to are the compiler vendors, because they decide what
UB does in their implementation.

Quoted 7 lines

No. The bug was in the spec to begin with. The people to complain about are the incompetents that literally changed the C standard to be less well-specified.
As far as I know, no actual compiler has ever made integer left-shift done anything bad. You'd literally have to do extra work for it, so there's no reason to do so.

I agree that the standard should be changed. But this example also shows that UB in the standard does not inevitably causes the compilers to do something stupid. It is still their choice.

Quoted 4 lines

But because the standards committee messed up, the compiler writers *did* do the extra work to warn about the fact (for the trivial "negative constant" case - never mind that technically it's undefined for non-constants: that would have been too much work).

Compilers do *not* warn about a lot of undefined behavior (even where it would be easily possible and make perfect sense). So why here?

Martin

Miguel Ojeda

2022-02-27 18:12 UTC #27 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 4 lines · Martin Uecker

Roughly the same group of people / companies that write the compilers also control what goes into the standard. They then like to point to the standard

Indeed, at least to a substantial degree.

Quoted 5 lines

For signed overflow, I am not entirely sure what the right choice is. Wrapping for signed overflow also seems dangerous. I use UBsan to find such issues in my code, and this would not really work if signed overflow was defined to wrap.

UBsan and similar tooling may still be used to find whatever behavior one wants, whether defined or not. UBSan already has non-UB checks.

Cheers,
Miguel

Martin Uecker

2022-02-28 07:08 UTC #28 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Am Sonntag, den 27.02.2022, 19:12 +0100 schrieb Miguel Ojeda:

Quoted 7 lines · Martin Uecker

Quoted 3 lines
Roughly the same group of people / companies that write the compilers also control what goes into the standard. They then like to point to the standard
Indeed, at least to a substantial degree.

Quoted 8 lines

Quoted 5 lines
For signed overflow, I am not entirely sure what the right choice is. Wrapping for signed overflow also seems dangerous. I use UBsan to find such issues in my code, and this would not really work if signed overflow was defined to wrap.
UBsan and similar tooling may still be used to find whatever behavior one wants, whether defined or not. UBSan already has non-UB checks.

Technically, this is true but not really in practice. If signed overflow would be defined to wrap, then code would start to rely on it and detecting it becomes useless because there are too many false positives. In your own small controlled code base it could work though.

Martin

Miguel Ojeda

2022-02-28 13:50 UTC #29 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 6 lines · Martin Uecker

Technically, this is true but not really in practice. If signed overflow would be defined to wrap, then code would start to rely on it and detecting it becomes useless because there are too many false positives. In your own small controlled code base it could work though.

Either code is written with signed overflow in mind, or not. That is what actually matters for detection, not whether it is UB in the standard or not.

If a project starts relying on overflow wrapping, then they are taking the same stance as projects that already rely on -fwrapv. That is their choice, same as today.

But making it non-UB in the standard does not force a project to consider it "not an error", which is what actually matters for being able to use UBSan effectively or not.

In fact, if you are just concerned about detectability or -fwrapv being the wrong default, there is still a way out without keeping it
UB: we could consider it an error (thus people is not encouraged to rely on it), yet not UB. That is what Rust does, and why I suggested the past exploring the move of some existing UB in C into an "erroneous behavior, yet defined" area.

And, for the cherry on top, if users had a way to write exactly what they mean (per operation or per type), then we can flag exactly the cases that are not intentional, and users can still use unchecked operations for performance sensitive code, etc.

Cheers,
Miguel

Linus Torvalds

2022-03-01 20:30 UTC #30 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 4 lines · Miguel Ojeda

But making it non-UB in the standard does not force a project to consider it "not an error", which is what actually matters for being able to use UBSan effectively or not.

Absolutely.

I think people should treat UBsan and friends a bit like "runtime lint".

"lint" traditionally doesn't necessarily check for just *incorrect* C.

It checks for things that can be confusing to humans, even if they are 100% completely conforming standard C.

Classic example: indentation. Having the wrong indentation is not in any shape of form "undefined behavior" from a C standpoint, but it sure is something that makes sense checking for anyway.

I think "integer overflow" should be considered the exact same thing. It should *not* be treated as "undefined behavior", and it should not give the compiler the option to generate code that doesn't match what the programmer wrote.

But having a checking tool that says "This looks wrong - you just had an integer overflow"? THAT makes 100% sense.

The C standard rules "undefined behavior" really is a problem in the standard.

Linus

Martin Uecker

2022-03-02 07:28 UTC #31 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Am Dienstag, den 01.03.2022, 12:26 -0800 schrieb Linus Torvalds:

Quoted 10 lines

Quoted 3 lines · Miguel Ojeda
But making it non-UB in the standard does not force a project to consider it "not an error", which is what actually matters for being able to use UBSan effectively or not.
Absolutely.
I think people should treat UBsan and friends a bit like "runtime lint".

Quoted 8 lines

"lint" traditionally doesn't necessarily check for just *incorrect* C.
It checks for things that can be confusing to humans, even if they are 100% completely conforming standard C.
Classic example: indentation. Having the wrong indentation is not in any shape of form "undefined behavior" from a C standpoint, but it sure is something that makes sense checking for anyway.

You can automatically re-indent code form other sources without breaking it. Assume you have code that relis on signed integer wrapping, but you want to use UBSan to screen for possible signed arithmetic errors and/or have it trap in production to protect against exploits. You would then have to carefully analyze each individual case of signed arithmetic whether it makes sense, and then somehow add an annotation that it is actually ok (or rewrite it which introduces new risks). This does not seem comparable to indentation at all.

On the other hand, if you have a self-contained code base and like wrapping signed integer, you can now use a compiler flag and also get what you want.

So I am still not yet convinced that the standard was wrong making it undefined.

Whether it is wise for compilers to use it aggressively for optimization is a different question...

Martin

Arnd Bergmann

maintainer

2022-02-23 21:52 UTC #32 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

https://github.com/llvm/llvm-project/commit/59802321785b4b9fc31b10456c62ba3a06d3a631

Quoted 9 lines · Linus Torvalds

Quoted 6 lines · Arnd Bergmann
I looked at the gcc documentation for this flag, and it tells me that it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89 shows the same warning, so at least it doesn't sound like the actual behavior changed, only the warning output. clang does not warn for this code at all, regardless of the --std= flag.

Quoted 3 lines

Ok, so we should be able to basically convert '--std=gnu89' into '--std=gnu11 -Wno-shift-negative-value' with no expected change of behavior.

Yes, I think that is correct.

Quoted 2 lines

Of course, maybe we need to make -Wno-shift-negative-value be conditional on the compiler supporting it in the first place?

I think they all do. I discussed this with Nathan Chancellor on IRC, to see what clang does, and he points out that the warning was made conditional on -fwrapv there a while ago:

So the normal behavior on clang is to always warn about it, but since we explicitly ask for sane signed integer behavior, it doesn't warn for the kernel.

Arnd

Linus Torvalds

2022-02-23 21:54 UTC #33 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines
Ok, so we should be able to basically convert '--std=gnu89' into '--std=gnu11 -Wno-shift-negative-value' with no expected change of behavior.
Yes, I think that is correct.

Ok, somebody please remind me, and let's just try this early in the 5.18 merge window.

Because at least for me, doing

- -std=gnu89
+ -std=gnu11 -Wno-shift-negative-value

for KBUILD_CFLAGS works fine both in my gcc and clang builds. But that's obviously just one version of each.

(I left the host compiler flags alone - I have this memory of us having had issues with people having old host compilers and wanting headers for tools still build with gcc-4)

Linus

Nathan Chancellor

2022-02-24 16:04 UTC #34 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 16 lines · Linus Torvalds

Quoted 6 lines · Arnd Bergmann
Quoted 4 lines
Ok, so we should be able to basically convert '--std=gnu89' into '--std=gnu11 -Wno-shift-negative-value' with no expected change of behavior.
Yes, I think that is correct.
Ok, somebody please remind me, and let's just try this early in the 5.18 merge window.
Because at least for me, doing
(quoted diff) +1 -1
-                  -std=gnu89
+                  -std=gnu11 -Wno-shift-negative-value

Quoted 2 lines

for KBUILD_CFLAGS works fine both in my gcc and clang builds. But that's obviously just one version of each.

I ran that diff through my set of clang builds on v5.17-rc5-21-g23d04328444a and only found one issue:

https://github.com/ClangBuiltLinux/linux/issues/1603

I think that should be fixed on the clang side. Once it is, I think we could just disable that warning in those translation units for older versions of clang to keep the status quo.

Cheers,
Nathan

Segher Boessenkool

2022-02-26 12:48 UTC #35 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

In reply to Arnd Bergmann

Quoted 16 lines · Linus Torvalds

Quoted 2 lines · Linus Torvalds
Arnd - remind me, please.. Was there some other problem than just gcc-4.9?
Hmm. Interesting. I decided to just try it and see for the compiler I have, and changing the gnu89 to gnu99 I get new warnings (-Werror=shift-negative-value).
Very annoying. Especially since negative values are in many contexts actually *safer* than positive ones when used as a mask, because as long as the top bit is set in 'int', if the end result is then expanded to some wider type, the top bit stays set.
Example:

Quoted 7 lines

unsigned long mask(unsigned long x)
{ return x & (~0 << 5); }
unsigned long badmask(unsigned long x)
{ return x & (~0u << 5); }
One does it properly, the other is buggy.

You won't get this confusion if you write -1 instead. Not that that helps your problem here.

The GCC documentation says (at
<https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html>):

- The results of some bitwise operations on signed integers (C90 6.3, C99 and C11 6.5).

Bitwise operators act on the representation of the value including both the sign and value bits, where the sign bit is considered immediately above the highest-value value bit. Signed ‘>>’ acts on negative numbers by sign extension.

As an extension to the C language, GCC does not use the latitude given in C99 and C11 only to treat certain aspects of signed ‘<<’ as undefined. However, -fsanitize=shift (and -fsanitize=undefined) will diagnose such cases. They are also diagnosed where constant expressions are required.

But in fact they are diagnosed more often:
===
unsigned int n;
int f0(int x) { return x & (~0 << 5); }
int f1(int x) { return x & (-1 << 5); }
int f2(int x) { return x & (~0 << n); }
int f3(int x) { return x & (-1 << n); }
===
gets a warning on every line:
===
shifty.c: In function 'f0':
shifty.c:2:32: warning: left shift of negative value [-Wshift-negative-value]

    2 | int f0(int x) { return x & (~0 << 5); }
      |                                ^~
shifty.c: In function 'f1':
shifty.c:3:32: warning: left shift of negative value [-Wshift-negative-value]
    3 | int f1(int x) { return x & (-1 << 5); }
      |                                ^~
shifty.c: In function 'f2':
shifty.c:4:32: warning: left shift of negative value [-Wshift-negative-value]
    4 | int f2(int x) { return x & (~0 << n); }
      |                                ^~
shifty.c: In function 'f3':
shifty.c:5:32: warning: left shift of negative value [-Wshift-negative-value]
    5 | int f3(int x) { return x & (-1 << n); }
      |                                ^~

===
(with -O2 -Wall -W, nothing more). No constant expression is required in any of those cases, and in f2 and f3 the shift is not a constant expression even.

Quoted 3 lines

So this Werror=shift-negative-value warning seems to be actively detrimental, and I'm not seeing the reason for it. Can somebody explain the thinking for that stupid warning?

-Werror=*anything* is detrimental, *always*. Warnings are warnings and errors are errors. Warnings have false positives: if not, they should be errors instead!

Quoted 3 lines

That said, we seem to only have two cases of it in the kernel, at least by a x86-64 allmodconfig build. So we could examine the types there, or we could just add '-Wno-shift-negative-value".

Yes.

The only reason the warning exists is because it is undefined behaviour (not implementation-defined or anything). The reason it is that in the standard is that it is hard to implement and even describe for machines that are not two's complement. However relevant that is today :-)

Segher

Arnd Bergmann

maintainer

2022-02-26 22:14 UTC #36 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 6 lines · Segher Boessenkool

Quoted 3 lines · Linus Torvalds
That said, we seem to only have two cases of it in the kernel, at least by a x86-64 allmodconfig build. So we could examine the types there, or we could just add '-Wno-shift-negative-value".

Quoted 6 lines

Yes.
The only reason the warning exists is because it is undefined behaviour (not implementation-defined or anything). The reason it is that in the standard is that it is hard to implement and even describe for machines that are not two's complement. However relevant that is today :-)

Could gcc follow the clang behavior then and skip the warning and sanitizer for this case when -fno-strict-overflow or -fwrapv are used?

Arnd

Linus Torvalds

2022-02-26 23:03 UTC #37 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 3 lines · Arnd Bergmann

Could gcc follow the clang behavior then and skip the warning and sanitizer for this case when -fno-strict-overflow or -fwrapv are used?

Well, for the kernel, that horse has already left the barn, and we'd have to use -Wno-shift-negative-value anyway.

But yes, from a sanity standpoint, it would be good to shut that warning up automatically if compiling for a 2's complement machine (ie "all of them") with -fwrapv.

Considering that gcc doesn't support any non-2's-complement machines anyway afaik, and that the C standards people are also fixing the standard, and gcc has never done anything odd in this area in the first place, I think the warning is probably best removed entirely. But we'll have to do it manually for the existing situation.

Linus

Segher Boessenkool

2022-02-27 01:24 UTC #38 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Arnd Bergmann
Could gcc follow the clang behavior then and skip the warning and sanitizer for this case when -fno-strict-overflow or -fwrapv are used?
Well, for the kernel, that horse has already left the barn, and we'd have to use -Wno-shift-negative-value anyway.

Quoted 6 lines

But yes, from a sanity standpoint, it would be good to shut that warning up automatically if compiling for a 2's complement machine (ie "all of them") with -fwrapv.
Considering that gcc doesn't support any non-2's-complement machines anyway afaik,

* 'Whether signed integer types are represented using sign and magnitude, two's complement, or one's complement, and whether the extraordinary value is a trap representation or an ordinary value (C99 and C11 6.2.6.2).'

GCC supports only two's complement integer types, and all bit patterns are ordinary values.

Quoted 3 lines

and that the C standards people are also fixing the standard, and gcc has never done anything odd in this area in the first place, I think the warning is probably best removed entirely.

Well, not removed, it correctly identifies (formally) undefined behaviour after all; but I agree it should not be in -Wextra.

-Wall should include the warnings that have a very good balance for usefulness, number of false postives, seriousness of problems found. -Wextra is exactly the same conditions, just a slightly lower bar.

-Wall should be useful for everyone. -Wall -W should be good for most people.

Quoted 1 line

But we'll have to do it manually for the existing situation.

Yes, sorry about that :-/

Segher

Segher Boessenkool

2022-02-27 01:16 UTC #39 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 10 lines · Arnd Bergmann

Quoted 8 lines · Segher Boessenkool
Quoted 3 lines · Linus Torvalds
That said, we seem to only have two cases of it in the kernel, at least by a x86-64 allmodconfig build. So we could examine the types there, or we could just add '-Wno-shift-negative-value".
Yes.

Quoted 7 lines

Quoted 4 lines
The only reason the warning exists is because it is undefined behaviour (not implementation-defined or anything). The reason it is that in the standard is that it is hard to implement and even describe for machines that are not two's complement. However relevant that is today :-)
Could gcc follow the clang behavior then and skip the warning and sanitizer for this case when -fno-strict-overflow or -fwrapv are used?

As I said, we have this implementation choice documented as
As an extension to the C language, GCC does not use the latitude given in C99 and C11 only to treat certain aspects of signed '<<' as undefined. However, '-fsanitize=shift' (and '-fsanitize=undefined') will diagnose such cases. They are also diagnosed where constant expressions are required.
but that is not at all what we implement currently, we warn much more often. Constant expressions are required only in a few places (#if condition, bitfield length, (non-variable) array length, enumeration declarations, _Alignas, _Static_assert, case labels, most initialisers); not places you will see code like this problem normally.

So imo we should just never do this by default, not just if the nasty -fwrapv or nastier -fno-strict-overflow is used, just like we suggest in our own documentation. The only valid reason -Wshift-negative-value is in -Wextra is it warns for situations that always are undefined behaviour (even if not in GCC).

Could you open a GCC PR for this? The current situation is quite suboptimal, and what we document as our implementation choice is much more useful!

Segher

David Laight

2022-02-27 07:10 UTC #40 lore raw

RE: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

From: Segher Boessenkool

Quoted 12 lines

Sent: 27 February 2022 01:10
Quoted 9 lines · Arnd Bergmann
Quoted 7 lines · Segher Boessenkool
On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds wrote:
The only reason the warning exists is because it is undefined behaviour (not implementation-defined or anything). The reason it is that in the standard is that it is hard to implement and even describe for machines that are not two's complement. However relevant that is today :-)

I thought only right shifts of negative values were 'undefined'. And that was to allow cpu that only had logical shift right (ie ones that didn't propagate the sign) to be conformant. I wonder when the last cpu like that was?

Quite why the standards keeps using the term 'undefined behaviour' beats me - there ought to be something for 'undefined value'.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Segher Boessenkool

2022-02-27 11:38 UTC #41 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 11 lines · David Laight

From: Segher Boessenkool
Quoted 8 lines
Sent: 27 February 2022 01:10
Quoted 6 lines · Arnd Bergmann
Quoted 4 lines · Segher Boessenkool
The only reason the warning exists is because it is undefined behaviour (not implementation-defined or anything). The reason it is that in the standard is that it is hard to implement and even describe for machines that are not two's complement. However relevant that is today :-)
I thought only right shifts of negative values were 'undefined'.

All shifts by a negative amount, or an amount greater or equal to the width of the first operand (after the integer promotions).

Right shifts of a negative value.

Left shifts of a negative value where E1*2**E2 is not expressable in the result type.

C90 (aka C89) had those right shifts merely implementation-defined behaviour, and the left shifts perfectly well-defined.

Quoted 2 lines

And that was to allow cpu that only had logical shift right (ie ones that didn't propagate the sign) to be conformant.

The C99 rationale says
The C89 Committee affirmed the freedom in implementation granted by K&R in not requiring the signed right shift operation to sign extend, since such a requirement might slow down fast code and since the usefulness of sign extended shifts is marginal. (Shifting a negative two’s-complement integer arithmetically right one place is not the same as dividing by two!)

Quoted 1 line

I wonder when the last cpu like that was?

There still are one-off cores without such an instruction. If you have right shifts at all (if you have shifts at all!) it quickly becomes apparent what a hassle it is not to have an SRA/ASR/SAR instruction, and it is easy to implement, so :-)

The last widely spread ones' complement machine was the 6600 I guess, which disappeared somewhere in the 80's. Sign-magnitude machines are still made: all FP is like that, and some (simple, embedded, etc.) machines have no separate integer register set. C is available for most such fringe CPUs :-)

Quoted 2 lines

Quite why the standards keeps using the term 'undefined behaviour' beats me - there ought to be something for 'undefined value'.

It is pretty much impossible to not have *some* undefined behaviour. How will you define dividing by zero so that its behaviour is reasonable for every program, for example? Invoking an error handler at runtime has most of the same unwanted effects, except is is never silent. You can get that via UBSAN for example. Defining some arbitrary value as the "correct" answer when that is not at all obvious *does* give silent, unexpected results.

C does have "unspecified value"s and "unspecified behaviour". It requires the implementation to document the choice made here. At least some effort was made not to have undefined behaviour everywhere.

Perhaps C does have the concept you are after in 6.3.2/2, where it talks about using uninitialised objects? Of course, using an uninitialised object is undefined behaviour :-P

C has much more undefined behaviours than most other languages. On one hand there was no clear, formal definition of the language (and testsuites for it etc.) before it became popular. On the other hand, it was implemented on widely different architectures, back in the days when there was a lot more variety in implementation choices than there is now. When the language was standardised (and all the way to this day) the sentiment was to not unnecessarily break existing implementations.

Segher

Miguel Ojeda

2022-02-27 18:09 UTC #42 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 3 lines · Segher Boessenkool

How will you define dividing by zero so that its behaviour is reasonable for every program, for example?

The solution is to let the developer specify what they need to happen. That choice should include the unsafe possibility (i.e. unchecked), because sometimes that is precisely what we need.

Quoted 2 lines

Invoking an error handler at runtime
has most of the same unwanted effects, except is is never silent. You

It may not be what it is needed in some cases (thus the necessity to be able to choose), but at least one can predict what happens and different compilers, versions, flags, inputs, etc. would agree.

Cheers,
Miguel

Segher Boessenkool

2022-02-27 20:23 UTC #43 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 9 lines · Miguel Ojeda

Quoted 3 lines · Segher Boessenkool
How will you define dividing by zero so that its behaviour is reasonable for every program, for example?
The solution is to let the developer specify what they need to happen. That choice should include the unsafe possibility (i.e. unchecked), because sometimes that is precisely what we need.

Requiring to annotate every place that has UB (or *can* have UB!) by the user is even less friendly than having so much UB is already :-(

I don't see how you will fit this into the C syntax, btw?

Quoted 6 lines

Quoted 2 lines
Invoking an error handler at runtime
has most of the same unwanted effects, except is is never silent. You
It may not be what it is needed in some cases (thus the necessity to be able to choose), but at least one can predict what happens and different compilers, versions, flags, inputs, etc. would agree.

You need a VM like Java's to get even *close* to that. This is not the C target: it is slower than wanted/expected, it is hosted instead of embedded, and it comes with a whole host of issues of its own. One of the strengths of C is its tiny runtime, a few kB is a lot already!

I completely agree that if you design a new "systems" language, you want to have much less undefined behaviour than C has. But it is self- delusion to think you can eradicate all (or even most).

And there are much bigger problems in any case! If you think that if programmers could no longer write programs that invoke undefined behaviour they will write much better programs, programs with fewer serious functionality or security problems, even just a factor of two better, well...

Segher

Linus Torvalds

2022-02-27 21:05 UTC #44 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 3 lines · Segher Boessenkool

Requiring to annotate every place that has UB (or *can* have UB!) by the user is even less friendly than having so much UB is already :-(

Yeah, I don't think that's the solution. In fact, I don't think that's even practically the _issue_.

Honestly, a lot of "undefined behavior" in C is quite often of the kind "the programmer knows what he wants".

Things like word size or byte order issues etc are classic "undefined behavior" in the sense that the C compiler really doesn't understand them. The C compiler won't silently fix any silly behavior you get from writing files in native byte order, and them not working on other platforms.

Same goes for things like memory allocators - they often need to do things that the standard doesn't really cover, and shouldn't even *try* to cover. It's very much a core example of where people do odd pointer arithmetic and change the type of pointers.

The problem with the C model of "undefined behavior" is not that the behavior ends up being architecture-specific and depending on various in-memory (or in-register) representation of the data. No, those things are often very much intentional (even if in the case of byte order, the "intention" may be that the programmer simply does not care, and "knows" that all the world is little endian).

If the C compiler just generates reliable code that can sanely be debugged - including very much using tools that look for "hey, this behavior can be surprising", ie all those "look for bad patterns at run-time", then that would be 100% FINE.

But the problem with the C notion of undefined behavior is NOT that "results can depend on memory layout and other architecture issues that the compiler doesn't understand".

No, the problem is that the C standards people - and compiler people - have then declared that "because this can be surprising, and the compiler doesn't understand what is going on, now the compiler can do something *else* entirely".

THAT is the problem.

The classic case - and my personal "beat a dead horse" - is the completely broken type-based aliasing rules. The standards people literally said "the compiler doesn't understand this, it can expose byte order dependencies that shouldn't be explained, SO THE COMPILER CAN NOW DO SOMETHING COMPLETELY INSANE INSTEAD".

And compiler people? They rushed out to do completely broken garbage - at least some of them did.

You can literally find compiler people who see code like this (very traditional, traditionally very valid and common, although):

// Return the least significant 16 bits of 'a' on LE machines
#define low_16_bits(x) (*(unsigned short *)&(x))

and say "oh, because that's undefined, I can now decide to not do what the programmer told me to do AT ALL".

Note that the above wasn't actually even badly defined originally. It was well-defined, it was used, and it was done by programmers that knew what they were doing.

And then the C standards people decided that "because our job isn't to describe all the architectural issues you can hit, we'll call it undefined, and in the process let compiler people intentionally break it".

THAT is a problem.

Undefined results are are often intentional in system software - or they can be debugged using smart tools (including possibly very expensive run-time code generation) that actively look for them.

But compilers that randomly do crazy things because the standard was bad? That's just broken.

If compilers treated "undefined" as the same as "implementation-defined, but not explicitly documented", then that would be fine. But the C standards people - and a lot of compiler people - really don't seem to understand the problems they caused.

And, btw, caused for no actual good reason. The HPC people who wanted Fortran-style aliasing could easily have had that with an extension. Yes, "restrict" is kind of a crappy one, but it could have been improved upon. Instead, people said "let's just break the language".

Same exact thing goes for signed integer overflow.

Linus

David Laight

2022-02-28 06:16 UTC #45 lore raw

RE: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

From: Linus Torvalds

Quoted 1 line

Sent: 27 February 2022 21:05

...

Quoted 6 lines

And then the C standards people decided that "because our job isn't to describe all the architectural issues you can hit, we'll call it undefined, and in the process let compiler people intentionally break it".
THAT is a problem.

I'm waiting for them to decide that memset(ptr, 0, len) of any structure that contains a pointer is UB (because a NULL pointer need not be the all zero bit pattern) so decide to discard the call completely (or some such).

Non-zero NULL pointers is the only reason arithmetic on NULL pointers isn't valid.

Or maybe that character range tests are UB because '0' to '9' don't have to be adjacent - they are even adjacent in EBCDIC.

Some of the 'strict aliasing' bits are actually useful since they let the compiler reorder reads and writes. But the definition is brain-dead.
Sometimes it would be nice to have byte writes reordered, but even using int:8 doesn't work.

I have never worked out what 'restrict' actually does, in any places I've tried it did nothing. Although I may have been hoping it would still help when the function got inlined.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Miguel Ojeda

2022-02-27 22:43 UTC #46 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

In reply to Segher Boessenkool

Quoted 3 lines · Segher Boessenkool

Requiring to annotate every place that has UB (or *can* have UB!) by the user is even less friendly than having so much UB is already :-(

Sure, but I did not suggest to annotate every place -- at least not in C.

What I said is that in cases like your division by zero example, there is no one-fits-all solution. For instance, for integer arithmetic, leaving the choice (e.g. wrapping, saturating, unchecked...) to the user is a better approach.

Quoted 8 lines

You need a VM like Java's to get even *close* to that. This is not the C target: it is slower than wanted/expected, it is hosted instead of embedded, and it comes with a whole host of issues of its own. One of the strengths of C is its tiny runtime, a few kB is a lot already!
I completely agree that if you design a new "systems" language, you want to have much less undefined behaviour than C has. But it is self- delusion to think you can eradicate all (or even most).

Nobody is suggesting to remove "all UB" in that sense or to use VM-supported languages.

However, you can """eradicate all UB""" in a sense: you can offer to write most code in a subset that does not allow any potential-UB operations. This can even be "all" code, depending on how you count (e.g. all application code).

Obviously, you still need the unchecked operations to implement the safe APIs. This is why I mentioned them.

Quoted 5 lines

And there are much bigger problems in any case! If you think that if programmers could no longer write programs that invoke undefined behaviour they will write much better programs, programs with fewer serious functionality or security problems, even just a factor of two better, well...

Actually, according to several reports from Google, Microsoft, etc., it _is_ the biggest problem (~70%) when talking about security bugs.

So it is a good bet that it will translate into "better" programs, at least on that axis, unless it is showed that removing UB somehow increases the rate of other errors.

As for functionality problems, there are several ways to improve upon C too, though it is harder to show data on that.

Cheers,
Miguel

Arnd Bergmann

maintainer

2022-02-27 21:29 UTC #47 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

In reply to Segher Boessenkool

Quoted 6 lines · Segher Boessenkool

So imo we should just never do this by default, not just if the nasty -fwrapv or nastier -fno-strict-overflow is used, just like we suggest in our own documentation. The only valid reason -Wshift-negative-value is in -Wextra is it warns for situations that always are undefined behaviour (even if not in GCC).

Ok, I just realized that this is specific to the i915 driver because that, unlike
most of the kernel builds with -Wextra by default. -Wextra is enabled when users ask for a 'make W=1' build in linux, and i915 is one of just three drivers that enable an equivalent set of warnings, the other ones being greybus and btrfs.

This means to work around the extra warnings, we also just need to disable it in the W=1 part of scripts/Makefile.extrawarn, as well as the three drivers that copy those options, but not the default warnings that don't include them.

Quoted 3 lines

Could you open a GCC PR for this? The current situation is quite suboptimal, and what we document as our implementation choice is much more useful!

I hope I managed to capture the issue in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104711

Arnd

Segher Boessenkool

2022-02-27 22:49 UTC #48 lore raw

Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

Quoted 16 lines · Arnd Bergmann

Quoted 6 lines · Segher Boessenkool
So imo we should just never do this by default, not just if the nasty -fwrapv or nastier -fno-strict-overflow is used, just like we suggest in our own documentation. The only valid reason -Wshift-negative-value is in -Wextra is it warns for situations that always are undefined behaviour (even if not in GCC).
Ok, I just realized that this is specific to the i915 driver because that, unlike
most of the kernel builds with -Wextra by default. -Wextra is enabled when users ask for a 'make W=1' build in linux, and i915 is one of just three drivers that enable an equivalent set of warnings, the other ones being greybus and btrfs.

Quoted 3 lines

This means to work around the extra warnings, we also just need to disable it in the W=1 part of scripts/Makefile.extrawarn, as well as the three drivers that copy those options, but not the default warnings that don't include them.

Ah good, all of the workaround in one simple place, neat.

Quoted 6 lines

Quoted 3 lines
Could you open a GCC PR for this? The current situation is quite suboptimal, and what we document as our implementation choice is much more useful!
I hope I managed to capture the issue in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104711

That looks fine. Thank you!

(I attached the testcase to the bug itself, we prefer it that way, maybe godbolt will go away some day, who knows.)

Segher

Jakob Koschel

author

2022-02-17 18:50 UTC #49 lore raw

RFC PATCH 4/13 vfio/mdev: remove the usage of the list iterator after the loop

It is unsafe to assume that tmp != mdev can only evaluate to false if the break within the list iterator is hit.

When the break is not hit, tmp is set to an address derived from the head element. If mdev would match with that value of tmp it would allow continuing beyond the safety check even if mdev was never found within the list

---
 drivers/vfio/mdev/mdev_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/vfio/mdev/mdev_core.c +5 -2

index b314101237fe..e646ba5036f4 100644
@@ -339,14 +339,17 @@ int mdev_device_remove(struct mdev_device *mdev)
 {
 	struct mdev_device *tmp;
 	struct mdev_parent *parent = mdev->type->parent;
+	bool found = false;
 
 	mutex_lock(&mdev_list_lock);
 	list_for_each_entry(tmp, &mdev_list, next) {
-		if (tmp == mdev)
+		if (tmp == mdev) {
+			found = true;
 			break;
+		}
 	}
 
-	if (tmp != mdev) {
+	if (!found) {
 		mutex_unlock(&mdev_list_lock);
 		return -ENODEV;
 	}

Signature · 1 line

2.25.1

Jason Gunthorpe

2022-02-18 15:12 UTC #50 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 7 lines · Jakob Koschel

It is unsafe to assume that tmp != mdev can only evaluate to false if the break within the list iterator is hit.
When the break is not hit, tmp is set to an address derived from the head element. If mdev would match with that value of tmp it would allow continuing beyond the safety check even if mdev was never found within the list

I think due to other construction this is not actually possible, but I guess it is technically correct

This seems like just a straight up style fix with nothing to do with speculative execution though. Why not just send it as a proper patch?

Jason

Jakob

author

2022-02-23 14:18 UTC #51 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 13 lines · Jason Gunthorpe

Quoted 7 lines · Jakob Koschel
It is unsafe to assume that tmp != mdev can only evaluate to false if the break within the list iterator is hit.
When the break is not hit, tmp is set to an address derived from the head element. If mdev would match with that value of tmp it would allow continuing beyond the safety check even if mdev was never found within the list

Quoted 7 lines

I think due to other construction this is not actually possible, but I guess it is technically correct
This seems like just a straight up style fix with nothing to do with speculative execution though. Why not just send it as a proper patch?
Jason

Thank you for your feedback.

I've raised some confusion here, I'm sorry about that. The idea was to change list_for_each_entry() to set 'pos' to NULL when the list terminates to avoid invalid usage in speculation.

This will break this code and I therefore included the suggested change in this RFC. This RFC was not intended to be merged as is.

However, in this example, 'tmp' will be a out-of-bounds pointer if the loop did finish without hitting the break, so the check past the loop *could* match 'mdev' even though no break was ever met.

I've now realized that this is probably not realistic iff mdev always points to a valid struct mdev_device.
(It's a slightly different scenario on PATCH 03/13).

Jakob

Linus Torvalds

2022-02-23 19:06 UTC #52 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 4 lines · Jakob

However, in this example, 'tmp' will be a out-of-bounds pointer if the loop did finish without hitting the break, so the check past the loop *could* match 'mdev' even though no break was ever met.

So just as context for others, since I was hit with the same confusion and didn't see what the relevance was for type speculation, when these patches seemed to be not about speculation at all.

The background for this is that the list_for_each_entry() will set the iterator variable (here 'tmp') to be not the actual internal list pointer, but the pointer to the containing type (which is the whole 'entry' part of the name, of course).

And that is all good and true, but it's true only *WITHIN* that loop. At the exit condition, it will have hit the 'head' of the list, and the type that contains the list head is *not* necessarily the same type as the list entries.

So that's where the type confusion comes from: if you access the list iterator outside the loop, and it could have fallen off the end of the loop, the list iterator pointer is now not actually really a valid pointer of that 'entry' type at all.

And as such, you not only can't dereference it, but you also shouldn't even compare pointer values - because the pointer arithmetic that was valid for loop entries is not valid for the HEAD entry that is embedded in another type. So the pointer arithmetic might have turned it into a pointer outside the real container of the HEAD, and might actually match something else.

Now, there are a number of reasons I really dislike the current RFC patch series, so I'm not claiming the patch is something we should apply as-is, but I'm trying to clarify why Jakob is doing what he's doing (because clearly I wasn't the only one taken by surprise by it).

The reasons I don't like it is:

- patches like these are very random. And very hard to read (and very
easy to re-introduce the bug).

- I think it conflates the non-speculative "use pointer of the wrong
type" issue like in this patch with the speculation

- I'm not even convinced that 'list_for_each_entry()' is that special
wrt speculative type accesses, considering that we have other uses of doubly linked list *everywhere* - and it can happen in a lot of other situations anyway, so it all seems to be a bit ad hoc.

but I do think the problem is real.

So elsewhere I suggested that the fix to "you can't use the pointer outside the loop" be made to literally disallow it (using C99 for-loop variables seems the cleanest model), and have the compiler refuse to touch code that tries to use the loop iterator outside.

And that is then entirely separate from the issue of actual speculative accesses (but honestly, I think that's a "you have to teach the compiler not to do them" issue, not a "let's randomly change *one* of our loop walkers).

Linus

Jason Gunthorpe

2022-02-23 19:12 UTC #53 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 6 lines · Linus Torvalds

And as such, you not only can't dereference it, but you also shouldn't even compare pointer values - because the pointer arithmetic that was valid for loop entries is not valid for the HEAD entry that is embedded in another type. So the pointer arithmetic might have turned it into a pointer outside the real container of the HEAD, and might actually match something else.

Yes, this is what I had put together as well about this patch, and I think it is OK as-is. In this case the list head is in the .bss of a module so I don't think it is very likely that the type confused container_of() will alias a kalloc result, but it is certainly technically wrong as-is.

Quoted 4 lines

So elsewhere I suggested that the fix to "you can't use the pointer outside the loop" be made to literally disallow it (using C99 for-loop variables seems the cleanest model), and have the compiler refuse to touch code that tries to use the loop iterator outside.

Oh yes, that would be really nice solution.

Jason

Linus Torvalds

2022-02-23 19:31 UTC #54 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 6 lines · Jason Gunthorpe

Yes, this is what I had put together as well about this patch, and I think it is OK as-is. In this case the list head is in the .bss of a module so I don't think it is very likely that the type confused container_of() will alias a kalloc result, but it is certainly technically wrong as-is.

I think that the pattern we should strive to use is not top use a 'bool' with the

- initialize to false, and then in loop: do 'found = true;' if found

- use the iterator variable if 'found'.

but instead strive to

- either only use the iterator variable inside the loop

- if you want to use it after the loop, have a externally visible
separate pointer initialized to NULL, and set it to the iterator variable inside the loop

IOW, instead of having a variable that is a 'bool', just make that variable _be_ the pointer you want. It's clearer, and it avoids using the iterator variable outside the loop.

It also is likely to generate better code, because there are fewer live variables - outside the loop now only uses that one variable, rather than using the 'bool' variable _and_ the iterator variable.

Linus

Jakob

author

2022-02-23 20:15 UTC #55 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 19 lines · Linus Torvalds

Quoted 6 lines · Jason Gunthorpe
Yes, this is what I had put together as well about this patch, and I think it is OK as-is. In this case the list head is in the .bss of a module so I don't think it is very likely that the type confused container_of() will alias a kalloc result, but it is certainly technically wrong as-is.
I think that the pattern we should strive to use is not top use a 'bool' with the
- initialize to false, and then in loop: do 'found = true;' if found
- use the iterator variable if 'found'.

Quoted 7 lines

but instead strive to
- either only use the iterator variable inside the loop
- if you want to use it after the loop, have a externally visible separate pointer initialized to NULL, and set it to the iterator variable inside the loop

in such a case you would still have to set the iterator value to NULL when reaching the terminating condition or am I missing something?

Since the iterator value will always be set to *something* when calling list_for_each_entry().

Quoted 4 lines

IOW, instead of having a variable that is a 'bool', just make that variable _be_ the pointer you want. It's clearer, and it avoids using the iterator variable outside the loop.

I completely agree and was intending to do it in such a way. However I couldn't find a way to do that without breaking the kernel between commits.

Either you need to first set the iterator to NULL when terminating which breaks the current code here

patch the code location first to do if(iterator), which does not work with the current list_for_each_entry().

Quoted 6 lines

It also is likely to generate better code, because there are fewer live variables - outside the loop now only uses that one variable, rather than using the 'bool' variable _and_ the iterator variable.
Linus

Linus Torvalds

2022-02-23 20:22 UTC #56 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 3 lines · Jakob

in such a case you would still have to set the iterator value to NULL when reaching the terminating condition or am I missing something?

No.

Make the rule be "you never use the iterator outside the loop".

IOW, the code sequence is

        some_struct *ptr, *iter;

        ptr = NULL;
        list_for_each_entry(iter, ...) {
                if (iter_matches_condition(iter)) {
                        ptr = iter;
                        break;
                }
        }

        .. never use 'iter' here - you use 'ptr' and check it for NULL ..

See? Same number of variables as using a separate 'bool found' flag, but simpler code, and it matches the rule of 'don't use iter outside the loop'.

This is how you'd have to do it anyway if we start using a C99 style 'declare iter _in_ the loop' model.

And as mentioned, it actually tends to lead to better code, since the code outside the loop only has one variable live, not two.

Of course, compilers can do a lot of optimizations, so a 'found' variable can be made to generate good code too - if the compiler just tracks it and notices, and turns the 'break' into a 'goto found', and the fallthrough into the 'goto not_found'.

So 'better code generation' is debatable, but even if the compiler can do as good a job with a separate 'bool' variable and some cleverness, I think we should strive for code where we make it easy for the compiler to DTRT - and where the generated code is easier to match up with what we wrote.

Linus

Jakob

author

2022-02-23 22:09 UTC #57 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · Jakob
in such a case you would still have to set the iterator value to NULL when reaching the terminating condition or am I missing something?

Quoted 7 lines

No.
Make the rule be "you never use the iterator outside the loop".
IOW, the code sequence is
       some_struct *ptr, *iter;

with C99 iter would be defined within the loop instead right?

Quoted 11 lines

       ptr = NULL;
       list_for_each_entry(iter, ...) {
               if (iter_matches_condition(iter)) {
                       ptr = iter;
                       break;
               }
       }

       .. never use 'iter' here - you use 'ptr' and check it for NULL ..

Quoted 3 lines

See? Same number of variables as using a separate 'bool found' flag, but simpler code, and it matches the rule of 'don't use iter outside the loop'.

ah yes this does make sense. I missed the part of using a separate 'ptr' variable. Thanks for clarifying.
I think this is a great idea.

There are cases where pos->member is used (the only legitimate way to use it right now). I suppose those turn into something like this (this example is inspired by dev_add_offload() (net/core/gro.c:38)):

       some_struct *ptr, *iter;
       list_head *list_ptr;

       ptr = NULL;
       list_for_each_entry(iter, head, list) {
               if (iter_matches_condition(iter)) {
                       ptr = iter;
                       break;
               }
       }
       

       if (ptr)
               list_ptr = head->prev;
       else
               list_ptr = iter->list.prev;
       list_add(..., list_ptr);

before it was simply

       list_add(..., iter->list.prev);

The other possibility I suppose would be:

       if (!ptr)
               ptr = container_of(head, typeof(*ptr), list)
       list_add(..., ptr->list.prev);

which leaves you with the same type confusion as before, being far from ideal.

Quoted 11 lines

This is how you'd have to do it anyway if we start using a C99 style 'declare iter _in_ the loop' model.
And as mentioned, it actually tends to lead to better code, since the code outside the loop only has one variable live, not two.
Of course, compilers can do a lot of optimizations, so a 'found' variable can be made to generate good code too - if the compiler just tracks it and notices, and turns the 'break' into a 'goto found', and the fallthrough into the 'goto not_found'.

Quoted 7 lines

So 'better code generation' is debatable, but even if the compiler can do as good a job with a separate 'bool' variable and some cleverness, I think we should strive for code where we make it easy for the compiler to DTRT - and where the generated code is easier to match up with what we wrote.
Linus

If there is interest, I'm happy to send a new patch set once the fixes are clear.

Jakob

Rasmus Villemoes

2022-02-23 20:19 UTC #58 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 23 lines · Linus Torvalds

Quoted 6 lines · Jason Gunthorpe
Yes, this is what I had put together as well about this patch, and I think it is OK as-is. In this case the list head is in the .bss of a module so I don't think it is very likely that the type confused container_of() will alias a kalloc result, but it is certainly technically wrong as-is.
I think that the pattern we should strive to use is not top use a 'bool' with the
- initialize to false, and then in loop: do 'found = true;' if found
- use the iterator variable if 'found'.
but instead strive to
- either only use the iterator variable inside the loop
- if you want to use it after the loop, have a externally visible
separate pointer initialized to NULL, and set it to the iterator variable inside the loop

Quoted 7 lines

IOW, instead of having a variable that is a 'bool', just make that variable _be_ the pointer you want. It's clearer, and it avoids using the iterator variable outside the loop.
It also is likely to generate better code, because there are fewer live variables - outside the loop now only uses that one variable, rather than using the 'bool' variable _and_ the iterator variable.

I have often wished that the iterator macros would consistently set the loop variable to NULL upon reaching the end. It could be done by changing the stop condition from

i != HEAD

  i != HEAD || (i = NULL, 0)

[the comma operator mostly for clarity, we want the side effect of the assignment but the whole RHS to still evaluate to false, which "i=NULL" by itself of course would].

In the vast majority of cases where the iterator variable is not used after the loop, the compiler should see through it and just drop the dead assignment. And it would make the test for "did we break out early finding what we looked for, or did we iterate till the end" quite natural, without any auxiliary bool or extra 'struct foo*' variable. But it will probably break too much existing code.

Rasmus

Linus Torvalds

2022-02-23 20:34 UTC #59 lore raw

Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

Quoted 3 lines · Rasmus Villemoes

I have often wished that the iterator macros would consistently set the loop variable to NULL upon reaching the end.

I really think the rule should be that to a 99% approximation, we should strive only ever use the iterated-upon value *inside* the loop.

No, that's now how we do it now. But I think the "break out and do the work outside the loop" case is kind of broken anyway. It makes you test the condition twice - and while a compiler might be smart enough to optimize the second test away, it's still just plain ugly.

Linus

Jakob Koschel

author

2022-02-17 18:50 UTC #60 lore raw

RFC PATCH 5/13 drivers/perf: remove the usage of the list iterator after the loop

To introduce a speculative safe list iterator, the iterator variable will be set to NULL when the terminating condition of the loop is hit.

The code before assumed rentry would only be NULL if the break is hit, this assumption no longer holds. Once the speculative safe list iterator is merged the condition could be replace with if (!entry) instead.

---
 drivers/perf/xgene_pmu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/perf/xgene_pmu.c +3 -2

index 2b6d476bd213..523ff5c53103 100644
@@ -1463,6 +1463,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 	struct resource_entry *rentry;
 	int enable_bit;
 	int rc;
+	bool found = false;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)

@@ -1478,13 +1479,13 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
 	list_for_each_entry(rentry, &resource_list, node) {
 		if (resource_type(rentry->res) == IORESOURCE_MEM) {
 			res = *rentry->res;
-			rentry = NULL;
+			found = true;
 			break;
 		}
 	}
 	acpi_dev_free_resource_list(&resource_list);
 
-	if (rentry) {
+	if (!found) {
 		dev_err(dev, "PMU type %d: No memory resource found\n", type);
 		return NULL;
 	}

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #61 lore raw

RFC PATCH 6/13 ARM: mmp: remove the usage of the list iterator after the loop

To introduce a speculative safe list iterator, the iterator variable will be set to NULL when the terminating condition of the loop is hit.

The code before assumed info would be derived from the head if the break did not hit, this assumption no longer holds. Once the speculative safe list iterator is merged the condition could be replace with if (!info) instead.

---
 arch/arm/mach-mmp/sram.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

arch/arm/mach-mmp/sram.c +5 -2

index 6794e2db1ad5..b163d2da53e7 100644
@@ -39,6 +39,7 @@ static LIST_HEAD(sram_bank_list);
 struct gen_pool *sram_get_gpool(char *pool_name)
 {
 	struct sram_bank_info *info = NULL;
+	bool found = false;
 
 	if (!pool_name)
 		return NULL;

@@ -46,12 +47,14 @@ struct gen_pool *sram_get_gpool(char *pool_name)
 	mutex_lock(&sram_lock);
 
 	list_for_each_entry(info, &sram_bank_list, node)
-		if (!strcmp(pool_name, info->pool_name))
+		if (!strcmp(pool_name, info->pool_name)) {
+			found = true;
 			break;
+		}
 
 	mutex_unlock(&sram_lock);
 
-	if (&info->node == &sram_bank_list)
+	if (!found)
 		return NULL;
 
 	return info->gpool;

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #62 lore raw

RFC PATCH 7/13 udp_tunnel: remove the usage of the list iterator after the loop

The usage of node->dev after the loop body is a legitimate type confusion if the break was not hit. It will compare an undefined memory location with dev that could potentially be equal. The value of node->dev in this case could either be a random struct member of the head element or an out-of-bounds value.

Therefore it is more safe to use the found variable. With the introduction of speculative safe list iterator this check could be replaced with if (!node).

---
 net/ipv4/udp_tunnel_nic.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

net/ipv4/udp_tunnel_nic.c +5 -2

index b91003538d87..c47f9fb36d29 100644
@@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
 	 */
 	if (info->shared) {
 		struct udp_tunnel_nic_shared_node *node, *first;
+		bool found = false;
 
 		list_for_each_entry(node, &info->shared->devices, list)
-			if (node->dev == dev)
+			if (node->dev == dev) {
+				found = true;
 				break;
-		if (node->dev != dev)
+			}
+		if (!found)
 			return;
 
 		list_del(&node->list);

Signature · 1 line

2.25.1

Christophe JAILLET

2022-02-23 20:00 UTC #63 lore raw

Re: [RFC PATCH 07/13] udp_tunnel: remove the usage of the list iterator after the loop

Le 17/02/2022 à 19:48, Jakob Koschel a écrit :

Quoted 26 lines

The usage of node->dev after the loop body is a legitimate type confusion if the break was not hit. It will compare an undefined memory location with dev that could potentially be equal. The value of node->dev in this case could either be a random struct member of the head element or an out-of-bounds value.
Therefore it is more safe to use the found variable. With the introduction of speculative safe list iterator this check could be replaced with if (!node).
Signed-off-by Jakob Koschel <[email protected]>
---
  net/ipv4/udp_tunnel_nic.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
net/ipv4/udp_tunnel_nic.c +1 -0
index b91003538d87..c47f9fb36d29 100644
@@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
  	 */
  	if (info->shared) {
  		struct udp_tunnel_nic_shared_node *node, *first;
+		bool found = false;
 
  		list_for_each_entry(node, &info->shared->devices, list)

net/ipv4/udp_tunnel_nic.c udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn) · line 855

Quoted 10 lines

net/ipv4/udp_tunnel_nic.c +4 -2

-			if (node->dev == dev)
+			if (node->dev == dev) {
+				found = true;
  				break;
-		if (node->dev != dev)
+			}
+		if (!found)
  			return;
 
  		list_del(&node->list);

Hi,

just in case, see Dan Carpeter's patch for the same issue with another fix at:
https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/

Dan Carpenter

2022-02-24 06:21 UTC #64 lore raw

Re: [RFC PATCH 07/13] udp_tunnel: remove the usage of the list iterator after the loop

Quoted 38 lines · Christophe JAILLET

Le 17/02/2022 à 19:48, Jakob Koschel a écrit :
Quoted 24 lines
The usage of node->dev after the loop body is a legitimate type confusion if the break was not hit. It will compare an undefined memory location with dev that could potentially be equal. The value of node->dev in this case could either be a random struct member of the head element or an out-of-bounds value.
Therefore it is more safe to use the found variable. With the introduction of speculative safe list iterator this check could be replaced with if (!node).
Signed-off-by Jakob Koschel <[email protected]>
---
  net/ipv4/udp_tunnel_nic.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
net/ipv4/udp_tunnel_nic.c +1 -0
index b91003538d87..c47f9fb36d29 100644
@@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
  	 */
  	if (info->shared) {
  		struct udp_tunnel_nic_shared_node *node, *first;
+		bool found = false;
net/ipv4/udp_tunnel_nic.c udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn) · line 853
Quoted 10 lines
net/ipv4/udp_tunnel_nic.c +4 -2
  		list_for_each_entry(node, &info->shared->devices, list)
-			if (node->dev == dev)
+			if (node->dev == dev) {
+				found = true;
  				break;
-		if (node->dev != dev)
+			}
+		if (!found)
  			return;
  		list_del(&node->list);
Hi,

Quoted 3 lines

just in case, see Dan Carpeter's patch for the same issue with another fix at:
https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/

Yeah. My patch was already applied.

I've had an unpublished Smatch check for this for a while but I've been re-writing it recently to make it more generic so that it worked for all the different list_for_each type macros. I'm going to publish it soon.

Of course, all the real bugs are fixed so the remaining warnings are false positives.

regards,
dan carpenter

Jakob Koschel

author

2022-02-17 18:50 UTC #65 lore raw

RFC PATCH 8/13 net: dsa: future proof usage of list iterator after the loop

With the speculative safe version of the list iterator p will be NULL if the terminating condition was hit and needs to be reset to p derived from the head, before using p->list.

---
 drivers/net/dsa/sja1105/sja1105_vl.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/net/dsa/sja1105/sja1105_vl.c +2 -0

index f5dca6a9b0f9..a4f2b95b09c4 100644
@@ -40,6 +40,8 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 			if (e->interval < p->interval)
 				break;
 		}
+		if (!p)
+			p = list_entry(p, &gating_cfg->entries, list);
 		list_add(&e->list, p->list.prev);
 	}

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #66 lore raw

RFC PATCH 9/13 drbd: future proof usage of list iterator after the loop

With the speculative safe version of the list iterator req will be NULL if the terminating condition was hit and needs to be reset to req derived from the head, before calling list_for_each_entry_safe_from().

---
 drivers/block/drbd/drbd_main.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/block/drbd/drbd_main.c +2 -0

index 6f450816c4fa..d98205b46f59 100644
@@ -227,6 +227,8 @@ void tl_release(struct drbd_connection *connection, unsigned int barrier_nr,
 	list_for_each_entry(req, &connection->transfer_log, tl_requests)
 		if (req->epoch == expect_epoch)
 			break;
+	if (!req)
+		req = list_entry(req, &connection->transfer_log, tl_requests);
 	list_for_each_entry_safe_from(req, r, &connection->transfer_log, tl_requests) {
 		if (req->epoch != expect_epoch)
 			break;

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #67 lore raw

RFC PATCH 10/13 powerpc/spufs: future proof usage of list iterator after the loop

With the speculative safe version of the list iterator spu will be NULL if the terminating condition was hit and needs to be reset to spu derived from the head, before returning spu.

---
 arch/powerpc/platforms/cell/spufs/sched.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/platforms/cell/spufs/sched.c +2 -0

index 369206489895..5b2afda1653d 100644
@@ -384,6 +384,8 @@ static struct spu *ctx_location(struct spu *ref, int offset, int node)
 		}
 	}
 
+	if (!spu)
+		spu = list_entry(spu, ref->aff_list.prev, aff_list);
 	return spu;
 }

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #68 lore raw

RFC PATCH 11/13 ath6kl: remove use of list iterator after the loop

Since the list iteration cannot abort early, cur_ep_dist->list will always reference to ep_list and can therefore be replaced.

---
 drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath6kl/htc_mbox.c +1 -1

index e3874421c4c0..cf5b05860799 100644
@@ -104,7 +104,7 @@ static void ath6kl_credit_init(struct ath6kl_htc_credit_info *cred_info,
 	 * it use list_for_each_entry_reverse to walk around the whole ep list.
 	 * Therefore assign this lowestpri_ep_dist after walk around the ep_list
 	 */
-	cred_info->lowestpri_ep_dist = cur_ep_dist->list;
+	cred_info->lowestpri_ep_dist = *ep_list;
 
 	WARN_ON(cred_info->cur_free_credits <= 0);

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #69 lore raw

RFC PATCH 12/13 staging: greybus: audio: Remove usage of list iterator after the loop

The list iterator module should not be used if data == NULL. module can only old a legitimate value if data != NULL.

---
 drivers/staging/greybus/audio_codec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/greybus/audio_codec.c +2 -2

index b589cf6b1d03..e19b91e7a72e 100644
@@ -599,8 +599,8 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
 			break;
 	}
 	if (!data) {
-		dev_err(dai->dev, "%s:%s DATA connection missing\n",
-			dai->name, module->name);
+		dev_err(dai->dev, "%s DATA connection missing\n",
+			dai->name);
 		mutex_unlock(&codec->lock);
 		return -ENODEV;
 	}

Signature · 1 line

2.25.1

Jakob Koschel

author

2022-02-17 18:50 UTC #70 lore raw

RFC PATCH 13/13 scsi: mpt3sas: comment about invalid usage of the list iterator

Since the list iteration never exists early, reply_q is guaranteed to be an invalid variable and should not be used within _base_process_reply_queue(). Since I'm not sure what this code was supposed to do, I just marked this with this comment.