[PATCH] ceph: don't set req->r_locked_dir in ceph_d_revalidate

[PATCH] ceph: don't set req->r_locked_dir in ceph_d_revalidate

[Jeff Layton]

This function sets req->r_locked_dir which is supposed to indicate to
ceph_fill_trace that the parent's i_rwsem is locked for write.
Unfortunately, there is no guarantee that the dir will be locked when
d_revalidate is called, so we really don't want ceph_fill_trace to do
any dcache manipulation from this context. Clear req->r_locked_dir since
it's clearly not safe to do that.

What we really want to know with d_revalidate is whether the dentry
still points to the same inode. ceph_fill_trace installs a pointer to
the inode in req->r_target_inode, so we can just compare that to
d_inode(dentry) to see if it's the same one after the lookup.

Also, since we aren't generally interested in the parent here, we can
switch to using a GETATTR to hint that to the MDS, which also means that
we only need to reserve one cap.

Finally, just remove the d_unhashed check. That's really outside the
purview of a filesystem's d_revalidate. If the thing became unhashed
while we're checking it, then that's up to the VFS to handle anyway.

Fixes: 200fd27c8fa2 (ceph: use lookup request to revalidate dentry)
Reported-by: Donatas Abraitis <donatas.abraitis@gmail.com>
Cc: stable@vger.kernel.org # v4.6+
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/dir.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 78180d151730..a594c7879cc2 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1261,26 +1261,30 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 			return -ECHILD;
 
 		op = ceph_snap(dir) == CEPH_SNAPDIR ?
