[bug report] NFS: Support statx_get and statx_set ioctls

[bug report] NFS: Support statx_get and statx_set ioctls

[Dan Carpenter]

Hello Richard Sharpe,

This is a semi-automatic email about new static checker warnings.

The patch bc66f6805766: "NFS: Support statx_get and statx_set ioctls" 
from Dec 27, 2021, leads to the following Smatch complaint:

    fs/nfs/nfs4proc.c:8035 _nfs4_set_nfs4_statx()
    error: we previously assumed 'statx' could be null (see line 8026)

fs/nfs/nfs4proc.c
  8025	
  8026		if (statx && (statx->fa_valid[0] & NFS_FA_VALID_SIZE)) {
                    ^^^^^
The patch adds checks for NULL

  8027			sattr.ia_valid |= ATTR_SIZE;
  8028			sattr.ia_size = statx->fa_size;
  8029		}
  8030	
  8031		nfs4_stateid_copy(&arg.stateid, &zero_stateid);
  8032	
  8033		status = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 1);
  8034		if (!status) {
  8035			if (statx->fa_valid[0] & statx_win) {
                            ^^^^^
and unchecked dereferences

  8036				struct nfs_inode *nfsi = NFS_I(inode);
  8037	

regards,
dan carpenter

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Christoph Hellwig]

On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> Hello Richard Sharpe,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch bc66f6805766: "NFS: Support statx_get and statx_set ioctls" 
> from Dec 27, 2021, leads to the following Smatch complaint:

Yikes, how did that crap get merged?  Why the f**k does a remote file
system need to duplicate stat?  This kind of stuff needs a proper
discussion on linux-fsdevel.

And btw, the commit message is utter nonsense.

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Christian Brauner]

On Tue, Jan 11, 2022 at 12:48:14AM -0800, Christoph Hellwig wrote:
> On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> > Hello Richard Sharpe,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch bc66f6805766: "NFS: Support statx_get and statx_set ioctls" 
> > from Dec 27, 2021, leads to the following Smatch complaint:
> 
> Yikes, how did that crap get merged?  Why the f**k does a remote file
> system need to duplicate stat?  This kind of stuff needs a proper
> discussion on linux-fsdevel.
> 
> And btw, the commit message is utter nonsense.

That's not in mainline though, right? At least I don't see it there
looking for this commit. 

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Trond Myklebust]

On Tue, 2022-01-11 at 00:48 -0800, Christoph Hellwig wrote:
> On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> > Hello Richard Sharpe,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch bc66f6805766: "NFS: Support statx_get and statx_set
> > ioctls" 
> > from Dec 27, 2021, leads to the following Smatch complaint:
> 
> Yikes, how did that crap get merged?  Why the f**k does a remote file
> system need to duplicate stat?  This kind of stuff needs a proper
> discussion on linux-fsdevel.
> 
> And btw, the commit message is utter nonsense.

So firstly, there already has been a discussion of this on linux-
fsdevel (and linux-kernel):
https://lore.kernel.org/lkml/20160429125813.23636.49830.stgit@warthog.procyon.org.uk/
and the consensus at the time was that these attributes were not needed
in statx.

The other issue is that this is not a duplicate of stat. It's adding
support for both _setting_ and reading back these attributes. The
ability to supporting reading the archive bit / backup time, for
example, is completely useless unless you can also set those values
after backing up the file. Right now, there is no support in the VFS
for setting any attributes that are not part of the standard POSIX set.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[hch]

On Tue, Jan 11, 2022 at 06:08:27PM +0000, Trond Myklebust wrote:
> So firstly, there already has been a discussion of this on linux-
> fsdevel (and linux-kernel):
> https://lore.kernel.org/lkml/20160429125813.23636.49830.stgit@warthog.procyon.org.uk/
> and the consensus at the time was that these attributes were not needed
> in statx.

And apparently someone now has a different opinion, so we should restart
it.

> 
> The other issue is that this is not a duplicate of stat. It's adding
> support for both _setting_ and reading back these attributes. The
> ability to supporting reading the archive bit / backup time, for
> example, is completely useless unless you can also set those values
> after backing up the file. Right now, there is no support in the VFS
> for setting any attributes that are not part of the standard POSIX set.

