Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup()

David Howells
2019-12-27T09:10:23Z

Al Viro <viro@zeniv.linux.org.uk> wrote:

> > - int len;
> > + int len = 0;
> >
> > if (!net->ws_cell)
> > return ERR_PTR(-ENOENT);
>
> NAK. This is really, really wrong - passing zero to lookup_one_len() is
> always a bug.

You can't create a cell with a name of "" as afs_alloc_cell() will object with
EINVAL, so if net->ws_cell points to an afs_cell struct, that should never
have a zero-length name.

> BTW, what guarantees that cell->name won't be "@cell"?

afs_alloc_cell() won't allow that a cell with that that name either.

> The same for net->sysnames in afs_lookup_atsys() - what makes sure we won't
> see "@sys" among those?

afs_proc_sysname_write() checks for it. Note that @sys substitutions are set
locally and are not obtained remotely.

> While we are at it,
> d = d_splice_alias(inode, dentry);
> if (!IS_ERR_OR_NULL(d)) {
> d->d_fsdata = dentry->d_fsdata;
> trace_afs_lookup(dvnode, &d->d_name,
> inode ? AFS_FS_I(inode) : NULL);
> } else {
> trace_afs_lookup(dvnode, &dentry->d_name,
> IS_ERR_OR_NULL(inode) ? NULL
> : AFS_FS_I(inode));
> }
> is _very_ suspicious-looking - d_splice_alias() consumes
> an inode reference, and if it ends up failing on non-ERR_PTR()
> inode, the inode will be dropped by the time it returns.
> IOW, that AFS_FS_I(inode) in the second branch can bloody
> well point to already freed memory.

Yeah, fair point. I need to save the fid before calling d_splice_alias().

> Tracepoints: Just Say No...

You can go and argue that one with David Miller if you like.

David