linux-kernel - Re: [PATCH 2/2] string: retire bcmp()

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGudoHGATcA06Q23mCWbz10SSkO8H72jWN2k8gZGgm+h_ogG4Q@mail.gmail.com>
Date: Sat, 23 Nov 2024 20:15:03 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Nathan Chancellor <nathan@...nel.org>
Cc: David Laight <David.Laight@...lab.com>, "tglx@...utronix.de" <tglx@...utronix.de>, 
	"bp@...en8.de" <bp@...en8.de>, "andy@...nel.org" <andy@...nel.org>, 
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH 2/2] string: retire bcmp()

On Sat, Nov 23, 2024 at 8:09 PM Nathan Chancellor <nathan@...nel.org> wrote:
>
> Hi David,
>
> Thanks for the CC.
>
> On Sat, Nov 23, 2024 at 03:13:09PM +0000, David Laight wrote:
> > From: Mateusz Guzik
> > > Sent: 23 November 2024 09:47
> > >
> > > While architectures could override it thanks to __HAVE_ARCH_BCMP, none
> > > of them did. Instead it was implemented as a call to memcmp().
> > >
> > > These routines differ in the API contract: memcmp()'s result indicates
> > > which way the difference goes (making it usable for sorting), whereas
> > > bcmp()'s result merely states whether the buffers differ in any way.
> > >
> > > This means that a dedicated optimized bcmp() is cheaper to execute than
> > > memcmp() for differing buffers as there is no need to compute the return
> > > value.
> > >
> > > However, per the above nobody bothered to write one and it is unclear if
> > > it makes sense to do it.
> > >
> > > Users which really want to compare stuff may want to handle it
> > > differently (like e.g., the path lookup).
> > >
> > > As there are no users and the code is merely a wrapper around memcmp(),
> > > just whack it.
> > >
> > ...
> > >
> > > -/*
> > > - * Clang may lower `memcmp == 0` to `bcmp == 0`.
> > > - */
> > > -int bcmp(const void *s1, const void *s2, size_t len)
> > > -{
> > > -   return memcmp(s1, s2, len);
> > > -}
> > > -
> >
> > As per the comment I thought that clang would sometimes generate
> > calls to bcmp().
> >
> > So while the two symbols could refer to the same code I don't
> > think it can be removed.
>
> Right, commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
> explicitly added bcmp() to lib/string.c because LLVM will emit calls to
> bcmp instead of memcmp in certain circumstances [1], an optimization
> that still exists, thus this patch would trigger new errors at link or
> modpost time:
>
>   ERROR: modpost: "bcmp" [arch/x86/kvm/kvm.ko] undefined!
>   ERROR: modpost: "bcmp" [arch/x86/kvm/kvm-intel.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/quota/quota_v2.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/dlm/dlm.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/netfs/netfs.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/ext4/ext4.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/minix/minix.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/fat/fat.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/isofs/isofs.ko] undefined!
>   ERROR: modpost: "bcmp" [fs/nfs/nfs.ko] undefined!
>   WARNING: modpost: suppressed 254 unresolved symbol warnings because there were too many)
>
>   ld.lld: error: undefined symbol: bcmp
>   >>> referenced by fortify-string.h:715 (include/linux/fortify-string.h:715)
>   >>>               vmlinux.o:(load_pdptrs)
>   >>> referenced by fortify-string.h:715 (include/linux/fortify-string.h:715)
>   >>>               vmlinux.o:(kvm_arch_irqfd_route_changed)
>   >>> referenced by fortify-string.h:715 (include/linux/fortify-string.h:715)
>   >>>               vmlinux.o:(vmx_check_processor_compat)
>   >>> referenced 438 more times
>   >>> did you mean: bacmp
>   >>> defined in: vmlinux.o
>
> Please do not apply this patch. If we need to shore up the comment to
> make this explicit, I am happy to do so.
>
> [1]: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18eedcff13
>

Hi guys, I just tested and indeed clang will convert some of the
memcmp uses into bcmp.

However, given that this is counterproductive in the kernel (it merely
adds a detour through another symbol), the thing to do here is to
convince clang to *not* do it and then whack the symbol

I admit I did not bother checking how bcmp came to be because I assume
it's a leftover -- it was the standard comparison routine back in the
day in the BSD land for example.

-- 
Mateusz Guzik <mjguzik gmail.com>