Re: [PATCH RFC] namei: don't drop link paths acquired under LOOKUP_RCU

[Al Viro <viro@zeniv.linux.org.uk>]

On Sun, Feb 14, 2021 at 04:05:22PM +0000, Al Viro wrote:
> On Sun, Feb 07, 2021 at 01:26:19PM -0700, Jens Axboe wrote:
> 
> > Al, not sure if this is the right fix for the situation, but it's
> > definitely a problem. Observed by doing a LOOKUP_CACHED of something with
> > links, using /proc/self/comm as the example in the attached way to
> > demonstrate this problem.
> 
> That's definitely not the right fix.  What your analysis has missed is
> what legitimize_links() does to nd->depth when called.  IOW, on transitions
> from RCU mode you want nd->depth to set according the number of links we'd
> grabbed references to.  Flatly setting it to 0 on failure exit will lead
> to massive leaks.
> 
> Could you check if the following fixes your reproducers?
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 4cae88733a5c..afb293b39be7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -687,7 +687,7 @@ static bool try_to_unlazy(struct nameidata *nd)
>  
>  	nd->flags &= ~LOOKUP_RCU;
>  	if (nd->flags & LOOKUP_CACHED)
> -		goto out1;
> +		goto out2;
>  	if (unlikely(!legitimize_links(nd)))
>  		goto out1;
>  	if (unlikely(!legitimize_path(nd, &nd->path, nd->seq)))
> @@ -698,6 +698,8 @@ static bool try_to_unlazy(struct nameidata *nd)
>  	BUG_ON(nd->inode != parent->d_inode);
>  	return true;
>  
> +out2:
> +	nd->depth = 0;	// as we hadn't gotten to legitimize_links()
>  out1:
>  	nd->path.mnt = NULL;
>  	nd->path.dentry = NULL;
> @@ -725,7 +727,7 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi
>  
>  	nd->flags &= ~LOOKUP_RCU;
>  	if (nd->flags & LOOKUP_CACHED)
> -		goto out2;
> +		goto out3;
>  	if (unlikely(!legitimize_links(nd)))
>  		goto out2;
>  	if (unlikely(!legitimize_mnt(nd->path.mnt, nd->m_seq)))
> @@ -753,6 +755,8 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi
>  	rcu_read_unlock();
>  	return true;
>  
> +out3:
> +	nd->depth = 0;	// as we hadn't gotten to legitimize_links()
>  out2:
>  	nd->path.mnt = NULL;
>  out1:

Alternatively, we could use the fact that legitimize_links() is not
called anywhere other than these two places and have LOOKUP_CACHED
checked there.  As in

diff --git a/fs/namei.c b/fs/namei.c
index 4cae88733a5c..58962569cc20 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -630,6 +630,10 @@ static inline bool legitimize_path(struct nameidata *nd,
 static bool legitimize_links(struct nameidata *nd)
 {
 	int i;
+	if (unlikely(nd->flags & LOOKUP_CACHED)) {
+		nd->depth = 0;
+		return false;
+	}
 	for (i = 0; i < nd->depth; i++) {
 		struct saved *last = nd->stack + i;
 		if (unlikely(!legitimize_path(nd, &last->link, last->seq))) {
@@ -686,8 +690,6 @@ static bool try_to_unlazy(struct nameidata *nd)
 	BUG_ON(!(nd->flags & LOOKUP_RCU));
 
 	nd->flags &= ~LOOKUP_RCU;
-	if (nd->flags & LOOKUP_CACHED)
-		goto out1;
 	if (unlikely(!legitimize_links(nd)))
 		goto out1;
 	if (unlikely(!legitimize_path(nd, &nd->path, nd->seq)))
@@ -724,8 +726,6 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi
 	BUG_ON(!(nd->flags & LOOKUP_RCU));
 
 	nd->flags &= ~LOOKUP_RCU;
-	if (nd->flags & LOOKUP_CACHED)
-		goto out2;
 	if (unlikely(!legitimize_links(nd)))
 		goto out2;
 	if (unlikely(!legitimize_mnt(nd->path.mnt, nd->m_seq)))

That would be shorter, but might be harder to follow for reader.
Not sure...