linux-kernel - Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5700ac75-f6a9-877e-4011-9b314f12b5ab@acm.org>
Date:   Mon, 22 Aug 2022 11:20:03 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Hellwig <hch@....de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings
 triggered by is_signed_type()

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.