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@xxxxxx>
> Cc: J. Bruce Fields <bfields@xxxxxxxxxx>
> Fixes: 909e22e05353 ("exportfs: fix 'passing zero to ERR_PTR()' warning")
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> 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
>
