ceph: fix potential ERR_PTR dereference in ceph_submit_write()

[PATCH] ceph: fix potential ERR_PTR dereference in ceph_submit_write()

Posted by Hongling Zeng 1 month, 4 weeks ago

Fix smatch warning:
 fs/ceph/addr.c:1489 ceph_submit_write() error: 'req' dereferencing possible ERR_PTR()

Add a check to ensure req is not an ERR_PTR before calling
ceph_osdc_put_request().

Fixes: 1551ec61dc55 ("ceph: improve error handling in writeback")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 fs/ceph/addr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index fa4f25f99409..6091818674be 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1468,7 +1468,8 @@ int ceph_submit_write(struct address_space *mapping,
 			unlock_page(page);
 		}
 
-		ceph_osdc_put_request(req);
+		if (!IS_ERR(req))
+			ceph_osdc_put_request(req);
 		return -EIO;
 	}
 
-- 
2.25.1

Re: [PATCH] ceph: fix potential ERR_PTR dereference in ceph_submit_write()

Posted by Viacheslav Dubeyko 1 month, 4 weeks ago

On Fri, 2026-04-17 at 16:06 +0800, Hongling Zeng wrote:
> Fix smatch warning:
>  fs/ceph/addr.c:1489 ceph_submit_write() error: 'req' dereferencing possible ERR_PTR()
> 
> Add a check to ensure req is not an ERR_PTR before calling
> ceph_osdc_put_request().
> 
> Fixes: 1551ec61dc55 ("ceph: improve error handling in writeback")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>  fs/ceph/addr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index fa4f25f99409..6091818674be 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -1468,7 +1468,8 @@ int ceph_submit_write(struct address_space *mapping,
>  			unlock_page(page);
>  		}
>  
> -		ceph_osdc_put_request(req);
> +		if (!IS_ERR(req))
> +			ceph_osdc_put_request(req);
>  		return -EIO;
>  	}
>  

I don't think that this fix makes sense. We create and check the request here
[1]:

	req = ceph_osdc_new_request(&fsc->client->osdc,
				    &ci->i_layout, vino,
				    offset, &len, 0, ceph_wbc->num_ops,
				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE,
				    ceph_wbc->snapc, ceph_wbc->truncate_seq,
				    ceph_wbc->truncate_size, false);
	if (IS_ERR(req)) {
		req = ceph_osdc_new_request(&fsc->client->osdc,
					    &ci->i_layout, vino,
					    offset, &len, 0,
					    min(ceph_wbc->num_ops,
						CEPH_OSD_SLAB_OPS),
					    CEPH_OSD_OP_WRITE,
					    CEPH_OSD_FLAG_WRITE,
					    ceph_wbc->snapc,
					    ceph_wbc->truncate_seq,
					    ceph_wbc->truncate_size,
					    true);
		BUG_ON(IS_ERR(req));
	}

So, it should be the valid request pointer when we called
ceph_osdc_put_request(). Am I missing something?

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v7.0/source/fs/ceph/addr.c#L1421

Re: [PATCH] ceph: fix potential ERR_PTR dereference in ceph_submit_write()

Posted by nana 1 month, 3 weeks ago

  Hi  Slava
       You're correct: the fix doesn't make sense.The 
BUG_ON(IS_ERR(req))  ensures that req is a valid pointer, There is no 
code path that can skip the allocation code.

Thank you for your review.

在 2026年04月18日 01:48, Viacheslav Dubeyko 写道:
> On Fri, 2026-04-17 at 16:06 +0800, Hongling Zeng wrote:
>> Fix smatch warning:
>>   fs/ceph/addr.c:1489 ceph_submit_write() error: 'req' dereferencing possible ERR_PTR()
>>
>> Add a check to ensure req is not an ERR_PTR before calling
>> ceph_osdc_put_request().
>>
>> Fixes: 1551ec61dc55 ("ceph: improve error handling in writeback")
>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>> ---
>>   fs/ceph/addr.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>> index fa4f25f99409..6091818674be 100644
>> --- a/fs/ceph/addr.c
>> +++ b/fs/ceph/addr.c
>> @@ -1468,7 +1468,8 @@ int ceph_submit_write(struct address_space *mapping,
>>   			unlock_page(page);
>>   		}
>>   
>> -		ceph_osdc_put_request(req);
>> +		if (!IS_ERR(req))
>> +			ceph_osdc_put_request(req);
>>   		return -EIO;
>>   	}
>>   
> I don't think that this fix makes sense. We create and check the request here
> [1]:
>
> 	req = ceph_osdc_new_request(&fsc->client->osdc,
> 				    &ci->i_layout, vino,
> 				    offset, &len, 0, ceph_wbc->num_ops,
> 				    CEPH_OSD_OP_WRITE, CEPH_OSD_FLAG_WRITE,
> 				    ceph_wbc->snapc, ceph_wbc->truncate_seq,
> 				    ceph_wbc->truncate_size, false);
> 	if (IS_ERR(req)) {
> 		req = ceph_osdc_new_request(&fsc->client->osdc,
> 					    &ci->i_layout, vino,
> 					    offset, &len, 0,
> 					    min(ceph_wbc->num_ops,
> 						CEPH_OSD_SLAB_OPS),
> 					    CEPH_OSD_OP_WRITE,
> 					    CEPH_OSD_FLAG_WRITE,
> 					    ceph_wbc->snapc,
> 					    ceph_wbc->truncate_seq,
> 					    ceph_wbc->truncate_size,
> 					    true);
> 		BUG_ON(IS_ERR(req));
> 	}
>
> So, it should be the valid request pointer when we called
> ceph_osdc_put_request(). Am I missing something?
>
> Thanks,
> Slava.
>
> [1] https://elixir.bootlin.com/linux/v7.0/source/fs/ceph/addr.c#L1421