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
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.
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.
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/[email protected]/
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
[email protected]