Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

From: Al Viro <viro@ZenIV.linux.org.uk>
To: Sergey Klyaus <sergey.m.klyaus@gmail.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Li Wang <liwang@redhat.com>, Andreas Dilger <adilger@dilger.ca>,
	Andi Kleen <andi.kleen@intel.com>
Subject: Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files
Date: Thu, 5 Oct 2017 21:57:24 +0100	[thread overview]
Message-ID: <20171005205724.GJ21978@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20171005183636.20732-1-sergey.m.klyaus@gmail.com>

On Thu, Oct 05, 2017 at 09:36:36PM +0300, Sergey Klyaus wrote:
> compat_statfs64 structure has some 32-bit and some 64-bit fields, so
> 64d2ab32e "vfs: fix put_compat_statfs64() does not handle errors" fixed
> 32-bit overflow checks not being performed, but accidentally enabled
> checks for f_files and f_ffree that are 64-bit and cannot have overflow.
> Now checks for both groups of fields are enabled by different
> conditions.

TBH, the logics in there looks very dubious.  First of all, could somebody
show an architecture where compat_statfs64 would *not* have 32bit f_bsize?

AFAICS, there are only 3 variants of struct compat_statfs64 declaration in
the entire tree:
arch/mips/include/uapi/asm/statfs.h:82:struct compat_statfs64 {
arch/s390/include/asm/compat.h:167:struct compat_statfs64 {
include/uapi/asm-generic/statfs.h:68:struct compat_statfs64 {

mips one has
        __u32   f_bsize;
s390 -
        u32             f_bsize;
and generic -
        __u32 f_bsize;

So what is that if (sizeof... == 4) about?  Before or after the commit in
question - f_blocks is consistently 64bit, f_bsize - 32bit.  IOW, that
commit has turned an obfuscated if (0) into equally obfuscated if (1).

In any case, that thing is supposed to behave like statfs64(2) on matching
32bit host, so what the hell is that EOVERFLOW about, anyway?  ->f_type value
not fitting into 32 bits?  That'd be an fs bug; I could see WARN_ON() on that,
but -EOVERFLOW is bloody odd.  And ->f_namelen exceeding 4Gb is even funnier...

Seriously, the old logics had been complete BS and the only saving grace had
been the fact that it never triggered.  What the hell is f_files and f_ffree
logics about?  Those are 64bit in *all* struct statfs64 variants.  Always
had been.

AFAICS, the real bug here is in hugetlbfs; that's where obscene values in
->f_bsize come from.  IMO all that code in put_compat_statfs64() should be
replaced with
	if (kbuf->bsize != (u32)kbuf->bsize)
		return -EOVERFLOW;
That, or hugetlbfs could be taught to fake saner ->f_bsize (recalculating
->f_bavail/->f_bfree/->f_blocks to go with that).

Comments?