This change fixes the following kernel NULL pointer dereference
which is reproduced by blktests srp/007 occasionally.
BUG: kernel NULL pointer dereference, address: 0000000000000170
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 0 P4D 0
Oops: 0002 [#1] PREEMPT SMP NOPTI
CPU: 0 PID: 9 Comm: kworker/0:1H Kdump: loaded Not tainted 6.0.0-rc1+ #37
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-29-g6a62e0cb0dfe-prebuilt.qemu.org 04/01/2014
Workqueue: 0x0 (kblockd)
RIP: 0010:srp_recv_done+0x176/0x500 [ib_srp]
Code: 00 4d 85 ff 0f 84 52 02 00 00 48 c7 82 80 02 00 00 00 00 00 00 4c 89 df 4c 89 14 24 e8 53 d3 4a f6 4c 8b 14 24 41 0f b6 42 13 <41> 89 87 70 01 00 00 41 0f b6 52 12 f6 c2 02 74 44 41 8b 42 1c b9
RSP: 0018:ffffaef7c0003e28 EFLAGS: 00000282
RAX: 0000000000000000 RBX: ffff9bc9486dea60 RCX: 0000000000000000
RDX: 0000000000000102 RSI: ffffffffb76bbd0e RDI: 00000000ffffffff
RBP: ffff9bc980099a00 R08: 0000000000000001 R09: 0000000000000001
R10: ffff9bca53ef0000 R11: ffff9bc980099a10 R12: ffff9bc956e14000
R13: ffff9bc9836b9cb0 R14: ffff9bc9557b4480 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff9bc97ec00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000170 CR3: 0000000007e04000 CR4: 00000000000006f0
Call Trace:
<IRQ>
__ib_process_cq+0xb7/0x280 [ib_core]
ib_poll_handler+0x2b/0x130 [ib_core]
irq_poll_softirq+0x93/0x150
__do_softirq+0xee/0x4b8
irq_exit_rcu+0xf7/0x130
sysvec_apic_timer_interrupt+0x8e/0xc0
</IRQ>
Fixes: aef9ec39c47f ("IB: Add SCSI RDMA Protocol (SRP) initiator")
Signed-off-by: Xiao Yang <[email protected]>
---
drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7720ea270ed8..528cdd0daba4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
if (scmnd) {
req = scsi_cmd_priv(scmnd);
scmnd = srp_claim_req(ch, req, NULL, scmnd);
+ scmnd->result = rsp->status;
} else {
shost_printk(KERN_ERR, target->scsi_host,
"Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n",
@@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
return;
}
- scmnd->result = rsp->status;
if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {
memcpy(scmnd->sense_buffer, rsp->data +
--
2.34.1
On 8/30/22 18:47, [email protected] wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 7720ea270ed8..528cdd0daba4 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
> if (scmnd) {
> req = scsi_cmd_priv(scmnd);
> scmnd = srp_claim_req(ch, req, NULL, scmnd);
> + scmnd->result = rsp->status;
> } else {
> shost_printk(KERN_ERR, target->scsi_host,
> "Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n",
> @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
>
> return;
> }
> - scmnd->result = rsp->status;
>
> if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {
> memcpy(scmnd->sense_buffer, rsp->data +
Since there is a 'return' statement in the else branch, I don't see how
this patch can make a difference?
Thanks,
Bart.
On 31/08/2022 09:47, [email protected] wrote:
> This change fixes the following kernel NULL pointer dereference
> which is reproduced by blktests srp/007 occasionally.
>
> BUG: kernel NULL pointer dereference, address: 0000000000000170
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0002) - not-present page
> PGD 0 P4D 0
> Oops: 0002 [#1] PREEMPT SMP NOPTI
> CPU: 0 PID: 9 Comm: kworker/0:1H Kdump: loaded Not tainted 6.0.0-rc1+ #37
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-29-g6a62e0cb0dfe-prebuilt.qemu.org 04/01/2014
> Workqueue: 0x0 (kblockd)
> RIP: 0010:srp_recv_done+0x176/0x500 [ib_srp]
> Code: 00 4d 85 ff 0f 84 52 02 00 00 48 c7 82 80 02 00 00 00 00 00 00 4c 89 df 4c 89 14 24 e8 53 d3 4a f6 4c 8b 14 24 41 0f b6 42 13 <41> 89 87 70 01 00 00 41 0f b6 52 12 f6 c2 02 74 44 41 8b 42 1c b9
> RSP: 0018:ffffaef7c0003e28 EFLAGS: 00000282
> RAX: 0000000000000000 RBX: ffff9bc9486dea60 RCX: 0000000000000000
> RDX: 0000000000000102 RSI: ffffffffb76bbd0e RDI: 00000000ffffffff
> RBP: ffff9bc980099a00 R08: 0000000000000001 R09: 0000000000000001
> R10: ffff9bca53ef0000 R11: ffff9bc980099a10 R12: ffff9bc956e14000
> R13: ffff9bc9836b9cb0 R14: ffff9bc9557b4480 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffff9bc97ec00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000170 CR3: 0000000007e04000 CR4: 00000000000006f0
> Call Trace:
> <IRQ>
> __ib_process_cq+0xb7/0x280 [ib_core]
> ib_poll_handler+0x2b/0x130 [ib_core]
> irq_poll_softirq+0x93/0x150
> __do_softirq+0xee/0x4b8
> irq_exit_rcu+0xf7/0x130
> sysvec_apic_timer_interrupt+0x8e/0xc0
> </IRQ>
>
> Fixes: aef9ec39c47f ("IB: Add SCSI RDMA Protocol (SRP) initiator")
> Signed-off-by: Xiao Yang <[email protected]>
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 7720ea270ed8..528cdd0daba4 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
> if (scmnd) {
> req = scsi_cmd_priv(scmnd);
> scmnd = srp_claim_req(ch, req, NULL, scmnd);
> + scmnd->result = rsp->status;
> } else {
> shost_printk(KERN_ERR, target->scsi_host,
> "Null scmnd for RSP w/tag %#016llx received on ch %td / QP %#x\n",
> @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
>
> return;
> }
> - scmnd->result = rsp->status;
What i can see is that we have other places to de-reference scmnd and
scmnd = srp_claim_req(ch, req, NULL, scmnd) is possible to return a NULL to scmnd
Thanks
Zhijian
>
> if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {
> memcpy(scmnd->sense_buffer, rsp->data +
^^^^
On 2022/8/31 10:47, Bart Van Assche wrote:
> On 8/30/22 18:47, [email protected] wrote:
>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>> b/drivers/infiniband/ulp/srp/ib_srp.c
>> index 7720ea270ed8..528cdd0daba4 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -1961,6 +1961,7 @@ static void srp_process_rsp(struct srp_rdma_ch
>> *ch, struct srp_rsp *rsp)
>> if (scmnd) {
>> req = scsi_cmd_priv(scmnd);
>> scmnd = srp_claim_req(ch, req, NULL, scmnd);
>> + scmnd->result = rsp->status;
>> } else {
>> shost_printk(KERN_ERR, target->scsi_host,
>> "Null scmnd for RSP w/tag %#016llx received on
>> ch %td / QP %#x\n",
>> @@ -1972,7 +1973,6 @@ static void srp_process_rsp(struct srp_rdma_ch
>> *ch, struct srp_rsp *rsp)
>> return;
>> }
>> - scmnd->result = rsp->status;
>> if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {
>> memcpy(scmnd->sense_buffer, rsp->data +
>
> Since there is a 'return' statement in the else branch, I don't see how
> this patch can make a difference?
Hi Bart,
Sorry, I didn't make the right fix. I will send v2 patch.
I think scmnd may be set to NULL after srp_claim_req() is called and
then setting scmnd->result can trigger the NULL pointer dereference.
Best Regards,
Xiao Yang
>
> Thanks,
>
> Bart.
On 2022/8/31 9:59, Li Zhijian wrote:
>
> What i can see is that we have other places to de-reference scmnd and
>
> scmnd = srp_claim_req(ch, req, NULL, scmnd) is possible to return a NULL
> to scmnd
Hi,
Thanks for your review.
Yes, it seems better to just check scmnd before setting scmnd->result,
like this:
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index 7720ea270ed8..99f5e7f852b3 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1972,7 +1972,9 @@ static void srp_process_rsp(struct srp_rdma_ch
*ch, struct srp_rsp *rsp)
return;
}
- scmnd->result = rsp->status;
+
+ if (scmnd)
+ scmnd->result = rsp->status;
Best Regards,
Xiao Yang
On 8/30/22 20:16, [email protected] wrote:
> Sorry, I didn't make the right fix. I will send v2 patch.
> I think scmnd may be set to NULL after srp_claim_req() is called and
> then setting scmnd->result can trigger the NULL pointer dereference.
Something like this untested patch may be what you are looking for:
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index 7720ea270ed8..d7f69e593a63 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1961,7 +1961,8 @@ static void srp_process_rsp(struct srp_rdma_ch
*ch, struct srp_rsp *rsp)
if (scmnd) {
req = scsi_cmd_priv(scmnd);
scmnd = srp_claim_req(ch, req, NULL, scmnd);
- } else {
+ }
+ if (!scmnd) {
shost_printk(KERN_ERR, target->scsi_host,
"Null scmnd for RSP w/tag %#016llx received on ch %td / QP
%#x\n",
rsp->tag, ch - target->ch, ch->qp->qp_num);
Bart.
On 31/08/2022 10:31, Yang, Xiao/杨 晓 wrote:
> On 2022/8/31 9:59, Li Zhijian wrote:
>> What i can see is that we have other places to de-reference scmnd and
>>
>> scmnd = srp_claim_req(ch, req, NULL, scmnd) is possible to return a NULL
>> to scmnd
> Hi,
>
> Thanks for your review.
>
> Yes, it seems better to just check scmnd before setting scmnd->result,
> like this:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
> b/drivers/infiniband/ulp/srp/ib_srp.c
> index 7720ea270ed8..99f5e7f852b3 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1972,7 +1972,9 @@ static void srp_process_rsp(struct srp_rdma_ch
> *ch, struct srp_rsp *rsp)
>
> return;
> }
> - scmnd->result = rsp->status;
> +
> + if (scmnd)
> + scmnd->result = rsp->status;
Not really, i think you may need to return directly if !scmnd
>
> Best Regards,
> Xiao Yang