I got a kernel NULL pointer derference report as below: BUG: kernel NULL pointer dereference, address: 0000000000000038 CPU: 4 PID: 1050 Comm: cat Not tainted 5.19.0 #38 RIP: 0010:kernel_getpeername+0xf/0x30 Call Trace: <TASK> ? iscsi_sw_tcp_conn_get_param+0x11f/0x17f show_conn_ep_param_ISCSI_PARAM_CONN_ADDRESS+0x90/0xb0 dev_attr_show+0x1d/0x50 sysfs_kf_seq_show+0xad/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x12e/0x4d0 ? aa_file_perm+0x177/0x590 kernfs_fop_read_iter+0x183/0x210 new_sync_read+0xfe/0x180 ? 0xffffffff81000000 vfs_read+0x14d/0x1a0 ksys_read+0x6d/0xf0 __x64_sys_read+0x1a/0x20 do_syscall_64+0x3b/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd The problem scenario is: CPU1 CPU2 ------------------------- ------------------------ iscsi_sw_tcp_conn_get_param spin_lock_bh(&frwd_lock) if (!tcp_sw_conn || !tcp_sw_conn->sock) spin_unlock_bh(&frwd_lock) return -ENOTCONN sock = tcp_sw_conn->sock; sock_hold(sock->sk) spin_unlock_bh(&frwd_lock) iscsi_sw_tcp_release_conn spin_lock_bh(&frwd_lock) tcp_sw_conn->sock = NULL spin_unlock_bh(&frwd_lock) sockfd_put(sock) task_work __fput sock_close __sock_release sock->sk = NULL sock->ops = NULL sock->file = NULL kernel_getpeername sock->ops->getname ^^^^^^^^^ NULL pointer dereference of sock->ops sock_hold() and sock_put() can't ensure that sock_close() won't be called before calling getpeername() or getsockname(). Using fget() and sockfd_put() replace sock_hold() and sock_put(), and put them under frwd_lock protection, to ensure that the socket struct is preserved until after the getpeernaem() or getsockname() complete. Fixes: bcf3a2953d36 ("scsi: iscsi: iscsi_tcp: Avoid holding spinlock while calling getpeername()") Signed-off-by: Li Jinlin <lijinlin3@xxxxxxxxxx> --- drivers/scsi/iscsi_tcp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 9fee70d6434a..63532dc3970d 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -612,8 +612,8 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn) spin_lock_bh(&session->frwd_lock); tcp_sw_conn->sock = NULL; - spin_unlock_bh(&session->frwd_lock); sockfd_put(sock); + spin_unlock_bh(&session->frwd_lock); } static void iscsi_sw_tcp_conn_destroy(struct iscsi_cls_conn *cls_conn) @@ -756,7 +756,7 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn, return -ENOTCONN; } sock = tcp_sw_conn->sock; - sock_hold(sock->sk); + fget(sock->file); spin_unlock_bh(&conn->session->frwd_lock); if (param == ISCSI_PARAM_LOCAL_PORT) @@ -765,7 +765,9 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn, else rc = kernel_getpeername(sock, (struct sockaddr *)&addr); - sock_put(sock->sk); + spin_lock_bh(&conn->session->frwd_lock); + sockfd_put(sock); + spin_unlock_bh(&conn->session->frwd_lock); if (rc < 0) return rc; @@ -808,12 +810,14 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost, spin_unlock_bh(&session->frwd_lock); return -ENOTCONN; } - sock_hold(sock->sk); + fget(sock->file); spin_unlock_bh(&session->frwd_lock); rc = kernel_getsockname(sock, (struct sockaddr *)&addr); - sock_put(sock->sk); + spin_lock_bh(&conn->session->frwd_lock); + sockfd_put(sock); + spin_unlock_bh(&conn->session->frwd_lock); if (rc < 0) return rc; -- 2.27.0