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@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;
Sure, will send the second version.
-- Xiubo
The bug and fix look real though.