Either way this has no business being private in NFS.  We need to
come to an agreement to have a proper interface for CIFS, fat, exfat
and the 2 ntfs drivers as welŀ.  With the first three being the most
important ones.

So big fat nack for stashing something like this into nfs without proper
review and coordination.

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Amir Goldstein]

On Wed, Jan 12, 2022 at 4:10 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> > Hello Richard Sharpe,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch bc66f6805766: "NFS: Support statx_get and statx_set ioctls"
> > from Dec 27, 2021, leads to the following Smatch complaint:
>
> Yikes, how did that crap get merged?

Did it? The bots are scanning through patches on ML:

https://lore.kernel.org/linux-nfs/20211227190504.309612-1-trondmy@kernel.org/

> Why the f**k does a remote file system need to duplicate stat?
> This kind of stuff needs a proper discussion on linux-fsdevel.

+ntfs3 +linux-cifs +linux-api

The proposal of statx_get() is very peculiar.
statx() was especially designed to be extended and accommodate
a diversity of filesystem attributes.

Moreover, NFSv4 is not the only fs that supports those extra attributes.
ntfs3 supports set/get of dos attrib bits via xattr SYSTEM_NTFS_ATTRIB.
cifs support set/get of CIFS_XATTR_ATTRIB and CIFS_XATTR_CREATETIME.

Not only that, but Linux now has ksmbd which actually emulates
those attributes on the server side (like samba) by storing a samba
formatted blob in user.DOSATTRIB xattr.
It should have a way to get/set them on filesystems that support them
natively.

The whole thing shouts for standardization.

Samba should be able to get/set the extra attributes by statx() and
ksmbd should be able to get them from the filesystem by vfs_getattr().

WRT statx_set(), standardization is also in order, both for userspace
API and for vfs API to be used by ksmbd and nfsd v4.

The new-ish vfs API fileattr_get/set() comes to mind when considering
a method to set the dos attrib bits.
Heck, FS_NODUMP_FL is the same as FILE_ATTRIBUTE_ARCHIVE.
That will also make it easy for filesystems that support the fileattr flags
to add support for FS_SYSTEM_FL, FS_HIDDEN_FL.

There is a use case for that. It can be inferred from samba config options
"map hidden/system/archive" that are used to avoid the cost of getxattr
per file during a "readdirplus" query. I recently quantified this cost on a
standard file server and it was very high.

Which leaves us with an API to set the 'time backup' attribute, which
is a "mutable creation time" [*].
cifs supports setting it via setxattr and I guess ntfs3 could use an
API to set it as well.

One natural interface that comes to mind is:

struct timespec times[3] = {/* atime, mtime, crtime */}
utimensat(dirfd, path, times, AT_UTIMES_ARCHIVE);

and add ia_crtime with ATTR_CRTIME to struct iattr.

Trond,

Do you agree to rework your patches in this direction?
Perhaps as the first stage, just use statx() and ioctls to set the
attributes to give enough time for bikeshedding the set APIs
and follow up with the generic set API patches later?

Thanks,
Amir.

[*] I find it convenient to use the statx() terminology of "btime"
to refer to the immutable birth time provided by some filesystems
and to use "crtime" for the mutable creation time for archiving,
so that at some point, some filesystems may provide both of
these times independently.

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Darrick J. Wong]

