linux-kernel - Re: [PATCH v1 1/3] vt: keyboard, use GENMAASK()/BIT() macros instead of open coded variants

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:   Mon, 9 Nov 2020 10:57:20 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        David Laight <David.Laight@...lab.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v1 1/3] vt: keyboard, use GENMAASK()/BIT() macros instead
 of open coded variants

On 06. 11. 20, 17:06, Andy Shevchenko wrote:
> On Fri, Nov 6, 2020 at 5:35 PM David Laight <David.Laight@...lab.com> wrote:
>>
>> From: Andy Shevchenko
>>> Sent: 06 November 2020 14:36
>>>
>>> There are few places when GENMASK() or BIT() macro is suitable and makes code
>>> easier to understand.
>>>
>> ...
>>> -     if ((d & ~0xff) == BRL_UC_ROW) {
>>> -             if ((ch & ~0xff) == BRL_UC_ROW)
>>> +     if ((d & ~GENMASK(7, 0)) == BRL_UC_ROW) {
>>> +             if ((ch & ~GENMASK(7, 0)) == BRL_UC_ROW)
>>>                        return d | ch;
>>
>> Do you really think that makes it more readable?
> 
> Yes. Because this tells explicitly how many bits are used for metadata
> vs. data.

No, because ~0xff is clearly what it is. GENMASK(7, 0) is:
1) longer to read & parse by brain with result: "GENMASK undefined"
2) terrible in this particular use case

Another instance of an even worse switch:
-		if (arg & ~0x77)
+		if (arg & ~(GENMASK(6, 4) | GENMASK(2, 0)))

OTOH, the switch to BIT is legit in all cases except the comparisons 
with keycode:
-	if (keycode > 127)
+	if (keycode >= BIT(7))
-		if (keycode < 128) {
+		if (keycode < BIT(7)) {

That's horrid and non-sense too.

sorry,
-- 
js
suse labs