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@xxxxxxxxxx> > --- > 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. -- Jeff Layton <jlayton@xxxxxxxxxx>
