ERROR: modpost: "__mulodi4" [drivers/block/nbd.ko] undefined!

[nathanchance]

After commit fad7cd3310db ("nbd: add the check to prevent overflow in __nbd_ioctl()") in the block tree, the following error is seen with several different 32-bit configurations:

$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1 LLVM_IAS=1 distclean allmodconfig all
...
ERROR: modpost: "__mulodi4" [drivers/block/nbd.ko] undefined!
...

check_mul_overflow() will ultimately call __builtin_mul_overflow(), where I assume that it is problematic that the operands are long long so LLVM calls to the compiler-rt functions that we do not link against.

cc @kees

[nickdesaulniers]

there's an upstream bug for this (@stephenhines and @kraj are on it): https://llvm.org/pr28629

[stephenhines]

The bad news is that this is probably still very low priority to get fixed. For Android, we just started out by including sub-components of compiler-rt directly with libgcc, and later switched completely to Clang's own builtin library with no libgcc.

[rengolin]

My attempts to reproduce a long long multiplication come out lowered from clang on all Arm 32/64 soft/hard I could come up with, with the only exception being -march=armv6 -mthumb, which calls __aeabi_lmul, not __mulodi4. It's possible they're aliases and that's what the symbol points to? Is this it?

But when I try to link, it just works. clang is calling arm-linux-gnueabi-ld. I'm not trying freestanding, so that may be another reason I can't reproduce?

I think the "low priority" of it is because of the fuzzy nature of the original reports, which had __has_builtin, sanitizers and all sorts of other red herrings that it took too long to narrow down. I don't think this is a "low priority" bug per se, but it seems that it's such a corner case that was always worked around, so no one really bothered to go down the rabbit hole.

With a small enough reproducer, it should be much clearer to find where the problem is and perhaps it's even easy to implement. I'm guessing the pattern is not enabled in Thumb and ends up going to the runtime. It could be a simple case of adding a custom lowering for Thumb...

[nathanchance]

I'll see if I can get a smaller reproducer tomorrow then.

NOTE: This is not just a 32-bit ARM error, I do also see it on 32-bit MIPS.

[nickdesaulniers]

We can probably creduce the kernel easily.

[rengolin]

Thanks! Please attach the reproducer to the original bug, so other people can see it, and we update this if/once we fix it.

[rengolin]

NOTE: This is not just a 32-bit ARM error, I do also see it on 32-bit MIPS.

This sounds like people just didn't get to it because no one cared enough about 32-bits by the time the change was introduced...

[nathanchance]

Reproducer posted: https://llvm.org/pr28629

[nickdesaulniers]

If we can fix clang for ARM and MIPS at least, then we might be able to ship in the kernel:

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5170a630778d..2271a50ba4c0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1388,6 +1388,12 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
                blk_queue_rq_timeout(nbd->disk->queue, 30 * HZ);
 }
 
+// FIXME: https://llvm.org/pr28629
+// https://github.com/ClangBuiltLinux/linux/issues/1438
+#if defined(__clang__) && (defined(__arm__) || (defined(__mips__))
+#define MULODI4_BROKEN
+#endif
+
 /* Must be called with config_lock held */
 static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
                       unsigned int cmd, unsigned long arg)
@@ -1408,8 +1414,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
        case NBD_SET_SIZE:
                return nbd_set_size(nbd, arg, config->blksize);
        case NBD_SET_SIZE_BLOCKS:
+#ifndef MULODI4_BROKEN
                if (check_mul_overflow((loff_t)arg, config->blksize, &bytesize))
                        return -EINVAL;
+#endif
                return nbd_set_size(nbd, bytesize, config->blksize);
        case NBD_SET_TIMEOUT:
                nbd_set_cmd_timeout(nbd, arg);

but with an added version check. Err, that would use bytesize uninitialized. We'd maybe go back to using arg * config->blksize instead. Errr...the kernel patch that exposed this fixes a bug; we'd probably wan't to use a hand written check_mul_overflow that doesn't rely on a builtin.

[nathanchance]

Shouldn't we be working around this issue generically, rather than in that specific call site? Initial idea would be breaking up the COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define into add, sub, and mul then defining the mul one only if we know we are working with a good compiler and architecture combination. See include/linux/overflow.h.

[rengolin]

Quick update: This problem was mostly fixed earlier this month (by @topperc) for x86 and @nickdesaulniers is working to get that propagated to Arm and Mips. It will only fix from now on (clang 14+), leaving previous clangs with the same problem.

We may be able to get this in clang 13 if we can backport the current RC3. No promises, though, as it's very late in the release process. But if it works, we would get 13 with the fix in a a few weeks.

