[PATCH 0/2] ceph: misc fixes

[PATCH 0/2] ceph: misc fixes

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

Based the testing branch.

Xiubo Li (2):
  ceph: fix inode reference leakage in ceph_get_snapdir()
  ceph: fix a NULL pointer dereference in ceph_handle_caps()

 fs/ceph/caps.c  |  2 +-
 fs/ceph/inode.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.27.0

[PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir()

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The ceph_get_inode() will search for or insert a new inode into the
hash for the given vino, and return a reference to it. If new is
non-NULL, its reference is consumed.

We should release the reference when in error handing cases.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/inode.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8b0832271fdf..cbeba8a93a07 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -164,13 +164,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 	if (!S_ISDIR(parent->i_mode)) {
 		pr_warn_once("bad snapdir parent type (mode=0%o)\n",
 			     parent->i_mode);
-		return ERR_PTR(-ENOTDIR);
+		goto err;
 	}
 
 	if (!(inode->i_state & I_NEW) && !S_ISDIR(inode->i_mode)) {
 		pr_warn_once("bad snapdir inode type (mode=0%o)\n",
 			     inode->i_mode);
-		return ERR_PTR(-ENOTDIR);
+		goto err;
 	}
 
 	inode->i_mode = parent->i_mode;
@@ -190,6 +190,12 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 	}
 
 	return inode;
+err:
+	if ((inode->i_state & I_NEW))
+		discard_new_inode(inode);
+	else
+		iput(inode);
+	return ERR_PTR(-ENOTDIR);
 }
 
 const struct inode_operations ceph_file_iops = {
-- 
2.27.0

[PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps()

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The ceph_find_inode() may will fail and return NULL.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/caps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 0b36020207fd..0762b55fdbcb 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -4303,7 +4303,6 @@ void ceph_handle_caps(struct ceph_mds_session *session,
 
 	/* lookup ino */
 	inode = ceph_find_inode(mdsc->fsc->sb, vino);
-	ci = ceph_inode(inode);
 	dout(" op %s ino %llx.%llx inode %p\n", ceph_cap_op_name(op), vino.ino,
 	     vino.snap, inode);
 
@@ -4333,6 +4332,7 @@ void ceph_handle_caps(struct ceph_mds_session *session,
 		}
 		goto flush_cap_releases;
 	}
+	ci = ceph_inode(inode);
 
 	/* these will work even if we don't have a cap yet */
 	switch (op) {
-- 
2.27.0

Re: [PATCH 1/2] ceph: fix inode reference leakage in ceph_get_snapdir()

[Jeff Layton]

On Wed, 2022-03-02 at 16:54 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The ceph_get_inode() will search for or insert a new inode into the
> hash for the given vino, and return a reference to it. If new is
> non-NULL, its reference is consumed.
> 
> We should release the reference when in error handing cases.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/inode.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..cbeba8a93a07 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -164,13 +164,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>  	if (!S_ISDIR(parent->i_mode)) {
>  		pr_warn_once("bad snapdir parent type (mode=0%o)\n",
>  			     parent->i_mode);
> -		return ERR_PTR(-ENOTDIR);
> +		goto err;
>  	}
>  
>  	if (!(inode->i_state & I_NEW) && !S_ISDIR(inode->i_mode)) {
>  		pr_warn_once("bad snapdir inode type (mode=0%o)\n",
>  			     inode->i_mode);
> -		return ERR_PTR(-ENOTDIR);
> +		goto err;
>  	}
>  
>  	inode->i_mode = parent->i_mode;
> @@ -190,6 +190,12 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>  	}
>  
>  	return inode;
> +err:
> +	if ((inode->i_state & I_NEW))
> +		discard_new_inode(inode);
> +	else
> +		iput(inode);
> +	return ERR_PTR(-ENOTDIR);
>  }
>  
>  const struct inode_operations ceph_file_iops = {

Good catch!

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps()

[Jeff Layton]

On Wed, 2022-03-02 at 16:54 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The ceph_find_inode() may will fail and return NULL.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/caps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 0b36020207fd..0762b55fdbcb 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4303,7 +4303,6 @@ void ceph_handle_caps(struct ceph_mds_session *session,
>  
>  	/* lookup ino */
>  	inode = ceph_find_inode(mdsc->fsc->sb, vino);
> -	ci = ceph_inode(inode);
>  	dout(" op %s ino %llx.%llx inode %p\n", ceph_cap_op_name(op), vino.ino,
>  	     vino.snap, inode);
>  
> @@ -4333,6 +4332,7 @@ void ceph_handle_caps(struct ceph_mds_session *session,
>  		}
>  		goto flush_cap_releases;
>  	}
> +	ci = ceph_inode(inode);
>  
>  	/* these will work even if we don't have a cap yet */
>  	switch (op) {

I don't think this is an actual bug. We're just assigning "ci" here and
that doesn't involve a dereference of inode. If "inode" is NULL, then ci
will be close to NULL, but it doesn't get used in that case.

Assigning this lower in the function is fine though, and it discourages
anyone trying to use ci when they shouldn't, so you can add my ack, but
maybe fix the patch description since there is no dereference here.

Acked-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] ceph: fix a NULL pointer dereference in ceph_handle_caps()

[Xiubo Li]

On 3/2/22 10:06 PM, Jeff Layton wrote:
> On Wed, 2022-03-02 at 16:54 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The ceph_find_inode() may will fail and return NULL.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/caps.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> index 0b36020207fd..0762b55fdbcb 100644
>> --- a/fs/ceph/caps.c
>> +++ b/fs/ceph/caps.c
>> @@ -4303,7 +4303,6 @@ void ceph_handle_caps(struct ceph_mds_session *session,
>>   
>>   	/* lookup ino */
>>   	inode = ceph_find_inode(mdsc->fsc->sb, vino);
>> -	ci = ceph_inode(inode);
>>   	dout(" op %s ino %llx.%llx inode %p\n", ceph_cap_op_name(op), vino.ino,
>>   	     vino.snap, inode);
>>   
>> @@ -4333,6 +4332,7 @@ void ceph_handle_caps(struct ceph_mds_session *session,
>>   		}
>>   		goto flush_cap_releases;
>>   	}
>> +	ci = ceph_inode(inode);
>>   
>>   	/* these will work even if we don't have a cap yet */
>>   	switch (op) {
> I don't think this is an actual bug. We're just assigning "ci" here and
> that doesn't involve a dereference of inode. If "inode" is NULL, then ci
> will be close to NULL, but it doesn't get used in that case.

Right, I misread the code.

> Assigning this lower in the function is fine though, and it discourages
> anyone trying to use ci when they shouldn't, so you can add my ack, but
> maybe fix the patch description since there is no dereference here.
>
> Acked-by: Jeff Layton <jlayton@kernel.org>

Sure, thanks.