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
--
Jan Kara <j...@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