On Wed, Jan 12, 2022 at 09:57:58AM +0200, Amir Goldstein wrote:
> On Wed, Jan 12, 2022 at 4:10 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> > > Hello Richard Sharpe,
> > >
> > > This is a semi-automatic email about new static checker warnings.
> > >
> > > The patch bc66f6805766: "NFS: Support statx_get and statx_set ioctls"
> > > from Dec 27, 2021, leads to the following Smatch complaint:
> >
> > Yikes, how did that crap get merged?
> 
> Did it? The bots are scanning through patches on ML:
> 
> https://lore.kernel.org/linux-nfs/20211227190504.309612-1-trondmy@kernel.org/
> 
> > Why the f**k does a remote file system need to duplicate stat?
> > This kind of stuff needs a proper discussion on linux-fsdevel.
> 
> +ntfs3 +linux-cifs +linux-api
> 
> The proposal of statx_get() is very peculiar.
> statx() was especially designed to be extended and accommodate
> a diversity of filesystem attributes.
> 
> Moreover, NFSv4 is not the only fs that supports those extra attributes.
> ntfs3 supports set/get of dos attrib bits via xattr SYSTEM_NTFS_ATTRIB.
> cifs support set/get of CIFS_XATTR_ATTRIB and CIFS_XATTR_CREATETIME.
> 
> Not only that, but Linux now has ksmbd which actually emulates
> those attributes on the server side (like samba) by storing a samba
> formatted blob in user.DOSATTRIB xattr.
> It should have a way to get/set them on filesystems that support them
> natively.
> 
> The whole thing shouts for standardization.
> 
> Samba should be able to get/set the extra attributes by statx() and
> ksmbd should be able to get them from the filesystem by vfs_getattr().
> 
> WRT statx_set(), standardization is also in order, both for userspace

So, uh, this is the first I've heard of statx_set in years.

I'm glad to hear that standardizing FSGETFLAGS/FSSETXATTR/etc is still
alive. :)

> API and for vfs API to be used by ksmbd and nfsd v4.
> 
> The new-ish vfs API fileattr_get/set() comes to mind when considering
> a method to set the dos attrib bits.
> Heck, FS_NODUMP_FL is the same as FILE_ATTRIBUTE_ARCHIVE.
> That will also make it easy for filesystems that support the fileattr flags
> to add support for FS_SYSTEM_FL, FS_HIDDEN_FL.
> 
> There is a use case for that. It can be inferred from samba config options
> "map hidden/system/archive" that are used to avoid the cost of getxattr
> per file during a "readdirplus" query. I recently quantified this cost on a
> standard file server and it was very high.
> 
> Which leaves us with an API to set the 'time backup' attribute, which
> is a "mutable creation time" [*].
> cifs supports setting it via setxattr and I guess ntfs3 could use an
> API to set it as well.
> 
> One natural interface that comes to mind is:
> 
> struct timespec times[3] = {/* atime, mtime, crtime */}
> utimensat(dirfd, path, times, AT_UTIMES_ARCHIVE);
> 
> and add ia_crtime with ATTR_CRTIME to struct iattr.
> 
> Trond,
> 
> Do you agree to rework your patches in this direction?
> Perhaps as the first stage, just use statx() and ioctls to set the
> attributes to give enough time for bikeshedding the set APIs
> and follow up with the generic set API patches later?
> 
> Thanks,
> Amir.
> 
> [*] I find it convenient to use the statx() terminology of "btime"
> to refer to the immutable birth time provided by some filesystems
> and to use "crtime" for the mutable creation time for archiving,
> so that at some point, some filesystems may provide both of
> these times independently.

I disagree because XFS and ext4 both use 'crtime' for the immutable
birth time, not a mutable creation time for archiving.  I think we'd
need to be careful about wording here if there is interest in adding a
user-modifiable file creation time (as opposed to creation time for a
specific instance of an inode) to filesystems.

Once a year or so we get a question/complaint from a user about how they
can't change the file creation time and we have to explain to them
what's really going on.

--D

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Amir Goldstein]

