[PATCH 5.10] nvme: use nvme_cid to generate command_id in trace event

[PATCH 5.10] nvme: use nvme_cid to generate command_id in trace event

[Li Lingfeng]

From: Li Lingfeng <lilingfeng3@huawei.com>

A null-ptr-deref problem may occur since commit 706960d328f5 ("nvme: use
command_id instead of req->tag in trace_nvme_complete_rq()") tries to get
command_id by nvme_req(req)->cmd while nvme_req(req)->cmd is NULL.
The problem has been sloved since the patch has been reverted by commit
929ba86476b3. However, cmd->common.command_id is set to req->tag again
which should be ((genctl & 0xf)< 12 | req->tag).
Generating command_id by nvme_cid() in trace event instead of
nvme_req(req)->cmd->common.command_id to set it to
((genctl & 0xf)< 12 | req->tag) without trigging the null-ptr-deref
problem.

Fixes: commit 706960d328f5 ("nvme: use command_id instead of req->tag in trace_nvme_complete_rq()")
Reported-by: John Sperbeck <jsperbeck@google.com>
Link: https://lore.kernel.org/r/20240109181722.228783-1-jsperbeck@google.com
Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
 drivers/nvme/host/trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index 700fdce2ecf1..0de057a298dd 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -98,7 +98,7 @@ TRACE_EVENT(nvme_complete_rq,
 	    TP_fast_assign(
 		__entry->ctrl_id = nvme_req(req)->ctrl->instance;
 		__entry->qid = nvme_req_qid(req);
-		__entry->cid = req->tag;
+		__entry->cid = nvme_cid(req);
 		__entry->result = le64_to_cpu(nvme_req(req)->result.u64);
 		__entry->retries = nvme_req(req)->retries;
 		__entry->flags = nvme_req(req)->flags;
-- 
2.31.1

Re: [PATCH 5.10] nvme: use nvme_cid to generate command_id in trace event

[Greg KH]

On Wed, Mar 06, 2024 at 07:25:06PM +0800, Li Lingfeng wrote:
> From: Li Lingfeng <lilingfeng3@huawei.com>
> 
> A null-ptr-deref problem may occur since commit 706960d328f5 ("nvme: use
> command_id instead of req->tag in trace_nvme_complete_rq()") tries to get
> command_id by nvme_req(req)->cmd while nvme_req(req)->cmd is NULL.
> The problem has been sloved since the patch has been reverted by commit
> 929ba86476b3. However, cmd->common.command_id is set to req->tag again
> which should be ((genctl & 0xf)< 12 | req->tag).
> Generating command_id by nvme_cid() in trace event instead of
> nvme_req(req)->cmd->common.command_id to set it to
> ((genctl & 0xf)< 12 | req->tag) without trigging the null-ptr-deref
> problem.
> 
> Fixes: commit 706960d328f5 ("nvme: use command_id instead of req->tag in trace_nvme_complete_rq()")

This committ is reverted in the 5.10.208 release, so is this change
still needed?

thanks,

greg k-h

Re: [PATCH 5.10] nvme: use nvme_cid to generate command_id in trace event

[Li Lingfeng]

在 2024/3/29 19:59, Greg KH 写道:
> On Wed, Mar 06, 2024 at 07:25:06PM +0800, Li Lingfeng wrote:
>> From: Li Lingfeng <lilingfeng3@huawei.com>
>>
>> A null-ptr-deref problem may occur since commit 706960d328f5 ("nvme: use
>> command_id instead of req->tag in trace_nvme_complete_rq()") tries to get
>> command_id by nvme_req(req)->cmd while nvme_req(req)->cmd is NULL.
>> The problem has been sloved since the patch has been reverted by commit
>> 929ba86476b3. However, cmd->common.command_id is set to req->tag again
>> which should be ((genctl & 0xf)< 12 | req->tag).
>> Generating command_id by nvme_cid() in trace event instead of
>> nvme_req(req)->cmd->common.command_id to set it to
>> ((genctl & 0xf)< 12 | req->tag) without trigging the null-ptr-deref
>> problem.
>>
>> Fixes: commit 706960d328f5 ("nvme: use command_id instead of req->tag in trace_nvme_complete_rq()")
> This committ is reverted in the 5.10.208 release, so is this change
> still needed?
>
> thanks,
>
> greg k-h
As described by commit 706960d328f5 ("nvme: use command_id instead of 
req->tag in trace_nvme_complete_rq()"), we should use command_id instead 
of req->tag in trace_nvme_complete_rq(). So I don't think it's 
appropriate to just revert it.

Replacing req->tag with nvme_cid(req) can solve the problem described by 
commit 706960d328f5 without causing issues mentioned in commit 
929ba86476b3 ("Revert "nvme: use command_id instead of req->tag in 
trace_nvme_complete_rq()"").

Maybe fix tag should be changed to "Fixes: commit 929ba86476b3 ("Revert 
"nvme: use command_id instead of req->tag in trace_nvme_complete_rq()"")"?

Thanks.

Re: [PATCH 5.10] nvme: use nvme_cid to generate command_id in trace event

[Greg KH]

On Sat, Mar 30, 2024 at 09:30:57AM +0800, Li Lingfeng wrote:
> 
> 在 2024/3/29 19:59, Greg KH 写道:
> > On Wed, Mar 06, 2024 at 07:25:06PM +0800, Li Lingfeng wrote:
> > > From: Li Lingfeng <lilingfeng3@huawei.com>
> > > 
> > > A null-ptr-deref problem may occur since commit 706960d328f5 ("nvme: use
> > > command_id instead of req->tag in trace_nvme_complete_rq()") tries to get
> > > command_id by nvme_req(req)->cmd while nvme_req(req)->cmd is NULL.
> > > The problem has been sloved since the patch has been reverted by commit
> > > 929ba86476b3. However, cmd->common.command_id is set to req->tag again
> > > which should be ((genctl & 0xf)< 12 | req->tag).
> > > Generating command_id by nvme_cid() in trace event instead of
> > > nvme_req(req)->cmd->common.command_id to set it to
> > > ((genctl & 0xf)< 12 | req->tag) without trigging the null-ptr-deref
> > > problem.
> > > 
> > > Fixes: commit 706960d328f5 ("nvme: use command_id instead of req->tag in trace_nvme_complete_rq()")
> > This committ is reverted in the 5.10.208 release, so is this change
> > still needed?
> > 
> > thanks,
> > 
> > greg k-h
> As described by commit 706960d328f5 ("nvme: use command_id instead of
> req->tag in trace_nvme_complete_rq()"), we should use command_id instead of
> req->tag in trace_nvme_complete_rq(). So I don't think it's appropriate to
> just revert it.

It's been reverted, and is in a release, so if you want something
changed here, it needs to be sent as a new change.

thanks,

greg k-h