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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20241123190922.GA3314432@thelio-3990X>
Date: Sat, 23 Nov 2024 12:09:22 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: David Laight <David.Laight@...lab.com>
Cc: 'Mateusz Guzik' <mjguzik@...il.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()

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

Cheers,
Nathan