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

[David Howells]

Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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