From patchwork Mon Nov 14 10:59:42 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Li Wang X-Patchwork-Id: 9427221 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6829C60476 for ; Mon, 14 Nov 2016 11:06:15 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5917628504 for ; Mon, 14 Nov 2016 11:06:15 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4DE7728772; Mon, 14 Nov 2016 11:06:15 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C78A228504 for ; Mon, 14 Nov 2016 11:06:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752904AbcKNLGN (ORCPT ); Mon, 14 Nov 2016 06:06:13 -0500 Received: from mail-pf0-f171.google.com ([209.85.192.171]:36037 "EHLO mail-pf0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752270AbcKNLGM (ORCPT ); Mon, 14 Nov 2016 06:06:12 -0500 Received: by mail-pf0-f171.google.com with SMTP id 189so29345737pfz.3 for ; Mon, 14 Nov 2016 03:06:12 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=g+9soCMp7VVpOee1dpnzjf0CoYHxJvXFq+6jEfKszBg=; b=Lw/Hr5wxjdEsqpmia54n3b/uOJ1VrDX9y6xnSYEHK12qWBKnsE6veKvXZQV3da8ZWE 7GWGrkLkm3fWpQABfRwt7SmchC9D6xwSLu0rcxeR7WEserUUfrwjdA8AjqWRZQl0g0VI x+K09ZrT6fu6k1THG5EnmQ9WPA+VGnl0TnB1NMykzOt5CWiR9cFAJXKseWKe1uTy96Gz LmAFve3VyYxchDOZQzWpEvDd2fbYG0mk7iEicsJ+tXIRkODgq6PtZ85Wvt6V3sDLpYQy QiVwnevf9Hrr7OvqldatDtjc9H3i44oEZwSgKXafFK7ZsTzoNTS6AJgutzDWuIV39hQ4 NHUg== X-Gm-Message-State: ABUngvfU2zzfGPmRew0aswfxGDZQeZQJj4Rich3XtpYfxh8dDTSfswPeDpP+At4yIrh94Jqs X-Received: by 10.99.124.66 with SMTP id l2mr28146915pgn.116.1479121223561; Mon, 14 Nov 2016 03:00:23 -0800 (PST) Received: from gmail.com ([209.132.188.80]) by smtp.gmail.com with ESMTPSA id x1sm33480829pax.38.2016.11.14.03.00.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Nov 2016 03:00:22 -0800 (PST) Date: Mon, 14 Nov 2016 18:59:42 +0800 From: Li Wang To: Andreas Dilger Cc: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] vfs: fix statfs64() does not handle errors Message-ID: <20161114105942.GA772@gmail.com> References: <1478514077-24855-1-git-send-email-liwang@redhat.com> <6D8E6AD1-CE9D-4578-A508-8FCD4C97C6BE@dilger.ca> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6D8E6AD1-CE9D-4578-A508-8FCD4C97C6BE@dilger.ca> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Nov 07, 2016 at 11:03:11AM -0700, Andreas Dilger wrote: > On Nov 7, 2016, at 3:21 AM, Li Wang wrote: > > > > statfs64() does NOT return -1 and setting errno to EOVERFLOW when some > > variables(like: f_bsize) overflowed in the returned struct. > > > > reproducer: > > step1. mount hugetlbfs with two different pagesize on ppc64 arch. > > > > $ hugeadm --pool-pages-max 16M:0 > > $ hugeadm --create-mount > > $ mount | grep -i hugetlbfs > > none on /var/lib/hugetlbfs/pagesize-16MB type hugetlbfs (rw,relatime,seclabel,pagesize=16777216) > > none on /var/lib/hugetlbfs/pagesize-16GB type hugetlbfs (rw,relatime,seclabel,pagesize=17179869184) > > > > step2. compile & run this C program. > > > > $ cat statfs64_test.c > > > > #define _LARGEFILE64_SOURCE > > #include > > #include > > > > int main() > > { > > struct statfs64 sb; > > int err; > > > > err = statfs64("/var/lib/hugetlbfs/pagesize-16GB", &sb); > > if (err) > > return -1; > > > > printf("sizeof f_bsize = %d, f_bsize=%ld\n", sizeof(sb.f_bsize), sb.f_bsize); > > > > return 0; > > } > > > > $ gcc -m32 statfs64_test.c > > $ ./a.out > > sizeof f_bsize = 4, f_bsize=0 > > > > Signed-off-by: Li Wang > > --- > > > > Notes: > > This is my first patch to kernel fs part, I'm not sure if > > this one useful, but just want someone have a look. > > > > thanks~ > > > > fs/statfs.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/fs/statfs.c b/fs/statfs.c > > index 083dc0a..849dde95 100644 > > --- a/fs/statfs.c > > +++ b/fs/statfs.c > > @@ -151,6 +151,23 @@ static int do_statfs64(struct kstatfs *st, struct statfs64 __user *p) > > if (sizeof(buf) == sizeof(*st)) > > memcpy(&buf, st, sizeof(*st)); > > else { > > + if (sizeof buf.f_bsize == 4) { > > Linux CodingStyle says this should be used like sizeof(buf.f_bsize). agree. > > > + if ((st->f_blocks | st->f_bfree | st->f_bavail | > > + st->f_bsize | st->f_frsize) & > > + 0xffffffff00000000ULL) > > + return -EOVERFLOW; > > I'm not sure I agree with this check. Sure, if sizeof(buf.f_bsize) == 4 > then the large st->f_bsize will overflow this field, and that is valid. After thinking over, I feel that my fix in this patch is not right. The reproducer.c running on ppc64 arch was build in 32bit, but it does not call SYS_statfs64 in kernel. It calls compat_sys_statfs64 indeed. # cat reproducer.c #define _LARGEFILE64_SOURCE #include #include #include int main() { struct statfs64 sb; int err; err = syscall(SYS_statfs64, "/var/lib/hugetlbfs/pagesize-16GB", sizeof(sb), &sb); if (err) return -1; printf("sizeof f_bsize = %d, f_bsize=%ld\n", sizeof(sb.f_bsize), sb.f_bsize); return 0; } # gcc reproducer.c -m32 # stap -e 'probe kernel.function("compat_sys_statfs64") {printf ("%s", $$parms);}' -vvv & # ./a.out sizeof f_bsize = 4, f_bsize=0 # pathname=0x100006c4 sz=0x58 buf=0xff8a20b0 Guess the fix should be like: return -EOVERFLOW; I will test it and send a new patch. Regards, Li Wang > > However, that doesn't mean that large values for f_blocks, f_bfree, f_bavail > should return an error. I assume you are concerned that the product of the > large f_bsize and one of those values would overflow a 64-bit bytes value, > but that is for userspace to worry about, since the values in the individual > fields themselves are OK. > > We're already over 100PiB Lustre filesystems (2^57 bytes) today, and I > don't want statfs() failing prematurely because userspace feels the need > to multiply out the blocks and blocksize into bytes, instead of shifting > the values to KB (which would allow filesystems up to 2^74-1024 bytes to > be handled correctly in userspace). > > > + /* > > + * f_files and f_ffree may be -1; it's okay to stuff > > + * that into 32 bits > > + */ > > + if (st->f_files != -1 && > > + (st->f_files & 0xffffffff00000000ULL)) > > + return -EOVERFLOW; > > > + if (st->f_ffree != -1 && > > + (st->f_ffree & 0xffffffff00000000ULL)) > > + return -EOVERFLOW; > > Why does sizeof(f_bsize) have anything to do with the number of free files? > These two checks are just plain wrong, since f_files and f_ffree are 64-bit > fields in struct statfs64. right. > > Cheers, Andreas > > > + } > > + > > buf.f_type = st->f_type; > > buf.f_bsize = st->f_bsize; > > buf.f_blocks = st->f_blocks; > > -- > > 1.8.3.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Cheers, Andreas > > > > > --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/compat.c b/fs/compat.c index bd064a2..3d923fd 100644 --- a/fs/compat.c +++ b/fs/compat.c @@ -253,7 +253,7 @@ static int put_compat_statfs(struct compat_statfs __user *ubuf, struct kstatfs * static int put_compat_statfs64(struct compat_statfs64 __user *ubuf, struct kstatfs *kbuf) { - if (sizeof ubuf->f_blocks == 4) { + if (sizeof ubuf->f_bsize == 4) { if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail | kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)