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
| ||
|
Date: Sat, 12 Jun 2021 10:54:23 +0200 From: Geert Uytterhoeven <geert@...ux-m68k.org> To: Dave Chinner <david@...morbit.com> Cc: Dave Chinner <dchinner@...hat.com>, Chandan Babu R <chandanrlinux@...il.com>, "Darrick J . Wong" <djwong@...nel.org>, Allison Henderson <allison.henderson@...cle.com>, Christoph Hellwig <hch@....de>, linux-xfs@...r.kernel.org, Linux-Next <linux-next@...r.kernel.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, noreply@...erman.id.au Subject: Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs() Hi Dave, On Sat, Jun 12, 2021 at 12:46 AM Dave Chinner <david@...morbit.com> wrote: > On Fri, Jun 11, 2021 at 08:55:24AM +0200, Geert Uytterhoeven wrote: > > On Fri, Jun 11, 2021 at 12:02 AM Dave Chinner <david@...morbit.com> wrote: > > > On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote: > > > > On 32-bit (e.g. m68k): > > > > > > > > ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined! > > > > > > > > Fix this by using a uint32_t intermediate, like before. > > > > > > > > Reported-by: noreply@...erman.id.au > > > > Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log") > > > > Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org> > > > > --- > > > > Compile-tested only. > > > > --- > > > > fs/xfs/xfs_log.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > <sigh> > > > > > > 64 bit division on 32 bit platforms is still a problem in this day > > > and age? > > > > They're not a problem. But you should use the right operations from > > <linux/math64.h>, iff you really need these expensive operations. > > See, that's the whole problem. This *isn't* obviously a 64 bit > division - BBTOB is shifting the variable down by 9 (bytes to blocks) > and then using that as the divisor. BTOBB, not BBTOB ;-) > The problem is that BBTOB has an internal cast to a 64 bit size, > and roundup() just blindly takes it and hence we get non-obvious > compile errors only on 32 bit platforms. Indeed. Perhaps the macros should be fixed? #define BBSHIFT 9 #define BBSIZE (1<<BBSHIFT) #define BBMASK (BBSIZE-1) #define BTOBB(bytes) (((__u64)(bytes) + BBSIZE - 1) >> BBSHIFT) #define BTOBBT(bytes) ((__u64)(bytes) >> BBSHIFT) Why are these two casting bytes to u64? The result will be smaller due to the shift. if the type holding bytes was too small, you're screwed anyway. #define BBTOB(bbs) ((bbs) << BBSHIFT) Why does this one lack the cast? If the passed bbs is ever 32-bit, it may overflow due to the shift. > We have type checking macros for all sorts of generic functionality > - why haven't these generic macros that do division also have type > checking to catch this? i.e. so that when people build kernels on > 64 bit machines find out that they've unwittingly broken 32 bit > builds the moment they use roundup() and/or friends incorrectly? > > That would save a lot of extra work having fix crap like this up > after the fact... While adding checks would work for e.g. roundup(), it wouldn't work for plain divisions not involving rounding, as we don't have a way to catch this for "a / b", except for the link error on 32-bit platforms. Perhaps the build bots are not monitoring linux-xfs? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Powered by blists - more mailing lists