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]

Date:   Sun, 21 Aug 2022 15:55:33 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.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>,
        Bart Van Assche <bvanassche@....org>
Subject: Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings
 triggered by is_signed_type()

On Sun, 21 Aug 2022 11:35:29 -0700
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <rostedt@...dmis.org> 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