linux-kernel - Re: [PATCH 02/18] fsinfo: Add fsinfo() syscall to query filesystem information [ver #21]

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2041168.1596540881@warthog.procyon.org.uk>
Date:   Tue, 04 Aug 2020 12:34:41 +0100
From:   David Howells <dhowells@...hat.com>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     dhowells@...hat.com, viro@...iv.linux.org.uk,
        linux-api@...r.kernel.org, torvalds@...ux-foundation.org,
        raven@...maw.net, mszeredi@...hat.com, christian@...uner.io,
        jannh@...gle.com, darrick.wong@...cle.com, kzak@...hat.com,
        jlayton@...hat.com, linux-fsdevel@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/18] fsinfo: Add fsinfo() syscall to query filesystem information [ver #21]

Miklos Szeredi <miklos@...redi.hu> wrote:

> > 		__u32	Mth;
> 
> The Mth field seems to be unused in this patchset.  Since the struct is
> extensible, I guess there's no point in adding it now.

Yeah - I was using it to index through the server address lists for network
filesystems (ie. the Mth address of the Nth server), but I've dropped the nfs
patch and made afs return an array of addresses for the Nth server since the
address list can get reordered.

Ordinarily, I'd just take it out, but I don't want to cause the patchset to
get dropped for yet another merge cycle :-/

> > +#define FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO 0x100	/* Information about attr N (for path) */
> > +#define FSINFO_ATTR_FSINFO_ATTRIBUTES	0x101	/* List of supported attrs (for path) */
> 
> I think it would make sense to move the actual attributes to a separate patch
> and leave this just being the infrastructure.

Maybe.  If there are no attributes, then it makes it a bit hard to test.

> > +struct fsinfo_u128 {
> ...
> 
> Shouldn't this belong in <linux/types.h>?

Maybe.  Ideally, I'd use a proper C type rather than a struct.

> Is there a reason these are 128 wide fields?  Are we approaching the limits of
> 64bits?

Dave Chinner was talking at LSF a couple of years ago, IIRC, about looking
beyond the 16 Exa limit in XFS.  I've occasionally talked to people who have
multi-Peta data sets in AFS or whatever they were using, streamed from science
experiments, so the limit isn't necessarily all *that* far off.

> > +struct fsinfo_limits {
> > +	struct fsinfo_u128 max_file_size;	/* Maximum file size */
> > +	struct fsinfo_u128 max_ino;		/* Maximum inode number */
> 
> Again, what's the reason.  AFACT we are not yet worried about overflowing 64
> bits.  Future proofing is good, but there has to be some rules and reasons
> behind the decisions.

This is cheap to do.  This information is expected to be static for the
lifetime a superblock and, for most filesystems, of the running kernel, so
simply copying it with memcpy() from rodata is going to suffice most of the
time.

But don't worry - 640K is sufficient for everyone ;-)

David