> > Which leaves us with an API to set the 'time backup' attribute, which
> > is a "mutable creation time" [*].
> > cifs supports setting it via setxattr and I guess ntfs3 could use an
> > API to set it as well.
> >
> > One natural interface that comes to mind is:
> >
> > struct timespec times[3] = {/* atime, mtime, crtime */}
> > utimensat(dirfd, path, times, AT_UTIMES_ARCHIVE);
> >
> > and add ia_crtime with ATTR_CRTIME to struct iattr.
> >
> > Trond,
> >
> > Do you agree to rework your patches in this direction?
> > Perhaps as the first stage, just use statx() and ioctls to set the
> > attributes to give enough time for bikeshedding the set APIs
> > and follow up with the generic set API patches later?
> >
> > Thanks,
> > Amir.
> >
> > [*] I find it convenient to use the statx() terminology of "btime"
> > to refer to the immutable birth time provided by some filesystems
> > and to use "crtime" for the mutable creation time for archiving,
> > so that at some point, some filesystems may provide both of
> > these times independently.
>
> I disagree because XFS and ext4 both use 'crtime' for the immutable
> birth time, not a mutable creation time for archiving.  I think we'd
> need to be careful about wording here if there is interest in adding a
> user-modifiable file creation time (as opposed to creation time for a
> specific instance of an inode) to filesystems.
>
> Once a year or so we get a question/complaint from a user about how they
> can't change the file creation time and we have to explain to them
> what's really going on.
>

To add one more terminology to the mix - when Samba needed to cope
with these two terminologies they came up with itime for "instantiation time"
(one may also consider it "immutable time").

Another issue besides wording, is that statx btime can be either of those
things depending on the filesystem, so if we ever add mutable btime to
ext4/xfs, what's statx btime going to return?

One more question to ask, if we were to add mutable btime to ext4/xfs
should it be an additional attribute at all or should we allow with explicit
filesystem flag and maybe also mount option to modify the existing crtime
inode field? if we can accept that some users are willing to trade the
immutable crtime with mutable btime, then we can settle with a flag
indicating "warranty seal removed" from the existing crtime field.
At least one advantage of this approach is that it simplifies terminology.

Thanks,
Amir.

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Jeremy Allison]

On Thu, Jan 13, 2022 at 05:52:40AM +0200, Amir Goldstein wrote:
>
>To add one more terminology to the mix - when Samba needed to cope
>with these two terminologies they came up with itime for "instantiation time"
>(one may also consider it "immutable time").

No, that's not what itime is. It's used as the basis
for the fileid return as MacOSX clients insist on no-reuse
of inode numbers when a file is deleted then re-created,
and ext4 will re-use the same inode.

Samba uses btime for "birth time", and will use statx
to get it from the filesystem but then store it in
the dos.attribute EA so it can be modified if the
client sets it.

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Trond Myklebust]

On Wed, 2022-01-12 at 22:30 -0800, Jeremy Allison wrote:
> On Thu, Jan 13, 2022 at 05:52:40AM +0200, Amir Goldstein wrote:
> > 
> > To add one more terminology to the mix - when Samba needed to cope
> > with these two terminologies they came up with itime for
> > "instantiation time"
> > (one may also consider it "immutable time").
> 
> No, that's not what itime is. It's used as the basis
> for the fileid return as MacOSX clients insist on no-reuse
> of inode numbers when a file is deleted then re-created,
> and ext4 will re-use the same inode.

So basically it serves more or less the same purpose as the generation
counter that most Linux filesystems use in the filehandle to provide
similar only-once semantics?

> 
> Samba uses btime for "birth time", and will use statx
> to get it from the filesystem but then store it in
> the dos.attribute EA so it can be modified if the
> client sets it.

Right. That appears to be a difference between Windows and Linux. In
most Linux filesystems, the btime is set by the filesystem at file
creation time, however Windows allows it to be set by the application,
presumably for the purpose of backup/restore?

NFSv4 supports both modes for the btime.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Jeremy Allison]

On Thu, Jan 13, 2022 at 02:58:19PM +0000, Trond Myklebust wrote:
>On Wed, 2022-01-12 at 22:30 -0800, Jeremy Allison wrote:
>> On Thu, Jan 13, 2022 at 05:52:40AM +0200, Amir Goldstein wrote:
>> >
>> > To add one more terminology to the mix - when Samba needed to cope
>> > with these two terminologies they came up with itime for
>> > "instantiation time"
>> > (one may also consider it "immutable time").
>>
>> No, that's not what itime is. It's used as the basis
>> for the fileid return as MacOSX clients insist on no-reuse
>> of inode numbers when a file is deleted then re-created,
>> and ext4 will re-use the same inode.
>
>So basically it serves more or less the same purpose as the generation
>counter that most Linux filesystems use in the filehandle to provide
>similar only-once semantics?