-			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_LOOKUP;
+			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_GETATTR;
 		req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
 		if (!IS_ERR(req)) {
 			req->r_dentry = dget(dentry);
-			req->r_num_caps = 2;
+			req->r_num_caps = op == CEPH_MDS_OP_GETATTR ? 1 : 2;
 
 			mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
 			if (ceph_security_xattr_wanted(dir))
 				mask |= CEPH_CAP_XATTR_SHARED;
 			req->r_args.getattr.mask = mask;
 
-			req->r_locked_dir = dir;
 			err = ceph_mdsc_do_request(mdsc, NULL, req);
-			if (err == 0 || err == -ENOENT) {
-				if (dentry == req->r_dentry) {
-					valid = !d_unhashed(dentry);
-				} else {
-					d_invalidate(req->r_dentry);
-					err = -EAGAIN;
-				}
+			switch (err) {
+			case 0:
+				if (d_really_is_positive(dentry) &&
+				    d_inode(dentry) == req->r_target_inode)
+					valid = 1;
+				break;
+			case -ENOENT:
+				if (d_really_is_negative(dentry))
+					valid = 1;
+				/* Fallthrough */
+			default:
+				break;
 			}
 			ceph_mdsc_put_request(req);
 			dout("d_revalidate %p lookup result=%d\n",
-- 
2.7.4

Re: [PATCH] ceph: don't set req->r_locked_dir in ceph_d_revalidate

[Yan, Zheng]

> On 3 Dec 2016, at 00:52, Jeff Layton <jlayton@redhat.com> wrote:
> 
> This function sets req->r_locked_dir which is supposed to indicate to
> ceph_fill_trace that the parent's i_rwsem is locked for write.
> Unfortunately, there is no guarantee that the dir will be locked when
> d_revalidate is called, so we really don't want ceph_fill_trace to do
> any dcache manipulation from this context. Clear req->r_locked_dir since
> it's clearly not safe to do that.
> 
> What we really want to know with d_revalidate is whether the dentry
> still points to the same inode. ceph_fill_trace installs a pointer to
> the inode in req->r_target_inode, so we can just compare that to
> d_inode(dentry) to see if it's the same one after the lookup.
> 
> Also, since we aren't generally interested in the parent here, we can
> switch to using a GETATTR to hint that to the MDS, which also means that
> we only need to reserve one cap.
> 
> Finally, just remove the d_unhashed check. That's really outside the
> purview of a filesystem's d_revalidate. If the thing became unhashed
> while we're checking it, then that's up to the VFS to handle anyway.
> 
> Fixes: 200fd27c8fa2 (ceph: use lookup request to revalidate dentry)
> Reported-by: Donatas Abraitis <donatas.abraitis@gmail.com>
> Cc: stable@vger.kernel.org # v4.6+
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/ceph/dir.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 78180d151730..a594c7879cc2 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1261,26 +1261,30 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
> 			return -ECHILD;
> 
> 		op = ceph_snap(dir) == CEPH_SNAPDIR ?
> -			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_LOOKUP;
> +			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_GETATTR;
> 		req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
> 		if (!IS_ERR(req)) {
> 			req->r_dentry = dget(dentry);
> -			req->r_num_caps = 2;
> +			req->r_num_caps = op == CEPH_MDS_OP_GETATTR ? 1 : 2;
> 
> 			mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
> 			if (ceph_security_xattr_wanted(dir))
> 				mask |= CEPH_CAP_XATTR_SHARED;
> 			req->r_args.getattr.mask = mask;
> 
> -			req->r_locked_dir = dir;
> 			err = ceph_mdsc_do_request(mdsc, NULL, req);
> -			if (err == 0 || err == -ENOENT) {
> -				if (dentry == req->r_dentry) {
> -					valid = !d_unhashed(dentry);
> -				} else {
> -					d_invalidate(req->r_dentry);
> -					err = -EAGAIN;
> -				}
> +			switch (err) {
> +			case 0:
> +				if (d_really_is_positive(dentry) &&
> +				    d_inode(dentry) == req->r_target_inode)
> +					valid = 1;
> +				break;
> +			case -ENOENT:
> +				if (d_really_is_negative(dentry))
> +					valid = 1;
> +				/* Fallthrough */
> +			default:
> +				break;
> 			}
> 			ceph_mdsc_put_request(req);
> 			dout("d_revalidate %p lookup result=%d\n”,

Looks good. As we discussed, please write a patch that guarantees safe access to d_parent when parent is not locked.
(__choose_mds, build_dentry_path and probably ceph_encode_dentry_release)

Regards
Yan, Zheng

-- 
> 2.7.4
> 

Re: [PATCH] ceph: don't set req->r_locked_dir in ceph_d_revalidate

[Jeff Layton]

On Mon, 2016-12-05 at 10:28 +0800, Yan, Zheng wrote:
> > 
> > On 3 Dec 2016, at 00:52, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > This function sets req->r_locked_dir which is supposed to indicate to
> > ceph_fill_trace that the parent's i_rwsem is locked for write.
> > Unfortunately, there is no guarantee that the dir will be locked when
> > d_revalidate is called, so we really don't want ceph_fill_trace to do
> > any dcache manipulation from this context. Clear req->r_locked_dir since
> > it's clearly not safe to do that.
> > 
> > What we really want to know with d_revalidate is whether the dentry
> > still points to the same inode. ceph_fill_trace installs a pointer to
> > the inode in req->r_target_inode, so we can just compare that to
> > d_inode(dentry) to see if it's the same one after the lookup.
> > 
> > Also, since we aren't generally interested in the parent here, we can
> > switch to using a GETATTR to hint that to the MDS, which also means that
> > we only need to reserve one cap.
> > 
> > Finally, just remove the d_unhashed check. That's really outside the
> > purview of a filesystem's d_revalidate. If the thing became unhashed
> > while we're checking it, then that's up to the VFS to handle anyway.
> > 
> > Fixes: 200fd27c8fa2 (ceph: use lookup request to revalidate dentry)
> > Reported-by: Donatas Abraitis <donatas.abraitis@gmail.com>
> > Cc: stable@vger.kernel.org # v4.6+
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > fs/ceph/dir.c | 24 ++++++++++++++----------
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 78180d151730..a594c7879cc2 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -1261,26 +1261,30 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
> > 			return -ECHILD;
> > 
> > 		op = ceph_snap(dir) == CEPH_SNAPDIR ?
> > -			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_LOOKUP;
> > +			CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_GETATTR;
> > 		req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
> > 		if (!IS_ERR(req)) {
> > 			req->r_dentry = dget(dentry);
> > -			req->r_num_caps = 2;
> > +			req->r_num_caps = op == CEPH_MDS_OP_GETATTR ? 1 : 2;
> > 
> > 			mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
> > 			if (ceph_security_xattr_wanted(dir))
> > 				mask |= CEPH_CAP_XATTR_SHARED;
> > 			req->r_args.getattr.mask = mask;
> > 
> > -			req->r_locked_dir = dir;
> > 			err = ceph_mdsc_do_request(mdsc, NULL, req);
> > -			if (err == 0 || err == -ENOENT) {
> > -				if (dentry == req->r_dentry) {
> > -					valid = !d_unhashed(dentry);
> > -				} else {
> > -					d_invalidate(req->r_dentry);
> > -					err = -EAGAIN;
> > -				}
> > +			switch (err) {
> > +			case 0:
> > +				if (d_really_is_positive(dentry) &&
> > +				    d_inode(dentry) == req->r_target_inode)
> > +					valid = 1;
> > +				break;
> > +			case -ENOENT:
> > +				if (d_really_is_negative(dentry))
> > +					valid = 1;
> > +				/* Fallthrough */
> > +			default:
> > +				break;
> > 			}
> > 			ceph_mdsc_put_request(req);
> > 			dout("d_revalidate %p lookup result=%d\n”,
> 
> Looks good. As we discussed, please write a patch that guarantees safe access to d_parent when parent is not locked.
> (__choose_mds, build_dentry_path and probably ceph_encode_dentry_release)
> 
> Regards
> Yan, Zheng
> 

Thanks. I'll add your Reviewed-by and merge it into ceph-client.git if
that's ok.

The issue with parent stability while walking back up the tree is a
bigger problem, but is separate from this one. I'm still looking at
that...

-- 
Jeff Layton <jlayton@redhat.com>