Re: [PATCH] ceph: is_root_ceph_dentry() cleanup

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Feb 11, 2025 at 07:32:16PM +0000, Viacheslav Dubeyko wrote:

> > The really unpleasant question is whether ceph_handle_notrace_create() could
> > end up feeding an already-positive dentry to direct call of ceph_lookup()...
> 
> We have ceph_handle_notrace_create() call in several methods:
> (1) ceph_mknod()
> (2) ceph_symlink()
> (3) ceph_mkdir()
> (4) ceph_atomic_open()
> 
> Every time we create object at first and, then, we call
> ceph_handle_notrace_create() if creation was successful. We have such pattern:
> 
> req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_MKNOD, USE_AUTH_MDS);
> 
> <skipped>
> 
> err = ceph_mdsc_do_request(mdsc, dir, req);
> if (!err && !req->r_reply_info.head->is_dentry)
>      err = ceph_handle_notrace_create(dir, dentry);
> 
> And ceph_lookup() has such logic:
> 
> if (d_really_is_negative(dentry)) {
>     <execute logic>
>     if (-ENOENT)
>         return NULL;
> }
> 
> req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
> 
> <skipped>
> 
> err = ceph_mdsc_do_request(mdsc, NULL, req);
> 
> So, we have two different type of requests here:
> (1) ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_MKNOD, USE_AUTH_MDS)
> (2) ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS)
> 
> The first request creates an object on MDS side and second one checks that this
> object exists on MDS side by lookup. I assume that first request simply reports
> success or failure of object creation. And only second one can extract metadata
> from MDS side.

If only...  The first request may return that metadata, in which case we'll get
splice_dentry() called by ceph_fill_trace() when reply arrives.  Note that
calls of ceph_handle_notrace_create() are conditional.

Intent is as you've described and normally that's how it works, but that
assumes sane reply.  Which is not a good assumption to make, when it comes
to memory corruption on client...

I _think_ the conditions are sufficient.  There are 4 callers of
ceph_handle_notrace_create().  All of them require is_dentry in reply to be false;
the one in ceph_mkdir() requires both is_dentry and is_target to be false.

ceph_fill_trace() does not call splice_dentry() when both is_dentry and is_target
are false, so we are only interested in the mknod/symlink/atomic_open callers.

Past the handling of !is_dentry && !is_target case ceph_fill_trace() has
a large if (is_dentry); not a concern for us.  Next comes if (is_target) where
we deal with ceph_fill_inode(); no calls of splice_dentry() in it.

After that we have
	if (is_dentry && ....) {
		...
		splice_dentry() call possible here
		...
	} else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
		    req->r_op == CEPH_MDS_OP_MKSNAP) &&
		   test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
		   !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
		...
		splice_dentry() call possible here
		...
	} else if (is_dentry && ...) {
		...
	}

Since we only care about !is_dentry case, that leaves the middle
part.  LOOKUPSNAP can come from ceph_lookup() or ceph_d_revalidate(),
neither of which call ceph_handle_notrace_create().  MKSNAP comes
only from ceph_mkdir(), which _does_ call ceph_handle_notrace_create(),
but only in case !is_dentry && !is_target, so that call of splice_dentry()
is not going to happen there.

*IF* the analysis above is correct, we can rely upon the ceph_lookup()
always getting a negative dentry and that if (d_really_is_negative(dentry))
is always taken.  But I could be missing something subtle in there ;-/