Fixup lustre ll_getname
v2 cover Fixup lustre ll_getname
From: Oleg Drokin <[email protected]>
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(-)
Signature · 1 line
2.1.0
PATCH v2 1/2 staging/lustre/llite: remove LL_IOC_REMOVE_ENTRY handler
From: Oleg Drokin <[email protected]>
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.
--- .../lustre/lustre/include/lustre/lustre_user.h | 1 - drivers/staging/lustre/lustre/llite/dir.c | 29 ---------------------- 2 files changed, 30 deletions(-)
drivers/staging/lustre/lustre/include/lustre/lustre_user.h +0 -1
@@ -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)
drivers/staging/lustre/lustre/llite/dir.c +0 -29
@@ -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 :
Signature · 1 line
2.1.0
PATCH v2 2/2 staging/lustre/llite: fix ll_getname user buffer copy
In reply to [email protected]From: Oleg Drokin <[email protected]>
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
--- drivers/staging/lustre/lustre/llite/dir.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
drivers/staging/lustre/lustre/llite/dir.c +13 -8
@@ -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) {
Signature · 1 line
2.1.0