Re: Is it OK to export symbols 'getname' and 'putname'?

[Jan Kara]

On Fri 17-04-15 20:35:30, Boqun Feng wrote:
> Hi Al,
> 
> On Sun, Apr 12, 2015 at 02:13:18AM +0100, Al Viro wrote:
> > 
> > BTW, looking at the __getname() callers...  Lustre one sure as hell looks
> > bogus:
> >         char *tmp = __getname();
> > 
> >         if (!tmp)
> >                 return ERR_PTR(-ENOMEM);
> > 
> >         len = strncpy_from_user(tmp, filename, PATH_MAX);
> >         if (len == 0)
> >                 ret = -ENOENT;
> >         else if (len > PATH_MAX)
> >                 ret = -ENAMETOOLONG;
> > 
> >         if (ret) {
> >                 __putname(tmp);
> >                 tmp =  ERR_PTR(ret);
> >         }
> >         return tmp;
> > 
> > Note that
> > 	* strncpy_from_user(p, u, n) can return a negative (-EFAULT)
> > 	* strncpy_from_user(p, u, n) cannot return a positive greater than
> > n.  In case of missing NUL among the n bytes starting at u (and no faults
> > accessing those) we get exactly n bytes copied and n returned.  In case
> > when NUL is there, we copy everything up to and including that NUL and
> > return number of non-NUL bytes copied.
> > 
> > IOW, these failure cases had never been tested.  Name being too long ends up
> > with non-NUL-terminated array of characters returned, and the very first
> > caller of ll_getname() feeds it to strlen().  Fault ends up with uninitialized
> > array...
> > 
> > AFAICS, the damn thing should just use getname() and quit reinventing the
> > wheel, badly.
> > 
> 
> I'm trying to clean that part of code you mentioned, and I found I have
> to export the symbols 'getname' and 'putname' as follow to replace that
> __getname() caller:
> 
> diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
> index a182019..014f51a 100644
> --- a/drivers/staging/lustre/lustre/llite/dir.c
> +++ b/drivers/staging/lustre/lustre/llite/dir.c
...
> +#define ll_getname(filename) getname(filename)
> +#define ll_putname(name) putname(name)
  Bonus points for getting rid of these useless defines.

> diff --git a/fs/namei.c b/fs/namei.c
> index ffab2e0..7a0948c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -205,6 +205,7 @@ getname(const char __user * filename)
>  {
>  	return getname_flags(filename, 0, NULL);
>  }
> +EXPORT_SYMBOL(getname);
>  
>  struct filename *
>  getname_kernel(const char * filename)
> @@ -254,6 +255,7 @@ void putname(struct filename *name)
>  	} else
>  		__putname(name);
>  }
> +EXPORT_SYMBOL(putname);
>  
>  static int check_acl(struct inode *inode, int mask)
>  {
> 
> 
> 
> Is that a good idea to export these symbols, given that lustre may be
> the only user? 
  Yes, it is a good idea.

								Honza
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/