| Message ID | 20220414054324.374694-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix possible NULL pointer dereference for req->r_session | expand |
On Thu, 2022-04-14 at 13:43 +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 | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 69af17df59be..6a9bf58478c8 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -2333,7 +2333,7 @@ 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 (unlikely(s->s_mds >= max_sessions)) { > + if (unlikely(s && s->s_mds >= max_sessions)) { > spin_unlock(&ci->i_unsafe_lock); > for (i = 0; i < max_sessions; i++) { > s = sessions[i]; > @@ -2343,7 +2343,7 @@ static int unsafe_request_wait(struct inode *inode) > kfree(sessions); > goto retry; > } > - if (!sessions[s->s_mds]) { > + if (s && !sessions[s->s_mds]) { > s = ceph_get_mds_session(s); > sessions[s->s_mds] = s; > } > @@ -2353,7 +2353,7 @@ 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 (unlikely(s->s_mds >= max_sessions)) { > + if (unlikely(s && s->s_mds >= max_sessions)) { > spin_unlock(&ci->i_unsafe_lock); > for (i = 0; i < max_sessions; i++) { > s = sessions[i]; > @@ -2363,7 +2363,7 @@ static int unsafe_request_wait(struct inode *inode) > kfree(sessions); > goto retry; > } > - if (!sessions[s->s_mds]) { > + if (s && !sessions[s->s_mds]) { > s = ceph_get_mds_session(s); > sessions[s->s_mds] = s; > } Good catch. I think it'd be cleaner to just do this in each loop though, to keep the if conditions simpler: s = req->r_session; if (!s) continue; The bug and fix look real though.
On 4/15/22 8:05 PM, Jeff Layton wrote: > On Thu, 2022-04-14 at 13:43 +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 | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 69af17df59be..6a9bf58478c8 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -2333,7 +2333,7 @@ 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 (unlikely(s->s_mds >= max_sessions)) { >> + if (unlikely(s && s->s_mds >= max_sessions)) { >> spin_unlock(&ci->i_unsafe_lock); >> for (i = 0; i < max_sessions; i++) { >> s = sessions[i]; >> @@ -2343,7 +2343,7 @@ static int unsafe_request_wait(struct inode *inode) >> kfree(sessions); >> goto retry; >> } >> - if (!sessions[s->s_mds]) { >> + if (s && !sessions[s->s_mds]) { >> s = ceph_get_mds_session(s); >> sessions[s->s_mds] = s; >> } >> @@ -2353,7 +2353,7 @@ 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 (unlikely(s->s_mds >= max_sessions)) { >> + if (unlikely(s && s->s_mds >= max_sessions)) { >> spin_unlock(&ci->i_unsafe_lock); >> for (i = 0; i < max_sessions; i++) { >> s = sessions[i]; >> @@ -2363,7 +2363,7 @@ static int unsafe_request_wait(struct inode *inode) >> kfree(sessions); >> goto retry; >> } >> - if (!sessions[s->s_mds]) { >> + if (s && !sessions[s->s_mds]) { >> s = ceph_get_mds_session(s); >> sessions[s->s_mds] = s; >> } > Good catch. I think it'd be cleaner to just do this in each loop though, > to keep the if conditions simpler: > > s = req->r_session; > if (!s) > continue; Sure, will send the second version. -- Xiubo > The bug and fix look real though.
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 69af17df59be..6a9bf58478c8 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -2333,7 +2333,7 @@ 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 (unlikely(s->s_mds >= max_sessions)) { + if (unlikely(s && s->s_mds >= max_sessions)) { spin_unlock(&ci->i_unsafe_lock); for (i = 0; i < max_sessions; i++) { s = sessions[i]; @@ -2343,7 +2343,7 @@ static int unsafe_request_wait(struct inode *inode) kfree(sessions); goto retry; } - if (!sessions[s->s_mds]) { + if (s && !sessions[s->s_mds]) { s = ceph_get_mds_session(s); sessions[s->s_mds] = s; } @@ -2353,7 +2353,7 @@ 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 (unlikely(s->s_mds >= max_sessions)) { + if (unlikely(s && s->s_mds >= max_sessions)) { spin_unlock(&ci->i_unsafe_lock); for (i = 0; i < max_sessions; i++) { s = sessions[i]; @@ -2363,7 +2363,7 @@ static int unsafe_request_wait(struct inode *inode) kfree(sessions); goto retry; } - if (!sessions[s->s_mds]) { + if (s && !sessions[s->s_mds]) { s = ceph_get_mds_session(s); sessions[s->s_mds] = s; }
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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)