[PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Xiubo Li]

The request will be inserted into the ci->i_unsafe_dirops before
assigning the req->r_session, so it's possible that we will hit
NULL pointer dereference bug here.

URL: https://tracker.ceph.com/issues/55327
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/caps.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 69af17df59be..c70fd747c914 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -2333,6 +2333,8 @@ static int unsafe_request_wait(struct inode *inode)
 			list_for_each_entry(req, &ci->i_unsafe_dirops,
 					    r_unsafe_dir_item) {
 				s = req->r_session;
+				if (!s)
+					continue;
 				if (unlikely(s->s_mds >= max_sessions)) {
 					spin_unlock(&ci->i_unsafe_lock);
 					for (i = 0; i < max_sessions; i++) {
@@ -2353,6 +2355,8 @@ static int unsafe_request_wait(struct inode *inode)
 			list_for_each_entry(req, &ci->i_unsafe_iops,
 					    r_unsafe_target_item) {
 				s = req->r_session;
+				if (!s)
+					continue;
 				if (unlikely(s->s_mds >= max_sessions)) {
 					spin_unlock(&ci->i_unsafe_lock);
 					for (i = 0; i < max_sessions; i++) {
-- 
2.36.0.rc1

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Jeff Layton]

On Mon, 2022-04-18 at 09:44 +0800, Xiubo Li wrote:
> The request will be inserted into the ci->i_unsafe_dirops before
> assigning the req->r_session, so it's possible that we will hit
> NULL pointer dereference bug here.
> 
> URL: https://tracker.ceph.com/issues/55327
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/caps.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 69af17df59be..c70fd747c914 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -2333,6 +2333,8 @@ static int unsafe_request_wait(struct inode *inode)
>  			list_for_each_entry(req, &ci->i_unsafe_dirops,
>  					    r_unsafe_dir_item) {
>  				s = req->r_session;
> +				if (!s)
> +					continue;
>  				if (unlikely(s->s_mds >= max_sessions)) {
>  					spin_unlock(&ci->i_unsafe_lock);
>  					for (i = 0; i < max_sessions; i++) {
> @@ -2353,6 +2355,8 @@ static int unsafe_request_wait(struct inode *inode)
>  			list_for_each_entry(req, &ci->i_unsafe_iops,
>  					    r_unsafe_target_item) {
>  				s = req->r_session;
> +				if (!s)
> +					continue;
>  				if (unlikely(s->s_mds >= max_sessions)) {
>  					spin_unlock(&ci->i_unsafe_lock);
>  					for (i = 0; i < max_sessions; i++) {

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

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Aaron Tomlin]

On Mon 2022-04-18 09:44 +0800, Xiubo Li wrote:
> The request will be inserted into the ci->i_unsafe_dirops before
> assigning the req->r_session, so it's possible that we will hit
> NULL pointer dereference bug here.
> 
> URL: https://tracker.ceph.com/issues/55327
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/caps.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 69af17df59be..c70fd747c914 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -2333,6 +2333,8 @@ static int unsafe_request_wait(struct inode *inode)
>  			list_for_each_entry(req, &ci->i_unsafe_dirops,
>  					    r_unsafe_dir_item) {
>  				s = req->r_session;
> +				if (!s)
> +					continue;
>  				if (unlikely(s->s_mds >= max_sessions)) {
>  					spin_unlock(&ci->i_unsafe_lock);
>  					for (i = 0; i < max_sessions; i++) {
> @@ -2353,6 +2355,8 @@ static int unsafe_request_wait(struct inode *inode)
>  			list_for_each_entry(req, &ci->i_unsafe_iops,
>  					    r_unsafe_target_item) {
>  				s = req->r_session;
> +				if (!s)
> +					continue;
>  				if (unlikely(s->s_mds >= max_sessions)) {
>  					spin_unlock(&ci->i_unsafe_lock);
>  					for (i = 0; i < max_sessions; i++) {
> -- 
> 2.36.0.rc1

Thanks Xiubo!

Reviewed-by: Aaron Tomlin <atomlin@redhat.com>

-- 
Aaron Tomlin

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Aaron Tomlin]

On Mon 2022-04-18 09:44 +0800, Xiubo Li wrote:
> The request will be inserted into the ci->i_unsafe_dirops before
> assigning the req->r_session, so it's possible that we will hit
> NULL pointer dereference bug here.
> 
> URL: https://tracker.ceph.com/issues/55327
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/caps.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 69af17df59be..c70fd747c914 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -2333,6 +2333,8 @@ static int unsafe_request_wait(struct inode *inode)
>  			list_for_each_entry(req, &ci->i_unsafe_dirops,
>  					    r_unsafe_dir_item) {
>  				s = req->r_session;
> +				if (!s)
> +					continue;
>  				if (unlikely(s->s_mds >= max_sessions)) {
>  					spin_unlock(&ci->i_unsafe_lock);
>  					for (i = 0; i < max_sessions; i++) {
> @@ -2353,6 +2355,8 @@ static int unsafe_request_wait(struct inode *inode)
>  			list_for_each_entry(req, &ci->i_unsafe_iops,
>  					    r_unsafe_target_item) {
>  				s = req->r_session;
> +				if (!s)
> +					continue;
>  				if (unlikely(s->s_mds >= max_sessions)) {
>  					spin_unlock(&ci->i_unsafe_lock);
>  					for (i = 0; i < max_sessions; i++) {
> -- 
> 2.36.0.rc1

Tested-by: Aaron Tomlin <atomlin@redhat.com>

-- 
Aaron Tomlin

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Xiubo Li]

On 4/18/22 6:43 PM, Aaron Tomlin wrote:
> On Mon 2022-04-18 09:44 +0800, Xiubo Li wrote:
>> The request will be inserted into the ci->i_unsafe_dirops before
>> assigning the req->r_session, so it's possible that we will hit
>> NULL pointer dereference bug here.
>>
>> URL: https://tracker.ceph.com/issues/55327
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/caps.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> index 69af17df59be..c70fd747c914 100644
>> --- a/fs/ceph/caps.c
>> +++ b/fs/ceph/caps.c
>> @@ -2333,6 +2333,8 @@ static int unsafe_request_wait(struct inode *inode)
>>   			list_for_each_entry(req, &ci->i_unsafe_dirops,
>>   					    r_unsafe_dir_item) {
>>   				s = req->r_session;
>> +				if (!s)
>> +					continue;
>>   				if (unlikely(s->s_mds >= max_sessions)) {
>>   					spin_unlock(&ci->i_unsafe_lock);
>>   					for (i = 0; i < max_sessions; i++) {
>> @@ -2353,6 +2355,8 @@ static int unsafe_request_wait(struct inode *inode)
>>   			list_for_each_entry(req, &ci->i_unsafe_iops,
>>   					    r_unsafe_target_item) {
>>   				s = req->r_session;
>> +				if (!s)
>> +					continue;
>>   				if (unlikely(s->s_mds >= max_sessions)) {
>>   					spin_unlock(&ci->i_unsafe_lock);
>>   					for (i = 0; i < max_sessions; i++) {
>> -- 
>> 2.36.0.rc1
> Tested-by: Aaron Tomlin <atomlin@redhat.com>
>
Hi Aaron,

Thanks very much for you testing.

BTW, did you test this by using Livepatch or something else ?

-- Xiubo

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Aaron Tomlin]

On Mon 2022-04-18 18:52 +0800, Xiubo Li wrote:
> Hi Aaron,

Hi Xiubo,

> Thanks very much for you testing.

No problem!

> BTW, did you test this by using Livepatch or something else ?

I mostly followed your suggestion here [1] by modifying/or patching the
kernel to increase the race window so that unsafe_request_wait() may more
reliably see a newly registered request with an unprepared session pointer
i.e. 'req->r_session == NULL'.

Indeed, with this patch we simply skip such a request while traversing the
Ceph inode's unsafe directory list in the context of unsafe_request_wait().

[1]: https://tracker.ceph.com/issues/55329

Kind regards,

-- 
Aaron Tomlin

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Xiubo Li]

On 4/18/22 10:45 PM, Aaron Tomlin wrote:
> On Mon 2022-04-18 18:52 +0800, Xiubo Li wrote:
>> Hi Aaron,
> Hi Xiubo,
>
>> Thanks very much for you testing.
> No problem!
>
>> BTW, did you test this by using Livepatch or something else ?
> I mostly followed your suggestion here [1] by modifying/or patching the
> kernel to increase the race window so that unsafe_request_wait() may more
> reliably see a newly registered request with an unprepared session pointer
> i.e. 'req->r_session == NULL'.
>
> Indeed, with this patch we simply skip such a request while traversing the
> Ceph inode's unsafe directory list in the context of unsafe_request_wait().

Okay, cool.

Thanks again for your help Aaron :-)

-- Xiubo


> [1]: https://tracker.ceph.com/issues/55329
>
> Kind regards,
>

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Ilya Dryomov]

On Tue, Apr 19, 2022 at 3:01 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 4/18/22 10:45 PM, Aaron Tomlin wrote:
> > On Mon 2022-04-18 18:52 +0800, Xiubo Li wrote:
> >> Hi Aaron,
> > Hi Xiubo,
> >
> >> Thanks very much for you testing.
> > No problem!
> >
> >> BTW, did you test this by using Livepatch or something else ?
> > I mostly followed your suggestion here [1] by modifying/or patching the
> > kernel to increase the race window so that unsafe_request_wait() may more
> > reliably see a newly registered request with an unprepared session pointer
> > i.e. 'req->r_session == NULL'.
> >
> > Indeed, with this patch we simply skip such a request while traversing the
> > Ceph inode's unsafe directory list in the context of unsafe_request_wait().
>
> Okay, cool.
>
> Thanks again for your help Aaron :-)
>
> -- Xiubo
>
>
> > [1]: https://tracker.ceph.com/issues/55329
> >
> > Kind regards,
> >
>

I went ahead and marked this for stable (it's already queued for -rc5).

Thanks,

                Ilya

Re: [PATCH v2] ceph: fix possible NULL pointer dereference for req->r_session

[Xiubo Li]

On 4/25/22 5:03 PM, Ilya Dryomov wrote:
> On Tue, Apr 19, 2022 at 3:01 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 4/18/22 10:45 PM, Aaron Tomlin wrote:
>>> On Mon 2022-04-18 18:52 +0800, Xiubo Li wrote:
>>>> Hi Aaron,
>>> Hi Xiubo,
>>>
>>>> Thanks very much for you testing.
>>> No problem!
>>>
>>>> BTW, did you test this by using Livepatch or something else ?
>>> I mostly followed your suggestion here [1] by modifying/or patching the
>>> kernel to increase the race window so that unsafe_request_wait() may more
>>> reliably see a newly registered request with an unprepared session pointer
>>> i.e. 'req->r_session == NULL'.
>>>
>>> Indeed, with this patch we simply skip such a request while traversing the
>>> Ceph inode's unsafe directory list in the context of unsafe_request_wait().
>> Okay, cool.
>>
>> Thanks again for your help Aaron :-)
>>
>> -- Xiubo
>>
>>
>>> [1]: https://tracker.ceph.com/issues/55329
>>>
>>> Kind regards,
>>>
> I went ahead and marked this for stable (it's already queued for -rc5).

Sure, thanks Ilya.

-- Xiubo


> Thanks,
>
>                  Ilya
>