[PATCH v2 0/2] Fixup lustre ll_getname

[PATCH v2 0/2] Fixup lustre ll_getname

[green]

From: Oleg Drokin <green@linuxhacker.ru>

Some time ago Al Viro noticed that lustre ll_getname is broken.
At the time a patch was submitted to convert lustre to use
exported getname, that was rejected by hch on the grounds that
filesystem code sould not really be reimplementing their own
lookups which kind of made sense back then.

But upon further investigation it seems that ll_getname is
used in a different way, it only gets a single path name
component that is then shiped to the server for some operations.
Going through VFS here to do proper lookups is not really
all that good of an idea since dcache pollution is undesired
at the very least.

So these two patches drop one of the ll_getname users that
could be done in another way and fix up ll_getname to only
allocate a single pathname component buffer and properly check
copy from userspace return value.

Please consider.

Oleg Drokin (2):
  staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler
  staging/lustre/llite: fix ll_getname

 .../lustre/lustre/include/lustre/lustre_user.h     |  1 -
 drivers/staging/lustre/lustre/llite/dir.c          | 50 ++++++----------------
 2 files changed, 13 insertions(+), 38 deletions(-)

-- 
2.1.0

[PATCH v2 1/2] staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler

[green]

From: Oleg Drokin <green@linuxhacker.ru>

It uses getname in unsafe manner and since it's to deal with corrupted
or inconsistent filesystem, we are probably better to deal with
it from lfsck anyway.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 .../lustre/lustre/include/lustre/lustre_user.h     |  1 -
 drivers/staging/lustre/lustre/llite/dir.c          | 29 ----------------------
 2 files changed, 30 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
index 89794fd..e095ada 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
@@ -243,7 +243,6 @@ struct ost_id {
 
 #define LL_IOC_LMV_SETSTRIPE	    _IOWR('f', 240, struct lmv_user_md)
 #define LL_IOC_LMV_GETSTRIPE	    _IOWR('f', 241, struct lmv_user_md)
-#define LL_IOC_REMOVE_ENTRY	    _IOWR('f', 242, __u64)
 #define LL_IOC_SET_LEASE		_IOWR('f', 243, long)
 #define LL_IOC_GET_LEASE		_IO('f', 244)
 #define LL_IOC_HSM_IMPORT		_IOWR('f', 245, struct hsm_user_import)
diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 4b0de8d..87a042c 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1436,35 +1436,6 @@ free_lmv:
 		kfree(tmp);
 		return rc;
 	}
-	case LL_IOC_REMOVE_ENTRY: {
-		char		*filename = NULL;
-		int		 namelen = 0;
-		int		 rc;
-
-		/* Here is a little hack to avoid sending REINT_RMENTRY to
-		 * unsupported server, which might crash the server(LU-2730),
-		 * Because both LVB_TYPE and REINT_RMENTRY will be supported
-		 * on 2.4, we use OBD_CONNECT_LVB_TYPE to detect whether the
-		 * server will support REINT_RMENTRY XXX*/
-		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);
-
-		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);
-		return rc;
-	}
 	case LL_IOC_LOV_SWAP_LAYOUTS:
 		return -EPERM;
 	case LL_IOC_OBD_STATFS:
-- 
2.1.0

[PATCH v2 2/2] staging/lustre/llite: fix ll_getname user buffer copy

[green]

From: Oleg Drokin <green@linuxhacker.ru>

strncpy_from_user could return negative values on error,
so need to take those into account.
Since ll_getname is used to get a single component name from userspace
to transfer to server as-is, there's no need to allocate 4k buffer
as done by __getname. Allocate NAME_MAX+1 buffer instead to ensure
we have enough for a null terminated max valid length buffer.

This was discovered by Al Viro in https://lkml.org/lkml/2015/4/11/243

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 drivers/staging/lustre/lustre/llite/dir.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 87a042c..50d685b 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1213,29 +1213,34 @@ out:
 	return rc;
 }
 
-static char *
-ll_getname(const char __user *filename)
+/* This function tries to get a single name component,
+ * to send to the server. No actual path traversal involved,
+ * so we limit to NAME_MAX */
+static char *ll_getname(const char __user *filename)
 {
 	int ret = 0, len;
-	char *tmp = __getname();
+	char *tmp;
 
+	tmp = kzalloc(NAME_MAX + 1, GFP_KERNEL);
 	if (!tmp)
 		return ERR_PTR(-ENOMEM);
 
-	len = strncpy_from_user(tmp, filename, PATH_MAX);
-	if (len == 0)
+	len = strncpy_from_user(tmp, filename, NAME_MAX + 1);
+	if (len < 0)
+		ret = len;
+	else if (len == 0)
 		ret = -ENOENT;
-	else if (len > PATH_MAX)
+	else if (len > NAME_MAX && tmp[NAME_MAX] != 0)
 		ret = -ENAMETOOLONG;
 
 	if (ret) {
-		__putname(tmp);
+		kfree(tmp);
 		tmp =  ERR_PTR(ret);
 	}
 	return tmp;
 }
 
-#define ll_putname(filename) __putname(filename)
+#define ll_putname(filename) kfree(filename)
 
 static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-- 
2.1.0