Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

From: Dave Chinner <david@fromorbit.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Dave Chinner <dchinner@redhat.com>,
	Chandan Babu R <chandanrlinux@gmail.com>,
	"Darrick J . Wong" <djwong@kernel.org>,
	Allison Henderson <allison.henderson@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	linux-xfs@vger.kernel.org,
	Linux-Next <linux-next@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	noreply@ellerman.id.au
Subject: Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()
Date: Sat, 12 Jun 2021 08:46:38 +1000	[thread overview]
Message-ID: <20210611224638.GT664593@dread.disaster.area> (raw)
In-Reply-To: <CAMuHMdWp3E3QDnbGDcTZsCiQNP3pLV2nXVmtOD7OEQO8P-9egQ@mail.gmail.com>

On Fri, Jun 11, 2021 at 08:55:24AM +0200, Geert Uytterhoeven wrote:
> Hi Dave,
> 
> On Fri, Jun 11, 2021 at 12:02 AM Dave Chinner <david@fromorbit.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@ellerman.id.au
> > > Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-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.

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.

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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com