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
pgpF8_FICh3Xs.pgp
Description: PGP signature
