netdev - Re: [PATCH] net/bluetooth: fix erroneous use of bitmap_from

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, 5 Jun 2022 11:51:48 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Guo Ren <guoren@...nel.org>,
        linux-bluetooth <linux-bluetooth@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-csky@...r.kernel.org,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>
Subject: Re: [PATCH] net/bluetooth: fix erroneous use of bitmap_from_u64()

On Sun, Jun 5, 2022 at 9:34 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> That code shouldn't use DECLARE_BITMAP() at all, it should just use
>
>     struct bdaddr_list_with_flags {
>             ..
>             unsigned long flags;
>     };
>
> and then use '&br_params->flags' when it needs the actual atomic
> 'set_bit()' things and friends,

Actually, I'm not convinced those things should be atomic at all.

*Most* of the accesses to those connection flags seem to be with
hci_dev_lock() held, and the ones that aren't can't possibly depend on
atomicity since those things are currently copied around with random
other "copy bitmaps" functions.

So I think the bluetooth code would actually be much better off with
something like

    /* Bitmask of connection flags */
    enum hci_conn_flags {
        HCI_CONN_FLAG_REMOTE_WAKEUP = 1,
        HCI_CONN_FLAG_DEVICE_PRIVACY = 2,
    };
    typedef u8 hci_conn_flags_t;

instead of playing games with DECLARE_BITMAP() and then using a mix of
atomic set_bit/clear_bit() and random non-atomic "copy bitmap values
around".

This attached patch builds cleanly for me, doing the above. But see a
few comments about those atomicity issues...

And no, it doesn't really need that new "hci_conn_flags_t", and it
could just use "u32" (which is what "current_flags" and
"supported_flags" use in the code), but those structures that contain
those connection flags do seem to have various other byte fields and
it would appear to pack better using just that simple one-byte type.

It *literally* just uses two bits in it, after all, and as mentioned,
the whole atomicity situation right now is very very dubious, so it
seems to make sense to do something like this.

Reactions?

                 Linus

View attachment "patch.diff" of type "text/x-patch" (8713 bytes)