[1/2,Xenial,SRU] vfs: add lookup_hash() helper

From: Miklos Szeredi <mszeredi@redhat.com>

BugLink: http://bugs.launchpad.net/bugs/1659417

Overlayfs needs lookup without inode_permission() and already has the name
hash (in form of dentry->d_name on overlayfs dentry).  It also doesn't
support filesystems with d_op->d_hash() so basically it only needs
the actual hashed lookup from lookup_one_len_unlocked()

So add a new helper that does unlocked lookup of a hashed name.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
(backported from commit 3c9fe8cdff1b889a059a30d22f130372f2b3885f)
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/namei.c            | 21 +++++++++++++++++++++
 include/linux/namei.h |  2 ++
 2 files changed, 23 insertions(+)

On Thu, Jan 26, 2017 at 07:33:42AM -0600, Seth Forshee wrote:
> From: Miklos Szeredi <mszeredi@redhat.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1659417
> 
> Overlayfs needs lookup without inode_permission() and already has the name
> hash (in form of dentry->d_name on overlayfs dentry).  It also doesn't
> support filesystems with d_op->d_hash() so basically it only needs
> the actual hashed lookup from lookup_one_len_unlocked()
> 
> So add a new helper that does unlocked lookup of a hashed name.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> (backported from commit 3c9fe8cdff1b889a059a30d22f130372f2b3885f)
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  fs/namei.c            | 21 +++++++++++++++++++++
>  include/linux/namei.h |  2 ++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index f251a018e79f..0587407fa003 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1537,6 +1537,27 @@ static struct dentry *__lookup_hash(struct qstr *name,
>  	return lookup_real(base->d_inode, dentry, flags);
>  }
>  
> +/**
> + * lookup_hash - lookup single pathname component on already hashed name
> + * @name:	name and hash to lookup
> + * @base:	base directory to lookup from
> + *
> + * The name must have been verified and hashed (see lookup_one_len()).  Using
> + * this after just full_name_hash() is unsafe.
> + *
> + * This function also doesn't check for search permission on base directory.
> + *
> + * Use lookup_one_len_unlocked() instead, unless you really know what you are
> + * doing.
> + *
> + * Do not hold i_mutex; this helper takes i_mutex if necessary.
> + */
> +struct dentry *lookup_hash(struct qstr *name, struct dentry *base)
> +{
> +	return __lookup_hash(name, base, 0);
> +}
> +EXPORT_SYMBOL(lookup_hash);
> +

This was tricky to review, as it's far from what the upstream commit
does. I see what you are doing there, skipping all the checks at
lookup_one_len that overlayfs wanted to skip and just doing
__lookup_hash, which is what it does.

But going through some of the upstream commits that massage
lookup_dcache and lookup_slow, which are in the original commit, and how
your next commit uses lookup_hash, I could conclude there is a missing
lock here. I guess, fixing that, this should be fine, as long as
overlayfs is the sole user you have in mind.

Cascardo.

>  /*
>   *  It's more convoluted than I'd like it to be, but... it's still fairly
>   *  small and for now I'd prefer to have fast path as straight as possible.
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index d8c6334cd150..0a6a1ab47b38 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -77,6 +77,8 @@ extern struct dentry *kern_path_locked(const char *, struct path *);
>  extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int);
>  
>  extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
> +struct qstr;
> +extern struct dentry *lookup_hash(struct qstr *, struct dentry *);
>  
>  extern int follow_down_one(struct path *);
>  extern int follow_down(struct path *);
> -- 
> 2.7.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

On Mon, Jan 30, 2017 at 05:54:10PM -0200, Thadeu Lima de Souza Cascardo wrote:
> On Thu, Jan 26, 2017 at 07:33:42AM -0600, Seth Forshee wrote:
> > From: Miklos Szeredi <mszeredi@redhat.com>
> > 
> > BugLink: http://bugs.launchpad.net/bugs/1659417
> > 
> > Overlayfs needs lookup without inode_permission() and already has the name
> > hash (in form of dentry->d_name on overlayfs dentry).  It also doesn't
> > support filesystems with d_op->d_hash() so basically it only needs
> > the actual hashed lookup from lookup_one_len_unlocked()
> > 
> > So add a new helper that does unlocked lookup of a hashed name.
> > 
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > (backported from commit 3c9fe8cdff1b889a059a30d22f130372f2b3885f)
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  fs/namei.c            | 21 +++++++++++++++++++++
> >  include/linux/namei.h |  2 ++
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index f251a018e79f..0587407fa003 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1537,6 +1537,27 @@ static struct dentry *__lookup_hash(struct qstr *name,
> >  	return lookup_real(base->d_inode, dentry, flags);
> >  }
> >  
> > +/**
> > + * lookup_hash - lookup single pathname component on already hashed name
> > + * @name:	name and hash to lookup
> > + * @base:	base directory to lookup from
> > + *
> > + * The name must have been verified and hashed (see lookup_one_len()).  Using
> > + * this after just full_name_hash() is unsafe.
> > + *
> > + * This function also doesn't check for search permission on base directory.
> > + *
> > + * Use lookup_one_len_unlocked() instead, unless you really know what you are
> > + * doing.
> > + *
> > + * Do not hold i_mutex; this helper takes i_mutex if necessary.
> > + */
> > +struct dentry *lookup_hash(struct qstr *name, struct dentry *base)
> > +{
> > +	return __lookup_hash(name, base, 0);
> > +}
> > +EXPORT_SYMBOL(lookup_hash);
> > +
> 
> This was tricky to review, as it's far from what the upstream commit
> does. I see what you are doing there, skipping all the checks at
> lookup_one_len that overlayfs wanted to skip and just doing
> __lookup_hash, which is what it does.
> 
> But going through some of the upstream commits that massage
> lookup_dcache and lookup_slow, which are in the original commit, and how
> your next commit uses lookup_hash, I could conclude there is a missing
> lock here. I guess, fixing that, this should be fine, as long as
> overlayfs is the sole user you have in mind.

Good catch, I did overlook how the locking had changed. I'll fix that
and send a v2.

Thanks,
Seth