Re: [PATCH] exportfs: fix handling of rename race in reconnect_one()

["J. Bruce Fields" <bfields@redhat.com>]

Thanks for spotting this!

On Mon, Jan 27, 2020 at 12:08:00AM +0200, Amir Goldstein wrote:
> If a disconnected dentry gets looked up and renamed between the
> call to exportfs_get_name() and lookup_one_len_unlocked(), and if also
> lookup_one_len_unlocked() returns ERR_PTR(-ENOENT), maybe because old
> parent was deleted, we return an error, although dentry may be connected.

A comment that -ENOENT means the parent's gone might be helpful.

But are we sure -ENOENT is what every filesystem returns in the case the
parent was deleted?  And are we sure there aren't other cases that
should be handled similarly to -ENOENT?

> Commit 909e22e05353 ("exportfs: fix 'passing zero to ERR_PTR()'
> warning") changes this behavior from always returning success,
> regardless if dentry was reconnected by somoe other task, to always
> returning a failure.

I wonder whether it might be safest to take the out_reconnected case on
any error, not just -ENOENT.

Looking further back through the history....  Looks like the missing
PTR_ERR(tmp) was just a mistake, introduced in 2013 by my bbf7a8a3562f
"exportfs: move most of reconnect_path to helper function".  So the
historical behavior was always to bail on error.

The old code still did a DCACHE_DISCONNECTED check on the target dentry
in that case and returned success if it found that already cleared, but
we can't necessarily rely on DCACHE_DISCONNECTED being cleared
immediately, so the old code was probably still vulnerable to the race
you saw.

There's not much value in preserving the error as exportfs_decode_fh()
ends up turning everything into ENOMEM or ESTALE for some reason.

Hm.--b.

> Change the lookup error handling to match that of exportfs_get_name()
> error handling and return success after getting -ENOENT and verifying
> that some other task has connected the dentry for us.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: J. Bruce Fields <bfields@redhat.com>
> Fixes: 909e22e05353 ("exportfs: fix 'passing zero to ERR_PTR()' warning")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/exportfs/expfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 2dd55b172d57..25a09bacf9c1 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -149,6 +149,8 @@ static struct dentry *reconnect_one(struct vfsmount *mnt,
>  	if (IS_ERR(tmp)) {
>  		dprintk("%s: lookup failed: %d\n", __func__, PTR_ERR(tmp));
>  		err = PTR_ERR(tmp);
> +		if (err == -ENOENT)
> +			goto out_reconnected;
>  		goto out_err;
>  	}
>  	if (tmp != dentry) {
> -- 
> 2.17.1
>