| Message ID | 20200126220800.32397-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exportfs: fix handling of rename race in reconnect_one() | expand |
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. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
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 >
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>
On Mon, Jan 27, 2020 at 08:38:00PM +0200, Amir Goldstein wrote: > > > 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. I can live with that. > > 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... Hah, I forgot that one. --b. > > 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> >
On Mon, Jan 27, 2020 at 11:18 PM J. Bruce Fields <bfields@redhat.com> wrote: > > On Mon, Jan 27, 2020 at 08:38:00PM +0200, Amir Goldstein wrote: > > > > 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. > > I can live with that. Will you take this patch through your tree, or do you want me to re-post to Al? With your Reviewed-by? Thanks, Amir.
On Thu, Feb 06, 2020 at 10:22:21PM +0200, Amir Goldstein wrote: > On Mon, Jan 27, 2020 at 11:18 PM J. Bruce Fields <bfields@redhat.com> wrote: > > > > On Mon, Jan 27, 2020 at 08:38:00PM +0200, Amir Goldstein wrote: > > > > > 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. > > > > I can live with that. > > Will you take this patch through your tree, > or do you want me to re-post to Al? If Al wants to delegate exportfs/ patches to nfsd maintainers that's OK by me, but in the past I think it's always him that's taken them. > With your Reviewed-by? That'd be fine. Thanks! --b.
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, unfortunately. For one thing, lookup_one_len_unlocked() will normally return a negative dentry, not ERR_PTR(-ENOENT). For another, it *can* fail for any number of other reasons (-ENOMEM, for example), without anyone having ever looked it up. So I agree that the damn thing needs work, but I don't believe that this is the right fix.
On Thu, Feb 6, 2020 at 11:45 PM Al Viro <viro@zeniv.linux.org.uk> 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, 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.
On Fri, Feb 7, 2020 at 8:26 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, Feb 6, 2020 at 11:45 PM Al Viro <viro@zeniv.linux.org.uk> 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.
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) {
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. 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(+)