On Fri, Feb 7, 2020 at 8:26 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Thu, Feb 6, 2020 at 11:45 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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.
> > >
> > > 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.
> > >
> > > 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.
> >
> > It's not that simple,
Al,
Ping.
Are you sure it is not that simple for all practical cases?
Please take a closer look.
My change attempts to handle a real rename race similar to how
it was handled before the "Fixes" commit.
This is Acked by Bruce and Christoph.
Please see my arguments below.
Thanks,
Amir.
> unfortunately. For one thing, lookup_one_len_unlocked()
> > will normally return a negative dentry, not ERR_PTR(-ENOENT).
>
> Which is why this fix is mostly relevant to removed directories.
> negative dentry case should be handled correctly by bellow:
>
> if (tmp != dentry) {
>
>
> > For another,
> > it *can* fail for any number of other reasons (-ENOMEM, for example), without
> > anyone having ever looked it up.
>
> Yes, but why should we care to NOT return an error in case of ENOMEM.
> The question is are there other errors that we can say "we can let this slide"
> as long as the dentry is connected?
>
> I certainly don't mind going to out_reconnected for any error and that includes
> the error from exportfs_get_name(). My patch checks only the rename race
> case because this is what this function has done so far and this is what the
> big comment in out_reconnect is about.
>
> Thanks,
> Amir.
