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

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

[Tiezhu Yang]

Fix the following compile warning:

  CC      fs/afs/dynroot.o
fs/afs/dynroot.c: In function ‘afs_dynroot_lookup’:
fs/afs/dynroot.c:117:6: warning: ‘len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  ret = lookup_one_len(name, dentry->d_parent, len);
      ^
fs/afs/dynroot.c:91:6: note: ‘len’ was declared here
  int len;
      ^

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 fs/afs/dynroot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index 7503899..303f712 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -88,7 +88,7 @@ static struct dentry *afs_lookup_atcell(struct dentry *dentry)
 	struct dentry *ret;
 	unsigned int seq = 0;
 	char *name;
-	int len;
+	int len = 0;
 
 	if (!net->ws_cell)
 		return ERR_PTR(-ENOENT);
-- 
2.1.0

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

[Al Viro]

On Thu, Dec 19, 2019 at 09:14:51PM +0800, Tiezhu Yang wrote:
> Fix the following compile warning:
> 
>   CC      fs/afs/dynroot.o
> fs/afs/dynroot.c: In function ‘afs_dynroot_lookup’:
> fs/afs/dynroot.c:117:6: warning: ‘len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   ret = lookup_one_len(name, dentry->d_parent, len);
>       ^
> fs/afs/dynroot.c:91:6: note: ‘len’ was declared here
>   int len;
>       ^
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  fs/afs/dynroot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
> index 7503899..303f712 100644
> --- a/fs/afs/dynroot.c
> +++ b/fs/afs/dynroot.c
> @@ -88,7 +88,7 @@ static struct dentry *afs_lookup_atcell(struct dentry *dentry)
>  	struct dentry *ret;
>  	unsigned int seq = 0;
>  	char *name;
> -	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.  It's not any better than undefined value; if we *can*
get to lookup_one_len() call without other assignments to len, we
are fucked.

As it were, it's a false positive - we have
                if (cell) {
                        len = cell->name_len;
                        memcpy(name, cell->name, len + 1);
                }
upstream of
        if (!cell)
                goto out_n;

        ret = lookup_one_len(name, dentry->d_parent, len);
so we can't reach the call of lookup_one_len() without having
hit the assignment to len.

BTW, what guarantees that cell->name won't be "@cell"?  The
things would get rather interesting in that case...  The same
for net->sysnames in afs_lookup_atsys() - what makes sure
we won't see "@sys" among those?  David?

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.  Tracepoints: Just Say No...

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

[David Howells]

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