Re: [PATCH] nvme: fix NULL pointer dereference
On Thu, Sep 17, 2020 at 11:32:12PM -0400, Tong Zhang wrote: > Please correct me if I am wrong. > After a bit more digging I found out that it is indeed command_id got > corrupted is causing this problem. Although the tag and command_id > range is checked like you said, the elements in rqs cannot be > guaranteed to be not NULL. thus although the range check is passed, > blk_mq_tag_to_rq() can still return NULL. I think your describing a sequence problem in initialization. We shouldn't have interrupts wired up to uninitialized tagsets. A more appropriate sequence would setup request_irq() after the tagset is ready. It makes handling a failed irq setup a bit weird for io queues, though. > It is clear that the current sanitization is not enough and there's > more implication about this -- when all rqs got populated, a corrupted > command_id may silently corrupt other data not belonging to the > current command. The block layer doesn't do anything with requests that haven't been started, so if your controller completes non-existent commands, then nothing particular will happen with the rqs. If the request had been started and the controller provides a corrupted completion, then the fault lies entirely with the controller and you should raise that issue with your vendor. There's no way the driver can distinguish a genuine completion from a corrupted one.
Re: [PATCH] nvme: fix NULL pointer dereference
Please correct me if I am wrong. After a bit more digging I found out that it is indeed command_id got corrupted is causing this problem. Although the tag and command_id range is checked like you said, the elements in rqs cannot be guaranteed to be not NULL. thus although the range check is passed, blk_mq_tag_to_rq() can still return NULL. It is clear that the current sanitization is not enough and there's more implication about this -- when all rqs got populated, a corrupted command_id may silently corrupt other data not belonging to the current command. - Tong On Thu, Sep 17, 2020 at 8:44 PM Tong Zhang wrote: > > Hmm..Yeah.. I see your point. > I was naivly thinking the command_id was the culprit. > > On Thu, Sep 17, 2020 at 1:14 PM Keith Busch wrote: > > > > On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote: > > > The command_id in CQE is writable by NVMe controller, driver should > > > check its sanity before using it. > > > > We already do that.
Re: [PATCH] nvme: fix NULL pointer dereference
Hmm..Yeah.. I see your point. I was naivly thinking the command_id was the culprit. On Thu, Sep 17, 2020 at 1:14 PM Keith Busch wrote: > > On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote: > > The command_id in CQE is writable by NVMe controller, driver should > > check its sanity before using it. > > We already do that.
Re: [PATCH] nvme: fix NULL pointer dereference
On Thu, Sep 17, 2020 at 12:56:59PM -0400, Tong Zhang wrote: > The command_id in CQE is writable by NVMe controller, driver should > check its sanity before using it. We already do that.
Re: [PATCH] nvme: fix NULL pointer dereference
The command_id in CQE is writable by NVMe controller, driver should check its sanity before using it. - Tong On Wed, Sep 16, 2020 at 12:54 PM Keith Busch wrote: > > On Wed, Sep 16, 2020 at 11:36:49AM -0400, Tong Zhang wrote: > > @@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue > > *nvmeq, u16 idx) > > } > > > > req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id); > > + if (!req) > > + return; > > As I mentioned before, blk_mq_tag_to_rq() returns NULL if the tag > exceeds the depth. We already verify the tag prior to calling this > function, so what's the real root cause for how we're winding up with > NULL here? I'm only asking this because it sounds like there's a bug > somewhere else and this change is masking over it. > > > > trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail); > > if (!nvme_try_complete_req(req, cqe->status, cqe->result)) > > nvme_pci_complete_rq(req);
Re: [PATCH] nvme: fix NULL pointer dereference
On Wed, Sep 16, 2020 at 11:36:49AM -0400, Tong Zhang wrote: > @@ -960,6 +960,8 @@ static inline void nvme_handle_cqe(struct nvme_queue > *nvmeq, u16 idx) > } > > req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id); > + if (!req) > + return; As I mentioned before, blk_mq_tag_to_rq() returns NULL if the tag exceeds the depth. We already verify the tag prior to calling this function, so what's the real root cause for how we're winding up with NULL here? I'm only asking this because it sounds like there's a bug somewhere else and this change is masking over it. > trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail); > if (!nvme_try_complete_req(req, cqe->status, cqe->result)) > nvme_pci_complete_rq(req);
