linux-kernel - Re: [PATCH v3] auxdisplay: Remove in

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: <20210216182619.xd7h4uwpqcw5kcup@linutronix.de>
Date:   Tue, 16 Feb 2021 19:26:19 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Willy Tarreau <w@....eu>
Subject: Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

On 2021-02-16 13:42:19 [+0100], Miguel Ojeda wrote:
> It is not so much about documenting the obvious, but about stating
> that 1) the precondition was properly taken into account and that 2)
> nothing non-obvious is undocumented. When code is changed later on, it
> is much more likely assumptions are broken if not documented.

That should be part of the commit message. You can always rewind to the
commit message that introduce something and check if the commit message
made sense or ignored a detail which made it wrong (or so).

> In fact, from a quick git blame, that seems to be what happened here:
> originally the function could be called from a public function
> intended to be used from inside the kernel; so I assume it was the
> intention to allow calls from softirq contexts. Then it was refactored
> and the check never removed. In this case, the extra check is not a
> big deal, but going in the opposite direction can happen too, and then
> we will have a bug.

So it was needed once, it is not needed anymore. That was my arguing in
v1 about. No word about general removing in_interrupt() from drivers.

> In general, when a patch for a fix is needed, it's usually a good idea
> to add a comment right in the code. Even if only to avoid someone else
> having to backtrack the calls to see it is only called form fs_ops
> etc.

This is not a fix. It just removes not needed code. Also I don't think
that this is a good idea to add a comment to avoid someone to backtrack
/ double check something. If you rely on a comment instead of double
checking that you do is indeed correct you will rely one day on a stale
comment and commit to a bug.

To give you another example: If I would have come along and replaced
GFP_ATOMIC with GFP_KERNEL would you ask for a comment?

Anyway, I'm posting a patch with changes as ordered in a jiffy.

> Cheers,
> Miguel

Sebastian