| Message ID | 20150417123530.GA4249@fixme-laptop.cn.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Fri 17-04-15 20:35:30, 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 ... > +#define ll_getname(filename) getname(filename) > +#define ll_putname(name) putname(name) Bonus points for getting rid of these useless defines. > 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? Yes, it is a good idea. Honza
Hi, On Mon, Apr 20, 2015 at 05:55:07PM +0200, Jan Kara wrote: > On Fri 17-04-15 20:35:30, Boqun Feng wrote: > > Hi Al, > > > > > > 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 > ... > > +#define ll_getname(filename) getname(filename) > > +#define ll_putname(name) putname(name) > Bonus points for getting rid of these useless defines. Yeah, make sense. Thank you for your comments, 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? > Yes, it is a good idea. > > Honza > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR
On Mon, Apr 20, 2015 at 05:55:07PM +0200, Jan Kara wrote: > > Is that a good idea to export these symbols, given that lustre may be > > the only user? > Yes, it is a good idea. It was if lustre was in core code and these idiotic ioctls were something we had to live with. But lustre is in staging code, and we shouldn't export symbols just for shitty staging code. Intead lustre should clean up their mess and get rid of these ioctls that do path name lookups. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 21, 2015 at 10:50:15PM -0700, Christoph Hellwig wrote: > On Mon, Apr 20, 2015 at 05:55:07PM +0200, Jan Kara wrote: > > > Is that a good idea to export these symbols, given that lustre may be > > > the only user? > > Yes, it is a good idea. > > It was if lustre was in core code and these idiotic ioctls were something > we had to live with. > > But lustre is in staging code, and we shouldn't export symbols just for > shitty staging code. > > Intead lustre should clean up their mess and get rid of these ioctls > that do path name lookups. OK, good point. Thank you! Regards, Boqun Feng
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 { 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) {