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

[Boqun Feng]

On Fri, Apr 17, 2015 at 08:35:30PM +0800, 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
> @@ -1216,29 +1216,8 @@ out:
>  	return rc;
>  }
>  
> -static char *
> -ll_getname(const char __user *filename)
> -{
> -	int ret = 0, len;
> -	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;
> -}
> -
> -#define ll_putname(filename) __putname(filename)
> +#define ll_getname(filename) getname(filename)
> +#define ll_putname(name) putname(name)
>  
>  static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
> @@ -1441,6 +1420,7 @@ free_lmv:
>  		return rc;
>  	}
>  	case LL_IOC_REMOVE_ENTRY: {
> +		struct filename *name = NULL;
>  		char		*filename = NULL;
>  		int		 namelen = 0;
>  		int		 rc;
> @@ -1453,20 +1433,17 @@ free_lmv:
>  		if (!(exp_connect_flags(sbi->ll_md_exp) & OBD_CONNECT_LVB_TYPE))
>  			return -ENOTSUPP;
>  
> -		filename = ll_getname((const char *)arg);
> -		if (IS_ERR(filename))
> -			return PTR_ERR(filename);
> +		name = ll_getname((const char *)arg);
> +		if (IS_ERR(name))
> +			return PTR_ERR(name);
>  
> +		filename = name->name;
>  		namelen = strlen(filename);
> -		if (namelen < 1) {
> -			rc = -EINVAL;
> -			goto out_rmdir;
> -		}
>  
>  		rc = ll_rmdir_entry(inode, filename, namelen);
>  out_rmdir:
> -		if (filename)
> -			ll_putname(filename);
> +		if (name)
> +			ll_putname(name);
>  		return rc;
>  	}
>  	case LL_IOC_LOV_SWAP_LAYOUTS:
> @@ -1481,15 +1458,17 @@ out_rmdir:
>  		struct lov_user_md *lump;
>  		struct lov_mds_md *lmm = NULL;
>  		struct mdt_body *body;
> +		struct filename *name;
>  		char *filename = NULL;
>  		int lmmsize;
>  
>  		if (cmd == IOC_MDC_GETFILEINFO ||
>  		    cmd == IOC_MDC_GETFILESTRIPE) {
> -			filename = ll_getname((const char *)arg);
> -			if (IS_ERR(filename))
> -				return PTR_ERR(filename);
> +			name = ll_getname((const char *)arg);
> +			if (IS_ERR(name))
> +				return PTR_ERR(name);
>  
> +			filename = name->name;
>  			rc = ll_lov_getstripe_ea_info(inode, filename, &lmm,
>  						      &lmmsize, &request);
>  		} else {

Sorry.. one modification is missing here:

@@ -1535,8 +1535,8 @@ skip_lmm:
 
 out_req:
                ptlrpc_req_finished(request);
-               if (filename)
-                       ll_putname(filename);
+               if (name)
+                       ll_putname(name);
                return rc;
        }
        case IOC_LOV_GETINFO: {

Regards,
Boqun

> 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? 
> 
> Looking forward to your insight.
> 
> Thanks,
> Boqun

Attachment: pgpNuV0xa6PUt.pgp
Description: PGP signature