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

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Jan 27, 2020 at 7:30 PM J. Bruce Fields <bfields@redhat.com> wrote:
>
> 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.

It doesn't have to mean that, but that's the most obvious case.

>
> But are we sure -ENOENT is what every filesystem returns in the case the
> parent was deleted?

No, it's what __lookup_slow() returns if parent is dead.
Most filesystems do not return -ENOENT for lookup, but a negative
dentry on NULL. I am not sure which filesystems return -ENOENT.
A short survey of NFS exporting fs I didn't find any.

> And are we sure there aren't other cases that
> should be handled similarly to -ENOENT?
>

Not sure, but ENOENT is the most obvious one for rename race.

> > 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.
>

I wondered that as well, but preferred to follow the precedent.

> 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.
>

Yeh, I started to try and document history, but since there seemed to be
no point where behavior looked sane I gave up.

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

You signed up on this reason...

Thanks,
Amir.

commit 09bb8bfffd29c3dffb72bc2c69a062dfb1ae624c
Author: NeilBrown <neilb@suse.com>
Date:   Thu Aug 4 10:19:06 2016 +1000

    exportfs: be careful to only return expected errors.

    When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
    In particular it can be tempting to return ENOENT, but this is not
    handled well by nfsd.

    Rather than requiring strict adherence to error code code filesystems,
    treat all unexpected error codes the same as ESTALE.  This is safest.

    Signed-off-by: NeilBrown <neilb@suse.com>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>