[PATCH v2] ceph: discard r_new_inode if open O_CREAT opened existing inode

[PATCH v2] ceph: discard r_new_inode if open O_CREAT opened existing inode

[Jeff Layton]

When we do an unchecked create, we optimistically pre-create an inode
and populate it, including its fscrypt context. It's possible though
that we'll end up opening an existing inode, in which case the
precreated inode will have a crypto context that doesn't match the
existing data.

If we're issuing an O_CREAT open and find an existing inode, just
discard the precreated inode and create a new one to ensure the context
is properly set.

Also, we should never end up opening an existing file on an async
create, since we should know that the dentry doesn't exist. Throw a
warning if that ever does occur.

Cc: Luís Henriques <lhenriques@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/mds_client.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

v2: WARN if this ever happens on an async create

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 840a60b812fc..b3cf3a22fa2a 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3504,13 +3504,21 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
 	/* Must find target inode outside of mutexes to avoid deadlocks */
 	rinfo = &req->r_reply_info;
 	if ((err >= 0) && rinfo->head->is_target) {
-		struct inode *in;
+		struct inode *in = xchg(&req->r_new_inode, NULL);
 		struct ceph_vino tvino = {
 			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
 			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
 		};
 
-		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
+		/* If we ended up opening an existing inode, discard r_new_inode */
+		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
+			/* This should never happen on an async create */
+			WARN_ON_ONCE(req->r_deleg_ino);
+			iput(in);
+			in = NULL;
+		}
+
+		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
 		if (IS_ERR(in)) {
 			err = PTR_ERR(in);
 			mutex_lock(&session->s_mutex);
-- 
2.35.1

Re: [PATCH v2] ceph: discard r_new_inode if open O_CREAT opened existing inode

[Xiubo Li]

On 3/31/22 5:59 PM, Jeff Layton wrote:
> When we do an unchecked create, we optimistically pre-create an inode
> and populate it, including its fscrypt context. It's possible though
> that we'll end up opening an existing inode, in which case the
> precreated inode will have a crypto context that doesn't match the
> existing data.
>
> If we're issuing an O_CREAT open and find an existing inode, just
> discard the precreated inode and create a new one to ensure the context
> is properly set.
>
> Also, we should never end up opening an existing file on an async
> create, since we should know that the dentry doesn't exist. Throw a
> warning if that ever does occur.
>
> Cc: Luís Henriques <lhenriques@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/mds_client.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> v2: WARN if this ever happens on an async create
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 840a60b812fc..b3cf3a22fa2a 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3504,13 +3504,21 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>   	/* Must find target inode outside of mutexes to avoid deadlocks */
>   	rinfo = &req->r_reply_info;
>   	if ((err >= 0) && rinfo->head->is_target) {
> -		struct inode *in;
> +		struct inode *in = xchg(&req->r_new_inode, NULL);
>   		struct ceph_vino tvino = {
>   			.ino  = le64_to_cpu(rinfo->targeti.in->ino),
>   			.snap = le64_to_cpu(rinfo->targeti.in->snapid)
>   		};
>   
> -		in = ceph_get_inode(mdsc->fsc->sb, tvino, xchg(&req->r_new_inode, NULL));
> +		/* If we ended up opening an existing inode, discard r_new_inode */
> +		if (req->r_op == CEPH_MDS_OP_CREATE && !req->r_reply_info.has_create_ino) {
> +			/* This should never happen on an async create */
> +			WARN_ON_ONCE(req->r_deleg_ino);
> +			iput(in);
> +			in = NULL;
> +		}
> +
> +		in = ceph_get_inode(mdsc->fsc->sb, tvino, in);
>   		if (IS_ERR(in)) {
>   			err = PTR_ERR(in);
>   			mutex_lock(&session->s_mutex);

LGTM.

Reviewed-by: Xiubo Li <xiubli@redhat.com>