That still leaves all previous clangs (12 and older) unfixed (we don't backport beyond six months). How the kernel is going to fix for those older versions is up to you guys.

[nickdesaulniers]

Some toolchain patches have gone in:
32b ARM: https://reviews.llvm.org/rG5c91b98c5d45243352bf10262454bcac77cd3fed
32b MIPS 1: https://reviews.llvm.org/rGc8c176d999d2c2166dee8d67c32b4f8f42b4f1bd

MIPS is now hitting __multi3 being undefined. @rengolin mentioned that perhaps the above 2 could be cherry picked to clang-13, but that would depend on https://reviews.llvm.org/rGb2ca4dc935859b324fd7e9ca804160913a7468a5 and @tstellar + @topperc 's feedback). Since we need to work around this in the kernel anyways, I'm not interested in rushing this, unless the clang-13 release is perhaps still "far" away (like a month).

[topperc]

Is 32-bit X86 still broken for __mulodi4?

[rengolin]

It seems so. In your original patch you only add it for 128-bits, not 64-bits. However, further up the X86Lowering code, ISD::SMULO is legal (custom lowered) for MVT::i64 and it doesn't seem to be conditional on 32/64 bits.

But when I compile the original code with -target i386 I still get the __mulodi4 call. Perhaps the X86 back-end needs the same handling for RTLIB on 64bits?

[topperc]

It seems so. In your original patch you only add it for 128-bits, not 64-bits. However, further up the X86Lowering code, ISD::SMULO is legal (custom lowered) for MVT::i64 and it doesn't seem to be conditional on 32/64 bits.

But when I compile the original code with -target i386 I still get the __mulodi4 call. Perhaps the X86 back-end needs the same handling for RTLIB on 64bits?

Isn't it in this for loop that has a continue for i64 on 32-bit targets?

  for (auto VT : { MVT::i8, MVT::i16, MVT::i32, MVT::i64 }) {                                                                                                                                                                                                
    if (VT == MVT::i64 && !Subtarget.is64Bit())                                                                                                                                                                                                              
      continue;                                                                                                                                                                                                                                              
    // Add/Sub/Mul with overflow operations are custom lowered.                                                                                                                                                                                              
    setOperationAction(ISD::SADDO, VT, Custom);                                                                                                                                                                                                              
    setOperationAction(ISD::UADDO, VT, Custom);                                                                                                                                                                                                              
    setOperationAction(ISD::SSUBO, VT, Custom);                                                                                                                                                                                                              
    setOperationAction(ISD::USUBO, VT, Custom);                                                                                                                                                                                                              
    setOperationAction(ISD::SMULO, VT, Custom);                                                                                                                                                                                                              
    setOperationAction(ISD::UMULO, VT, Custom);      

[rengolin]

Isn't it in this for loop that has a continue for i64 on 32-bit targets?

aha! Yes, I missed that. Been looking at this from different computers and mobiles, lacking context on all of them.

Not sure the kernel cares about i386 anymore but with your legalisation fix, it seems the same thing would work just like Arm and Mips.

[topperc]

Isn't it in this for loop that has a continue for i64 on 32-bit targets?

aha! Yes, I missed that. Been looking at this from different computers and mobiles, lacking context on all of them.

Not sure the kernel cares about i386 anymore but with your legalisation fix, it seems the same thing would work just like Arm and Mips.

Are we heading in the direction of removing MULO lib calls completely from llvm? My initial patch was just to fix the cases that wouldn’t link with either libgcc or compiler-rt.

[rengolin]

Are we heading in the direction of removing MULO lib calls completely from llvm? My initial patch was just to fix the cases that wouldn’t link with either libgcc or compiler-rt.

Maybe? I'm not advocating for it, I'm just trying to fix an issue that exists for over 5 years and still doesn't have a nice solution.

Linking compiler-rt isn't mandatory and not used on many systems. Linking with compiler-rt and libgcc won't work, so if there is functionality that is only available in compiler-rt, we can't know if the linker will find it or not, so lowering calls to __mulodi4 is bound to fail on many systems.

The current legalisation is un-optimised, but Compiler-RT's implementation is plain-C for all platforms, so not the most optimised either. If we can lower the builtin with the compiler and avoid the linking errors on anything that doesn't use compiler-rt, then I think it's a good compromise.

Even if libgcc implements it, the kernel and embedded software won't have it, so are bound to have the same problem. That's why they use __has_builtin all over the place and if we say we have it, then we better implement it. But we don't, we belay that for compiler-rt, which they don't link against.

I don't think eliding the calls as a target option is the best option, but right now it's the only option. I don't really know how Clang can emit code that it knows compiler-rt implements when it doesn't know if it will be linked against it.

There was a discussion many years ago to split builtins from compiler-rt and make it so clang always linked against that. It would obviously clash with symbols from libgcc and other RT libs, but we could mark them weak or some other linkage magic that escapes me now.

Perhaps we need some more thorough discussion on the list...

[kraj]

for compatibility reasons I think its best to remove it from clang.

[nickdesaulniers]

But when I compile the original code with -target i386 I still get the __mulodi4 call. Perhaps the X86 back-end needs the same handling for RTLIB on 64bits?

Not sure the kernel cares about i386 anymore but with your legalisation fix, it seems the same thing would work just like Arm and Mips.

Yes, this is broken for 32b x86 kernels as well (we just don't have CI coverage for ARCH=i386 defconfig + CONFIG_BLK_DEV_NBD=y). If I enable CONFIG_BLK_DEV_NBD=y, then I observe the following linkage failure:

ld.lld: error: undefined symbol: __mulodi4

[nickdesaulniers]

mips patch 2/2: https://reviews.llvm.org/D108926
i386 patch: https://reviews.llvm.org/D108928

For future travelers, llvm/include/llvm/IR/RuntimeLibcalls.def has the relevant mapping for these functions.

[nickdesaulniers]

https://lore.kernel.org/lkml/[email protected]/is the latest kernel patch in this saga.

[nickdesaulniers]

accepted: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=block-5.15&id=41e76c6a3c83c85e849f10754b8632ea763d9be4

(once this lands in mainline, I'll need to chase this into stable, then re-enable CONFIG_BLK_DEV_NBD=y in ACK)

[nickdesaulniers]

41e76c6 is in v5.15-rc4. GKH pulled this into stable 5.14.