From: Bart Van Assche <[email protected]>
Using a __bitwise type in a tracing __field() definition triggers four
sparse warnings in stage 4 of expanding the TRACE_EVENT() macro. These
warnings are triggered by the is_signed_type() macro implementation.
Since there is no known way of checking signedness of a bitwise type
without triggering sparse warnings, disable signedness checking when
verifying code with sparse.
Note: work is in progress to improve sparse but has not yet landed. See
also "[PATCH 0/5] allow -1 and compares in bitwise types"
(https://lore.kernel.org/all/[email protected]/ ).
Link: https://lore.kernel.org/linux-trace-devel/[email protected]
Suggested-by: Christoph Hellwig <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Luc Van Oostenryck <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Jens Axboe <[email protected]>
Signed-off-by: Bart Van Assche <[email protected]>
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
include/linux/trace_events.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..b60347a0ccde 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -814,7 +814,19 @@ extern int trace_add_event_call(struct trace_event_call *call);
extern int trace_remove_event_call(struct trace_event_call *call);
extern int trace_event_get_offsets(struct trace_event_call *call);
+/*
+ * There is no known way to check signedness of __bitwise types without
+ * triggering a sparse warning. Hence the #ifdef __CHECKER__.
+ *
+ * Since there is another definition of is_signed_type() in <linux/overflow.h>,
+ * undefine is_signed_type() before redefining it.
+ */
+#undef is_signed_type
+#ifdef __CHECKER__
+#define is_signed_type(type) 0
+#else
#define is_signed_type(type) (((type)(-1)) < (type)1)
+#endif
int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
int trace_set_clr_event(const char *system, const char *event, int set);
--
2.35.1
On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <[email protected]> wrote:
>
> Since there is no known way of checking signedness of a bitwise type
> without triggering sparse warnings, disable signedness checking when
> verifying code with sparse.
What, what, what?
The last I saw of this discussion, the fix was just to make sparse not
warn about these cases. Why did this bogus fix make it into a pull
request that I will now ignore?
If we want to just shut up the sparse warning, then afaik the simple
one-liner fix would have been
-#define is_signed_type(type) (((type)(-1)) < (type)1)
+#define is_signed_type(type) (((__force type)(-1)) < (__force type)1)
and at least then sparse checks the same source as is compiled,
instead of passing a "this is not a signed type" to places.
So that "no known way" was always bogus, the real question was whether
there was a way to make sparse not need the "ignore bitwise" hack.
Btw, that patch is entirely broken for *another* reason.
Even if you were to say "ok, sparse just gets a different argument",
the fact that the trace_events file re-defined that is_signed_type()
macro means that you added that
+#undef is_signed_type
to make the compiler happy about how you only modified one of them.
But that then means that if <linux/trace_events.h> gets included
*before* <linux/overflow.h>, you'll just get the warning *there*
instead.
Now, that warning would only happen for a __CHECKER__ build - but
that's the only build this patch is relevant for anyway.
And maybe that ordering doesn't exist, or maybe it only exists on some
very random config. Regardless, it's broken.
Of course, the real fix should be to just not re-define that macro at
all, and just have it in *one* place.
Linus
On Sun, 21 Aug 2022 11:35:29 -0700
Linus Torvalds <[email protected]> wrote:
> On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <[email protected]> wrote:
> >
> > Since there is no known way of checking signedness of a bitwise type
> > without triggering sparse warnings, disable signedness checking when
> > verifying code with sparse.
>
> What, what, what?
>
> The last I saw of this discussion, the fix was just to make sparse not
> warn about these cases. Why did this bogus fix make it into a pull
> request that I will now ignore?
Sorry, I was triaging my internal patchwork and saw the "Suggested-by"
Christoph and was thinking this was what we decided on.
>
> If we want to just shut up the sparse warning, then afaik the simple
> one-liner fix would have been
>
> -#define is_signed_type(type) (((type)(-1)) < (type)1)
> +#define is_signed_type(type) (((__force type)(-1)) < (__force type)1)
>
> and at least then sparse checks the same source as is compiled,
> instead of passing a "this is not a signed type" to places.
>
> So that "no known way" was always bogus, the real question was whether
> there was a way to make sparse not need the "ignore bitwise" hack.
>
> Btw, that patch is entirely broken for *another* reason.
>
> Even if you were to say "ok, sparse just gets a different argument",
> the fact that the trace_events file re-defined that is_signed_type()
> macro means that you added that
>
> +#undef is_signed_type
>
> to make the compiler happy about how you only modified one of them.
>
> But that then means that if <linux/trace_events.h> gets included
> *before* <linux/overflow.h>, you'll just get the warning *there*
> instead.
>
> Now, that warning would only happen for a __CHECKER__ build - but
> that's the only build this patch is relevant for anyway.
>
> And maybe that ordering doesn't exist, or maybe it only exists on some
> very random config. Regardless, it's broken.
>
> Of course, the real fix should be to just not re-define that macro at
> all, and just have it in *one* place.
I'll remove this patch and send another pull request.
Thanks,
-- Steve
On 8/21/22 11:35, Linus Torvalds wrote:
> If we want to just shut up the sparse warning, then afaik the simple
> one-liner fix would have been
>
> -#define is_signed_type(type) (((type)(-1)) < (type)1)
> +#define is_signed_type(type) (((__force type)(-1)) < (__force type)1)
>
> and at least then sparse checks the same source as is compiled,
> instead of passing a "this is not a signed type" to places.
Hi Linus,
I agree that it's better that sparse sees the same code as what is used to
build the kernel. However, I do not agree that the patch above is a solution.
Sparse reports a warning for the suggested definition above of is_signed_type()
because the new definition tries to use the less-than (<) operator to compare
two __bitwise types.
> Btw, that patch is entirely broken for *another* reason.
>
> Even if you were to say "ok, sparse just gets a different argument",
> the fact that the trace_events file re-defined that is_signed_type()
> macro means that you added that
>
> +#undef is_signed_type
>
> to make the compiler happy about how you only modified one of them.
>
> But that then means that if <linux/trace_events.h> gets included
> *before* <linux/overflow.h>, you'll just get the warning *there*
> instead.
>
> Now, that warning would only happen for a __CHECKER__ build - but
> that's the only build this patch is relevant for anyway.
>
> And maybe that ordering doesn't exist, or maybe it only exists on some
> very random config. Regardless, it's broken.
>
> Of course, the real fix should be to just not re-define that macro at
> all, and just have it in *one* place.
Agreed that is_signed_type() should only be defined once. If nobody else
beats me to this I will prepare a patch to fix this.
Thanks,
Bart.
On Mon, Aug 22, 2022 at 11:20 AM Bart Van Assche <[email protected]> wrote:
>
> I agree that it's better that sparse sees the same code as what is used to
> build the kernel. However, I do not agree that the patch above is a solution.
> Sparse reports a warning for the suggested definition above of is_signed_type()
> because the new definition tries to use the less-than (<) operator to compare
> two __bitwise types.
Argh. I forgot that part. It wasn't just the cast that warned, it was
the compare too.
But we did have a sparse fix for it, didn't we? That fix required that
the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
sparse, but it at least avoided the problem.
Linus
On 8/22/22 11:45, Linus Torvalds wrote:
> On Mon, Aug 22, 2022 at 11:20 AM Bart Van Assche <[email protected]> wrote:
>>
>> I agree that it's better that sparse sees the same code as what is used to
>> build the kernel. However, I do not agree that the patch above is a solution.
>> Sparse reports a warning for the suggested definition above of is_signed_type()
>> because the new definition tries to use the less-than (<) operator to compare
>> two __bitwise types.
>
> Argh. I forgot that part. It wasn't just the cast that warned, it was
> the compare too.
>
> But we did have a sparse fix for it, didn't we? That fix required that
> the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
> sparse, but it at least avoided the problem.
Hi Linus,
I haven't seen any progress on the plan to modify sparse. Hence my attempt to
modify the is_signed_type() definition in the kernel.
Thanks,
Bart.
On 22/08/2022 20.45, Linus Torvalds wrote:
> On Mon, Aug 22, 2022 at 11:20 AM Bart Van Assche <[email protected]> wrote:
>>
>> I agree that it's better that sparse sees the same code as what is used to
>> build the kernel. However, I do not agree that the patch above is a solution.
>> Sparse reports a warning for the suggested definition above of is_signed_type()
>> because the new definition tries to use the less-than (<) operator to compare
>> two __bitwise types.
>
> Argh. I forgot that part. It wasn't just the cast that warned, it was
> the compare too.
>
> But we did have a sparse fix for it, didn't we? That fix required that
> the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
> sparse, but it at least avoided the problem.
Heh. I originally wrote the comparison "< (t)1" instead of "< (t)0" to
avoid a -Wtype-limits warning when applied to unsigned types - yeah
yeah, the kernel isn't built with that, but it's a nice macro to
copy-paste to other projects, and sometimes people do explicitly enable
-Wtype-limits to manually go through some, and then it's nice to not
have tons of false positives from this macro.
But of course <1 is the same as <=0, and we can indeed spell it that way
without triggering Wtype-limits. So if that can help with also silencing
sparse, ack from me on that part.
Rasmus
On 8/23/22 00:06, Rasmus Villemoes wrote:
> On 22/08/2022 20.45, Linus Torvalds wrote:
>> But we did have a sparse fix for it, didn't we? That fix required that
>> the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
>> sparse, but it at least avoided the problem.
>
> Heh. I originally wrote the comparison "< (t)1" instead of "< (t)0" to
> avoid a -Wtype-limits warning when applied to unsigned types - yeah
> yeah, the kernel isn't built with that, but it's a nice macro to
> copy-paste to other projects, and sometimes people do explicitly enable
> -Wtype-limits to manually go through some, and then it's nice to not
> have tons of false positives from this macro.
>
> But of course <1 is the same as <=0, and we can indeed spell it that way
> without triggering Wtype-limits. So if that can help with also silencing
> sparse, ack from me on that part.
Thank you Rasmus for having shared this information. Since sparse will
have to be modified anyway, how about extending it such that the bitwise
attribute can be removed from a type, e.g. via a new no_bitwise
attribute? Wouldn't that be a more generic solution than only
suppressing sparse complaints when comparing compile-time constants and
when the left-hand-side and right-hand-side have different bitwise
attributes? For reference purposes, this is how __bitwise is defined:
#ifdef __CHECKER__
#define __bitwise __attribute__((bitwise))
#else
#define __bitwise
#endif
Thanks,
Bart.
On Tue, Aug 23, 2022 at 3:05 PM Bart Van Assche <[email protected]> wrote:
>
> Thank you Rasmus for having shared this information. Since sparse will
> have to be modified anyway, how about extending it such that the bitwise
> attribute can be removed from a type, e.g. via a new no_bitwise
> attribute?
I think it's actually easier to just make sparse happy.
Can you try the sparse version at
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
which I just set up temporarily with some patches of mine. It also
makes that '__cond_acquires' thing work that refcount_dec_and_lock()
uses.
It does require that kernel change to make
#define is_signed_type(type) (((type)(-1)) <= (type)0)
in both places, since only "no bits set" and "all bits set" are
special values for bitwise types.
Those patches of mine are fairly hacky, and I think Luc would probably
do it differently, but apart from the very last one, they aren't
actively disgusting.
Linus
On 8/23/22 16:18, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 3:05 PM Bart Van Assche <[email protected]> wrote:
>>
>> Thank you Rasmus for having shared this information. Since sparse will
>> have to be modified anyway, how about extending it such that the bitwise
>> attribute can be removed from a type, e.g. via a new no_bitwise
>> attribute?
>
> I think it's actually easier to just make sparse happy.
>
> Can you try the sparse version at
>
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
>
> which I just set up temporarily with some patches of mine. It also
> makes that '__cond_acquires' thing work that refcount_dec_and_lock()
> uses.
>
> It does require that kernel change to make
>
> #define is_signed_type(type) (((type)(-1)) <= (type)0)
>
> in both places, since only "no bits set" and "all bits set" are
> special values for bitwise types.
>
> Those patches of mine are fairly hacky, and I think Luc would probably
> do it differently, but apart from the very last one, they aren't
> actively disgusting.
Hi Linus,
I'm probably doing something wrong but even with sparse commit 658ee8e0f631
("unrestricted values are unrestricted even after a cast") I see warnings
being triggered by users of the is_signed_type() macro, warnings that
disappear if I change the definition of the is_signed_type() macro into 0:
$ make C=2 fs/f2fs/ </dev/null |& grep blk_opf_t
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
This is the kernel patch that I applied:
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f1221d11f8e5..10c55f97e02b 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -30,7 +30,7 @@
* https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
* credit to Christian Biere.
*/
-#define is_signed_type(type) (((type)(-1)) < (type)1)
+#define is_signed_type(type) (((__force type)(-1)) <= (__force type)0)
#define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
#define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
#define type_min(T) ((T)((T)-type_max(T)-(T)1))
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..c74cfa657025 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -9,6 +9,7 @@
#include <linux/hardirq.h>
#include <linux/perf_event.h>
#include <linux/tracepoint.h>
+#include <linux/overflow.h>
struct trace_array;
struct array_buffer;
@@ -814,8 +815,6 @@ extern int trace_add_event_call(struct trace_event_call *call);
extern int trace_remove_event_call(struct trace_event_call *call);
extern int trace_event_get_offsets(struct trace_event_call *call);
-#define is_signed_type(type) (((type)(-1)) < (type)1)
-
int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
int trace_set_clr_event(const char *system, const char *event, int set);
int trace_array_set_clr_event(struct trace_array *tr, const char *system,
Thanks,
Bart.
On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
<[email protected]> wrote:
>
> Can you try the sparse version at
>
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
>
> which I just set up temporarily with some patches of mine.
Ugh, and while testing this with sparse, I noticed that sparse itself
got that whole 'is_signed_type()' check wrong.
The sparse fix was to remove one line of code, but that one worries
me, because that one line was clearly very intentional:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d
Adding Al, since he's actually the original source of that bitwise
code (and did a lot of other sparse code on the type handling and
preprocessor side in particular).
Linus
On Tue, Aug 23, 2022 at 5:09 PM Bart Van Assche <[email protected]> wrote:
>
> I'm probably doing something wrong but even with sparse commit 658ee8e0f631
> ("unrestricted values are unrestricted even after a cast") I see warnings
> being triggered by users of the is_signed_type() macro, warnings that
> disappear if I change the definition of the is_signed_type() macro into 0:
That's the
> It does require that kernel change to make
>
> #define is_signed_type(type) (((type)(-1)) <= (type)0)
part I was talking about.
So your kernel side patch looks fine, except I don't think you need
the '__force' - the sparse patches in my tree should make sparse happy
about casting '-1'.
But I didn't do very much testing.
Linus
On Tue, Aug 23, 2022 at 6:49 PM Linus Torvalds
<[email protected]> wrote:
>
> That's the
>
> > It does require that kernel change to make
> >
> > #define is_signed_type(type) (((type)(-1)) <= (type)0)
>
> part I was talking about.
Side note: I think you could move this into '<linux/compiler.h>' and that would
(a) make some conceptual sense (unlike "overflow.h" and "trace_events.h")
and
(b) mean it gets included automatically in both files.
overflow.h already explicitly includes compiler.h, and trace_events.h
gets it from
linux/ring_buffer.h -> linux/mm.h -> linux/bug.h -> asm/bug.h ->
linux/compiler.h
(it goes other wats too, but those ones are through arch-specific asm
headers, so the above is the first non-arch-specific unconditional
chain I found.
And yes, we should have some tool for sorting out these nasty header
chains. Some automated tool that says "You don't need header X,
because you already got it" or "You don't need headed X, because
nothing you do depends on it".
Linus
On Tue, Aug 23, 2022 at 7:09 PM Al Viro <[email protected]> wrote:
>
> I'll take a look, but there's an unrelated problem:
> ANY ordered comparisons should spew warnings on bitwise
In the general case, absolutely.
But we have two special values that are the same in any bit ordering:
-1 and 0 (all bits set and no bits set). And yes, my patch verifies
that.
See commit 18f17cde ("allow restricted ordered compares with
unrestricted values") in that same tree.
That said, even those are strictly speaking only well-defined in
*unsigned* compares, so I guess we should add that - if you have a
*signed* bitwise thing, even those aren't well-defined to compare
against.
Now, I don't think signed bitwise types make much sense, and I
certainly hope we don't have them in the kernel, but yes, it is
probably worth adding that check too.
Of course, that check then depends on getting the signedness right for
bitwise types, which is exactly the problem I wanted you to look at ;)
Linus
On Tue, Aug 23, 2022 at 04:57:00PM -0700, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > Can you try the sparse version at
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
> >
> > which I just set up temporarily with some patches of mine.
>
> Ugh, and while testing this with sparse, I noticed that sparse itself
> got that whole 'is_signed_type()' check wrong.
>
> The sparse fix was to remove one line of code, but that one worries
> me, because that one line was clearly very intentional:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d
>
> Adding Al, since he's actually the original source of that bitwise
> code (and did a lot of other sparse code on the type handling and
> preprocessor side in particular).
Ouch... That'll take quite a bit of swap-in (and digging through the
old notes). I'll take a look, but there's an unrelated problem:
ANY ordered comparisons should spew warnings on bitwise
And we really want that to happen - things like
#define MASK cpu_to_le32(1023)
if (foo->len > MASK)
return -EINVAL;
something(le32_to_cpu(foo->len));
should trigger warnings and I have seen real bugs of that sort.
So I'm not sure how is that supposed to work without sparse getting
loudly unhappy.
Al, going to look through that thread and then try to reconstruct sparse-related
notes...
On Wed, Aug 24, 2022 at 03:09:07AM +0100, Al Viro wrote:
> On Tue, Aug 23, 2022 at 04:57:00PM -0700, Linus Torvalds wrote:
> > On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
> > <[email protected]> wrote:
> > >
> > > Can you try the sparse version at
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
> > >
> > > which I just set up temporarily with some patches of mine.
> >
> > Ugh, and while testing this with sparse, I noticed that sparse itself
> > got that whole 'is_signed_type()' check wrong.
> >
> > The sparse fix was to remove one line of code, but that one worries
> > me, because that one line was clearly very intentional:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d
> >
> > Adding Al, since he's actually the original source of that bitwise
> > code (and did a lot of other sparse code on the type handling and
> > preprocessor side in particular).
>
> Ouch... That'll take quite a bit of swap-in (and digging through the
> old notes).
The basic approach was to have them *NOT* treated as integer types;
it's SYM_RESTRICT node refering to the node for underlying type.
MOD_CHAR/MOD_LONG/MOD_UNSIGNED/etc. make no more sense for that than
they do for e.g. pointers.
Any operations like ordered comparisons would trigger unrestrict() on
these suckers, which would warn and convert to underlying type.
Your addition of "ordered comparison with 0 or -1" evades unrestrict().
Which shuts the warning up, but that leaves evaluate_compare() with
something unexpected -
if (ctype->ctype.modifiers & MOD_UNSIGNED)
expr->op = modify_for_unsigned(expr->op);
running into SYM_RESTRICT node.
*IF* you want to go that way, I would suggest a new return value for
restricted_binop() ("comparison with magical value"), with
something like
switch (restricted_binop(op, ctype)) {
case 1:
....
default:
break;
case 4:
// comparison with magic value:
// quietly go for underlying type, if not fouled
// if fouled, just return NULL and let the caller
// deal with that - loudly.
if (!(lclass & rclass & TYPE_FOULED))
ctype = ctype->ctype.base_type;
else
ctype = NULL;
}
in restricted_binop_type().
On Wed, Aug 24, 2022 at 04:10:25AM +0100, Al Viro wrote:
> *IF* you want to go that way, I would suggest a new return value for
> restricted_binop() ("comparison with magical value"), with
> something like
> switch (restricted_binop(op, ctype)) {
> case 1:
> ....
>
> default:
> break;
> case 4:
> // comparison with magic value:
> // quietly go for underlying type, if not fouled
> // if fouled, just return NULL and let the caller
> // deal with that - loudly.
> if (!(lclass & rclass & TYPE_FOULED))
if (!((lclass | rclass) & TYPE_FOULED))
that is - we don't need both of them being fouled to trigger a warning;
either one should suffice.
On 8/23/22 18:49, Linus Torvalds wrote:
> That's the
>
>> It does require that kernel change to make
>>
>> #define is_signed_type(type) (((type)(-1)) <= (type)0)
>
> part I was talking about.
>
> So your kernel side patch looks fine, except I don't think you need
> the '__force' - the sparse patches in my tree should make sparse happy
> about casting '-1'.
Thank you for having provided this feedback. If I change the
is_signed_type() definition into the above (no __force), the sparse
warnings shown in my previous email disappear. Now I'm puzzled about
how this is possible. I guess I should take a closer look at the sparse
patches.
Bart.
On 8/23/22 19:11, Linus Torvalds wrote:
> Side note: I think you could move this into '<linux/compiler.h>' and that would
>
> (a) make some conceptual sense (unlike "overflow.h" and "trace_events.h")
>
> and
>
> (b) mean it gets included automatically in both files.
>
> overflow.h already explicitly includes compiler.h, and trace_events.h
> gets it from
>
> linux/ring_buffer.h -> linux/mm.h -> linux/bug.h -> asm/bug.h ->
> linux/compiler.h
>
> (it goes other wats too, but those ones are through arch-specific asm
> headers, so the above is the first non-arch-specific unconditional
> chain I found.
OK, I will look into moving the definition of is_signed_type() into compiler.h.
Thanks,
Bart.
On Tue, Aug 23, 2022 at 8:10 PM Al Viro <[email protected]> wrote:
>
> Any operations like ordered comparisons would trigger unrestrict() on
> these suckers, which would warn and convert to underlying type.
>
> Your addition of "ordered comparison with 0 or -1" evades unrestrict().
No. Look. Try this modification to that test, and use
'./test-linearize' to see what sparse turns it into without my patch
to keep the signedness.
static long test(void)
{
return (le32) 0xffffffff;
}
yes, yes, it warns (twice, actually), but it also then generates
ret.64 $-1
for that return.
Why? Because it thinks that 'le32' is a signed 32-bit thing due to the
clearing of the MOD_UNSIGNED bit, so when it casts it to 'long' it
will sign-extend it.
So the sign confusion exists and is visible regardless of the added
ordered comparison.
Now, we normally don't *notice* any of this, because we obviously
don't rely on sparse generating any code. And we _do_ cast those
bitwise things in many places, although we use "__force" to show that
it's intentional. Including, very much, those kinds of widening casts
where the signedness matters.
See for example very much the csum code:
__wsum csum_partial(const void *buff, int len, __wsum wsum)
{
unsigned int sum = (__force unsigned int)wsum;
which is *exactly* that kind of code where it's fundamentally
important that 'wsum' is an unsigned type, and casting it to 'unsigned
int' does not sign-extend it.
So no. This has absolutely nothing to do with the new ordered comparisons.
Those bitwise types have always been integers, just with special rules
for warning about mis-using them.
And the sign handling has always been wrong.
It just so happens that me using 'test-linearize' to double-check what
sparse does for that signedness check *uncovered* that pre-existing
bug.
It was not introduced by the new code, and the ordered comparisons are
not AT ALL different from the equality comparisons, except for the
fact that they actually care about the signedness being right.
Linus
On 8/23/22 18:49, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 5:09 PM Bart Van Assche <[email protected]> wrote:
>>
>> I'm probably doing something wrong but even with sparse commit 658ee8e0f631
>> ("unrestricted values are unrestricted even after a cast") I see warnings
>> being triggered by users of the is_signed_type() macro, warnings that
>> disappear if I change the definition of the is_signed_type() macro into 0:
>
> That's the
>
>> It does require that kernel change to make
>>
>> #define is_signed_type(type) (((type)(-1)) <= (type)0)
>
> part I was talking about.
>
> So your kernel side patch looks fine, except I don't think you need
> the '__force' - the sparse patches in my tree should make sparse happy
> about casting '-1'.
>
> But I didn't do very much testing.
Hi Linus,
Can you take a look at the following report from the kernel test robot:
https://lore.kernel.org/all/[email protected]/ ?
Do I see correctly that gcc reports a new warning for the above
definition of is_signed_type() with W=1? I'm not sure how to get rid of
that new gcc warning without reintroducing a sparse warning.
The tree that the kernel robot tested is available here:
https://github.com/bvanassche/linux/tree/tracing
Thanks,
Bart.
On Wed, Aug 24, 2022 at 5:30 PM Linus Torvalds
<[email protected]> wrote:
>
> Let me think about this.
Actually, thinking about it, that was simple enough.
-#define is_signed_type(type) (((type)(-1)) < (type)1)
+#define is_signed_type(type) (((type)(-1)) < (__force type)1)
should work.
It looks a bit odd, because we only force one side.
But we only need to force one side, because the '-1' doesn't have any
issues with bitwise types, the same way 0 doesn't.
So only '1' needs to be force-cast to avoid a warning about casting an
integer to a bitwise type.
And since that -1 counts as an unrestricted value after a cast, now
the ordered comparison doesn't warn either.
Now, admittedly I think sparse should also allow a forced cast of an
unrestricted value to be unrestricted, so I think I should do this
static int restricted_value(struct expression *v, struct symbol *type)
{
- if (v->type == EXPR_CAST)
+ if (v->type == EXPR_CAST || v->type = EXPR_FORCE_CAST)
v = v->cast_expression;
in sparse, but even without that the above "is_signed_type()" macro
should make sparse happy (with that current tree of mine).
And since we don't now need to cast 0, gcc won't complain about that
NULL pointer comparison.
Does that solve things for you?
Linus
On Wed, Aug 24, 2022 at 4:28 PM Bart Van Assche <[email protected]> wrote:
>
> Can you take a look at the following report from the kernel test robot:
> https://lore.kernel.org/all/[email protected]/ ?
>
> Do I see correctly that gcc reports a new warning for the above
> definition of is_signed_type() with W=1? I'm not sure how to get rid of
> that new gcc warning without reintroducing a sparse warning.
Indeed. It looks like now gcc recognizes it as a NULL pointer, and
then special-cases that and warns for it.
Oh, how very annoying.
Let me think about this.
Linus
On Thu, Aug 25, 2022 at 12:57 AM Rasmus Villemoes
<[email protected]> wrote:
>
> One can also make the RHS not be a null pointer constant with something like
>
> (((t)(-1)) <= (1 ? (t)0 : (t)0))
Oh Gods.
Let's not go there. I'm sure some version of gcc will figure that out
as being NULL in the end and warn about that too.
I do agree that a comment about the exact choice and why (integers vs
pointers with NULL conversions, and compilers vs sparse) for the
particular syntax would not be misplaced.
Linus
On 25/08/2022 02.40, Linus Torvalds wrote:
> On Wed, Aug 24, 2022 at 5:30 PM Linus Torvalds
> <[email protected]> wrote:
>>
>> Let me think about this.
>
> Actually, thinking about it, that was simple enough.
>
> -#define is_signed_type(type) (((type)(-1)) < (type)1)
> +#define is_signed_type(type) (((type)(-1)) < (__force type)1)
>
> should work.
>
> It looks a bit odd, because we only force one side.
One can also make the RHS not be a null pointer constant with something like
(((t)(-1)) <= (1 ? (t)0 : (t)0))
In either case, we're a good way into "this needs a comment explaining
why it's written precisely the way it is" territory.
Rasmus
On 8/24/22 17:40, Linus Torvalds wrote:
> Actually, thinking about it, that was simple enough.
>
> -#define is_signed_type(type) (((type)(-1)) < (type)1)
> +#define is_signed_type(type) (((type)(-1)) < (__force type)1)
>
> should work.
>
> It looks a bit odd, because we only force one side.
>
> But we only need to force one side, because the '-1' doesn't have any
> issues with bitwise types, the same way 0 doesn't.
>
> So only '1' needs to be force-cast to avoid a warning about casting an
> integer to a bitwise type.
>
> And since that -1 counts as an unrestricted value after a cast, now
> the ordered comparison doesn't warn either.
>
> Now, admittedly I think sparse should also allow a forced cast of an
> unrestricted value to be unrestricted, so I think I should do this
>
> static int restricted_value(struct expression *v, struct symbol *type)
> {
> - if (v->type == EXPR_CAST)
> + if (v->type == EXPR_CAST || v->type = EXPR_FORCE_CAST)
> v = v->cast_expression;
>
> in sparse, but even without that the above "is_signed_type()" macro
> should make sparse happy (with that current tree of mine).
>
> And since we don't now need to cast 0, gcc won't complain about that
> NULL pointer comparison.
>
> Does that solve things for you?
Yes, thank you! No sparse warnings are triggered by the is_signed_type()
macro and the gcc warning about ordered comparison of a pointer with the
null pointer is gone.
The patch I came up with is available below. If nobody picks it up from
this email I will try to find an appropriate kernel maintainer to send
this kernel patch to.
Thanks,
Bart.
From: Bart Van Assche <[email protected]>
Date: Tue, 23 Aug 2022 12:59:25 -0700
Subject: [PATCH] tracing: Define the is_signed_type() macro once
There are two definitions of the is_signed_type() macro: one in
<linux/overflow.h> and a second definition in <linux/trace_events.h>.
As suggested by Linus Torvalds, move the definition of the
is_signed_type() macro into the <linux/compiler.h> header file. Change
the definition of the is_signed_type() macro to make sure that it does
not trigger any sparse warnings with future versions of sparse for
bitwise types. See also:
https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/
Cc: Rasmus Villemoes <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Signed-off-by: Bart Van Assche <[email protected]>
---
include/linux/compiler.h | 6 ++++++
include/linux/overflow.h | 1 -
include/linux/trace_events.h | 2 --
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 01ce94b58b42..7713d7bcdaea 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -239,6 +239,12 @@ static inline void *offset_to_ptr(const int *off)
/* &a[0] degrades to a pointer: a different type from an array */
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
+/*
+ * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
+ * bool and also pointer types.
+ */
+#define is_signed_type(type) (((type)(-1)) < (__force type)1)
+
/*
* This is needed in functions which generate the stack canary, see
* arch/x86/kernel/smpboot.c::start_secondary() for an example.
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f1221d11f8e5..0eb3b192f07a 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -30,7 +30,6 @@
* https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
* credit to Christian Biere.
*/
-#define is_signed_type(type) (((type)(-1)) < (type)1)
#define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
#define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
#define type_min(T) ((T)((T)-type_max(T)-(T)1))
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..8401dec93c15 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -814,8 +814,6 @@ extern int trace_add_event_call(struct trace_event_call *call);
extern int trace_remove_event_call(struct trace_event_call *call);
extern int trace_event_get_offsets(struct trace_event_call *call);
-#define is_signed_type(type) (((type)(-1)) < (type)1)
-
int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
int trace_set_clr_event(const char *system, const char *event, int set);
int trace_array_set_clr_event(struct trace_array *tr, const char *system,
On Thu, Aug 25, 2022 at 10:39:02AM -0700, Bart Van Assche wrote:
> On 8/24/22 17:40, Linus Torvalds wrote:
> > Actually, thinking about it, that was simple enough.
> >
> > -#define is_signed_type(type) (((type)(-1)) < (type)1)
> > +#define is_signed_type(type) (((type)(-1)) < (__force type)1)
> >
> > should work.
> >
> > It looks a bit odd, because we only force one side.
> >
> > But we only need to force one side, because the '-1' doesn't have any
> > issues with bitwise types, the same way 0 doesn't.
> >
> > So only '1' needs to be force-cast to avoid a warning about casting an
> > integer to a bitwise type.
> >
> > And since that -1 counts as an unrestricted value after a cast, now
> > the ordered comparison doesn't warn either.
> >
> > Now, admittedly I think sparse should also allow a forced cast of an
> > unrestricted value to be unrestricted, so I think I should do this
> >
> > static int restricted_value(struct expression *v, struct symbol *type)
> > {
> > - if (v->type == EXPR_CAST)
> > + if (v->type == EXPR_CAST || v->type = EXPR_FORCE_CAST)
> > v = v->cast_expression;
> >
> > in sparse, but even without that the above "is_signed_type()" macro
> > should make sparse happy (with that current tree of mine).
> >
> > And since we don't now need to cast 0, gcc won't complain about that
> > NULL pointer comparison.
> >
> > Does that solve things for you?
>
> Yes, thank you! No sparse warnings are triggered by the is_signed_type()
> macro and the gcc warning about ordered comparison of a pointer with the
> null pointer is gone.
>
> The patch I came up with is available below. If nobody picks it up from
> this email I will try to find an appropriate kernel maintainer to send
> this kernel patch to.
>
> Thanks,
>
> Bart.
>
>
> From: Bart Van Assche <[email protected]>
> Date: Tue, 23 Aug 2022 12:59:25 -0700
> Subject: [PATCH] tracing: Define the is_signed_type() macro once
>
> There are two definitions of the is_signed_type() macro: one in
> <linux/overflow.h> and a second definition in <linux/trace_events.h>.
>
> As suggested by Linus Torvalds, move the definition of the
> is_signed_type() macro into the <linux/compiler.h> header file. Change
> the definition of the is_signed_type() macro to make sure that it does
> not trigger any sparse warnings with future versions of sparse for
> bitwise types. See also:
> https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/
> https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/
>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Kees Cook <[email protected]>
> Signed-off-by: Bart Van Assche <[email protected]>
Looks good to me; thanks!
Acked-by: Kees Cook <[email protected]>
--
Kees Cook