Re: [PATCH 2/2] staging: lustre: replace ll_{get,put}name() with {get,put}name()

[Dilger, Andreas]

On 2015/04/21, 9:50 PM, "Boqun Feng" <boqun.feng@xxxxxxxxx> wrote:

>As pointed by Al Viro:
>
>https://lkml.org/lkml/2015/4/11/243
>
>There are bugs in ll_getname() because of wrong assumptions of returning
>values from strncpy_from_user(). Moreover, what ll_getname want to do is
>just to try copy the file name from userland. Since we already have
>getname() for the same purpose, it's better to replace ll_getname() with
>getname(), so is ll_putname().
>
>Besides, remove unused code for checking whether namelen is 0 or not in
>case LL_IOC_REMOVE_ENTRY, because zero-length file name is already
>handled by getname() in the same way as ll_getname().
>
>Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>

Looks good, you can add my:
Reviewed-by: Andreas Dilger <andreas.dilger@xxxxxxxxx>

>---
> drivers/staging/lustre/lustre/llite/dir.c          | 60
>++++++----------------
> .../staging/lustre/lustre/llite/llite_internal.h   |  2 +-
> drivers/staging/lustre/lustre/llite/namei.c        |  2 +-
> 3 files changed, 18 insertions(+), 46 deletions(-)
>
>diff --git a/drivers/staging/lustre/lustre/llite/dir.c
>b/drivers/staging/lustre/lustre/llite/dir.c
>index a182019..c75fc38 100644
>--- a/drivers/staging/lustre/lustre/llite/dir.c
>+++ b/drivers/staging/lustre/lustre/llite/dir.c
>@@ -1216,30 +1216,6 @@ 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)
>-
> static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned
>long arg)
> {
> 	struct inode *inode = file_inode(file);
>@@ -1441,7 +1417,7 @@ free_lmv:
> 		return rc;
> 	}
> 	case LL_IOC_REMOVE_ENTRY: {
>-		char		*filename = NULL;
>+		struct filename *name = NULL;
> 		int		 namelen = 0;
> 		int		 rc;
> 
>@@ -1453,20 +1429,16 @@ 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 = getname((const char *)arg);
>+		if (IS_ERR(name))
>+			return PTR_ERR(name);
> 
>-		namelen = strlen(filename);
>-		if (namelen < 1) {
>-			rc = -EINVAL;
>-			goto out_rmdir;
>-		}
>+		namelen = strlen(name->name);
>+
>+		rc = ll_rmdir_entry(inode, name->name, namelen);
> 
>-		rc = ll_rmdir_entry(inode, filename, namelen);
>-out_rmdir:
>-		if (filename)
>-			ll_putname(filename);
>+		if (name)
>+			putname(name);
> 		return rc;
> 	}
> 	case LL_IOC_LOV_SWAP_LAYOUTS:
>@@ -1481,16 +1453,16 @@ out_rmdir:
> 		struct lov_user_md *lump;
> 		struct lov_mds_md *lmm = NULL;
> 		struct mdt_body *body;
>-		char *filename = NULL;
>+		struct filename *name = 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 = getname((const char *)arg);
>+			if (IS_ERR(name))
>+				return PTR_ERR(name);
> 
>-			rc = ll_lov_getstripe_ea_info(inode, filename, &lmm,
>+			rc = ll_lov_getstripe_ea_info(inode, name->name, &lmm,
> 						      &lmmsize, &request);
> 		} else {
> 			rc = ll_dir_getstripe(inode, &lmm, &lmmsize, &request);
>@@ -1556,8 +1528,8 @@ skip_lmm:
> 
> out_req:
> 		ptlrpc_req_finished(request);
>-		if (filename)
>-			ll_putname(filename);
>+		if (name)
>+			putname(name);
> 		return rc;
> 	}
> 	case IOC_LOV_GETINFO: {
>diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h
>b/drivers/staging/lustre/lustre/llite/llite_internal.h
>index 2af1d72..0950565 100644
>--- a/drivers/staging/lustre/lustre/llite/llite_internal.h
>+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
>@@ -714,7 +714,7 @@ struct inode *ll_iget(struct super_block *sb, ino_t
>hash,
> int ll_md_blocking_ast(struct ldlm_lock *, struct ldlm_lock_desc *,
> 		       void *data, int flag);
> struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de);
>-int ll_rmdir_entry(struct inode *dir, char *name, int namelen);
>+int ll_rmdir_entry(struct inode *dir, const char *name, int namelen);
> 
> /* llite/rw.c */
> int ll_prepare_write(struct file *, struct page *, unsigned from,
>unsigned to);
>diff --git a/drivers/staging/lustre/lustre/llite/namei.c
>b/drivers/staging/lustre/lustre/llite/namei.c
>index 890ac19..ec48d8d 100644
>--- a/drivers/staging/lustre/lustre/llite/namei.c
>+++ b/drivers/staging/lustre/lustre/llite/namei.c
>@@ -867,7 +867,7 @@ static inline void ll_get_child_fid(struct dentry
>*child, struct lu_fid *fid)
> /**
>  * Remove dir entry
>  **/
>-int ll_rmdir_entry(struct inode *dir, char *name, int namelen)
>+int ll_rmdir_entry(struct inode *dir, const char *name, int namelen)
> {
> 	struct ptlrpc_request *request = NULL;
> 	struct md_op_data *op_data;
>-- 
>2.3.5
>
>


Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division


--
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/