Kind of, although we moved it recently to be
a current_time + random skew as the timestamp
resolution in ext4 just wasn't enough to get us
unique fileids.

Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Trond Myklebust]

On Wed, 2022-01-12 at 09:57 +0200, Amir Goldstein wrote:
> On Wed, Jan 12, 2022 at 4:10 AM Christoph Hellwig <hch@infradead.org>
> wrote:
> > 
> > On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> > > Hello Richard Sharpe,
> > > 
> > > This is a semi-automatic email about new static checker warnings.
> > > 
> > > The patch bc66f6805766: "NFS: Support statx_get and statx_set
> > > ioctls"
> > > from Dec 27, 2021, leads to the following Smatch complaint:
> > 
> > Yikes, how did that crap get merged?
> 
> Did it? The bots are scanning through patches on ML:
> 
> https://lore.kernel.org/linux-nfs/20211227190504.309612-1-trondmy@kernel.org/
> 
> > Why the f**k does a remote file system need to duplicate stat?
> > This kind of stuff needs a proper discussion on linux-fsdevel.
> 
> +ntfs3 +linux-cifs +linux-api
> 
> The proposal of statx_get() is very peculiar.
> statx() was especially designed to be extended and accommodate
> a diversity of filesystem attributes.
> 
> Moreover, NFSv4 is not the only fs that supports those extra
> attributes.
> ntfs3 supports set/get of dos attrib bits via xattr
> SYSTEM_NTFS_ATTRIB.
> cifs support set/get of CIFS_XATTR_ATTRIB and CIFS_XATTR_CREATETIME.
> 
> Not only that, but Linux now has ksmbd which actually emulates
> those attributes on the server side (like samba) by storing a samba
> formatted blob in user.DOSATTRIB xattr.
> It should have a way to get/set them on filesystems that support them
> natively.
> 
> The whole thing shouts for standardization.
> 
> Samba should be able to get/set the extra attributes by statx() and
> ksmbd should be able to get them from the filesystem by
> vfs_getattr().
> 
> WRT statx_set(), standardization is also in order, both for userspace
> API and for vfs API to be used by ksmbd and nfsd v4.
> 
> The new-ish vfs API fileattr_get/set() comes to mind when considering
> a method to set the dos attrib bits.
> Heck, FS_NODUMP_FL is the same as FILE_ATTRIBUTE_ARCHIVE.
> That will also make it easy for filesystems that support the fileattr
> flags
> to add support for FS_SYSTEM_FL, FS_HIDDEN_FL.
> 
> There is a use case for that. It can be inferred from samba config
> options
> "map hidden/system/archive" that are used to avoid the cost of
> getxattr
> per file during a "readdirplus" query. I recently quantified this
> cost on a
> standard file server and it was very high.
> 
> Which leaves us with an API to set the 'time backup' attribute, which
> is a "mutable creation time" [*].
> cifs supports setting it via setxattr and I guess ntfs3 could use an
> API to set it as well.
> 
> One natural interface that comes to mind is:
> 
> struct timespec times[3] = {/* atime, mtime, crtime */}
> utimensat(dirfd, path, times, AT_UTIMES_ARCHIVE);
> 
> and add ia_crtime with ATTR_CRTIME to struct iattr.
> 
> Trond,
> 
> Do you agree to rework your patches in this direction?
> Perhaps as the first stage, just use statx() and ioctls to set the
> attributes to give enough time for bikeshedding the set APIs
> and follow up with the generic set API patches later?
> 
> Thanks,
> Amir.
> 
> [*] I find it convenient to use the statx() terminology of "btime"
> to refer to the immutable birth time provided by some filesystems
> and to use "crtime" for the mutable creation time for archiving,
> so that at some point, some filesystems may provide both of
> these times independently.

I'll give it a shot. I've asked Anna to remove these patches from the
pull request for this merge Window, so I'll try to rework the retrieval
side using statx() and then work on a statx_set() API to enable setting
of these new variables.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com