linux-kernel - Re: [PATCH 1/5] clump_bits: Introduce the for_each_set

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: <X+nLT8bMsKJb7nug@shinobu>
Date:   Mon, 28 Dec 2020 07:10:55 -0500
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Syed Nayyar Waris <syednwaris@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Michal Simek <michal.simek@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        Robert Richter <rrichter@...vell.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amit.kucheria@...durent.com>,
        linux-arch <linux-arch@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux PM list <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 1/5] clump_bits: Introduce the for_each_set_clump macro

On Sun, Dec 27, 2020 at 11:03:06PM +0100, Arnd Bergmann wrote:
> On Sat, Dec 26, 2020 at 7:42 AM Syed Nayyar Waris <syednwaris@...il.com> wrote:
> >
> > This macro iterates for each group of bits (clump) with set bits,
> > within a bitmap memory region. For each iteration, "start" is set to
> > the bit offset of the found clump, while the respective clump value is
> > stored to the location pointed by "clump". Additionally, the
> > bitmap_get_value() and bitmap_set_value() functions are introduced to
> > respectively get and set a value of n-bits in a bitmap memory region.
> > The n-bits can have any size from 1 to BITS_PER_LONG. size less
> > than 1 or more than BITS_PER_LONG causes undefined behaviour.
> > Moreover, during setting value of n-bit in bitmap, if a situation arise
> > that the width of next n-bit is exceeding the word boundary, then it
> > will divide itself such that some portion of it is stored in that word,
> > while the remaining portion is stored in the next higher word. Similar
> > situation occurs while retrieving the value from bitmap.
> >
> > GCC gives warning in bitmap_set_value(): https://godbolt.org/z/rjx34r
> > Add explicit check to see if the value being written into the bitmap
> > does not fall outside the bitmap.
> > The situation that it is falling outside would never be possible in the
> > code because the boundaries are required to be correct before the
> > function is called. The responsibility is on the caller for ensuring the
> > boundaries are correct.
> > The code change is simply to silence the GCC warning messages
> > because GCC is not aware that the boundaries have already been checked.
> > As such, we're better off using __builtin_unreachable() here because we
> > can avoid the latency of the conditional check entirely.
> 
> Didn't the __builtin_unreachable() end up leading to an objtool
> warning about incorrect stack frames for the code path that leads
> into the undefined behavior? I thought I saw a message from the 0day
> build bot about that and didn't expect to see it again after that.
> 
> Can you actually measure any performance difference compared
> to BUG_ON() that avoids the undefined behavior? Practically
> all CPUs from the past 20 years have branch predictors that should
> completely hide measurable overhead from this.
> 
>       Arnd

When I initially recommended using __builtin_unreachable(), I was
anticipating the use of bitmap_set_value() in kernel at large -- so the
possible performance hit from a conditional check was a concern for me.
However, now that we're restricting the scope of bitmap_set_value() to
only the GPIO subsystem, such optimization is no longer a major concern
I feel: gpio-xilinx is the only driver utilizing bitmap_set_value() --
and we know it won't be called in a loop -- so whatever hypothetical
performance hit there might be is inconsequential in the end.

Instead, we should focus on code clarity now. I believe it makes sense
given the new scope of this function to revert back to the earlier
suggestion of passing in and checking the boundary explicitly, and to
remove the __builtin_unreachable() call for now. If bitmap_set_value()
becomes available to the rest of the kernel in the future, we can
reconsider whether or not to use __builtin_unreachable().

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)