| Message ID | 20220418014440.573533-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ceph: fix possible NULL pointer dereference for req->r_session | expand |
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>
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>
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>
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
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,
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, >
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
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 >
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++) {
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(+)