Re: [PATCH v2 04/14] ceph: simplify atomic_open to use finish_no_open()

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2026-09-26 at 07:49 +1000, NeilBrown wrote:
> On Thu, 24 Sep 2026, Jeff Layton wrote:
> > Another LLM review:
> > 
> > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > > index bd3e3f5c269e..83ceb2906f85 100644
> > > --- a/fs/ceph/file.c
> > > +++ b/fs/ceph/file.c
> > 
> > [ ... ]
> > 
> > > @@ -977,33 +976,25 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> > >  	}
> > >  	if (err)
> > >  		goto out_req;
> > > -	if (dn || d_really_is_negative(dentry) || d_is_symlink(dentry)) {
> > > -		/* make vfs retry on splice, ENOENT, or symlink */
> > > -		doutc(cl, "finish_no_open on dn %p\n", dn);
> > > -		err = finish_no_open(file, dn);
> > > -	} else {
> > > -		if (IS_ENCRYPTED(dir) &&
> > > -		    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> > > -			pr_warn_client(cl,
> > > -					"Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> > > -					ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> > > -			goto out_req;
> > > -		}
> > > +	if (IS_ENCRYPTED(dir) &&
> > > +	    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> > > +		pr_warn_client(cl,
> > > +			       "Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> > > +			       ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> > > +		goto out_req;
> > > +	}
> > 
> > The fscrypt check used to run only when the dentry was positive, not
> > a symlink, and had no alias. Now it runs unconditionally, but
> > d_inode() can still be NULL here: a lookup that found nothing leaves
> > a negative dentry with err == 0, and when the MDS reply splices an
> > alias the original dentry is never instantiated.
> > 
> > fscrypt_has_permitted_context() dereferences its child argument
> > without checking for NULL:
> > 
> > fs/crypto/policy.c:fscrypt_has_permitted_context() {
> > 	...
> > 	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
> > 	    !S_ISLNK(child->i_mode))
> > 		return 1;
> > 	...
> > }
> > 
> > Every other in-tree caller passes a known inode, while this one can
> > pass NULL. An open of a name that does not exist in an encrypted
> > directory reaches this through lookup_open() -> atomic_open() ->
> > ceph_atomic_open().
> > 
> > Could the fscrypt check stay behind the positive-dentry test, as
> > before?
> 
> Alternately: can we simply remove the fscrypt check?  I don't know a
> whole lot about fscrypt, but it seems that ceph_open() does all
> necessary fscrypt checks, and we already didn't need this one before
> calling ceph_open().
> 
> You added this in
>   Commit: 94af0470924c ("ceph: add some fscrypt guardrails")
> but the commit doesn't explain why ceph_atomic_open() needed more that
> ceph_open() needed.
> 

Yes, let's just drop it. I had an LLM verify that too, and here's it's
rationales:

Three reasons to remove rather than re-nest it:

1) For regular files it's redundant. finish_open() calls ceph_open(),
   which calls fscrypt_file_open(). That does the same
   fscrypt_has_permitted_context() against the parent (d_parent's inode
   is dir on this path), plus fscrypt_require_key(), which the
   open-coded version doesn't.

2) It's broken as written. err is 0 at that point, so tripping it
   returns 0 from ->atomic_open with FMODE_OPENED clear and
   f_path.dentry still DENTRY_NOT_SET. That hits the WARN_ON() in
   atomic_open() in fs/namei.c and gives back -EIO instead of -EPERM.
   Clearly it has never fired in testing.

3) The only thing it covers that ceph_open() doesn't is a directory
   child (symlinks and spliced/negative dentries all go to
   finish_no_open). But that coverup()
   has no context check at all, unlike ext4/f2fs/ubifs which check
   S_ISDIR||S_ISLNK children in -> via
   the normal lookup path -- including everything finish_no_open()
   bounces back to the VFS -- are already unchecked.

-- 
Jeff Layton <jlayton@kernel.org>