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

[Jeff Layton <jlayton@kernel.org>]

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?
-- 
Jeff Layton <jlayton@kernel.org>