Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-04-01 Thread Sagi Grimberg
request tag can't be zero? I forget... Of course it can. But the reserved tags are before the normal tags, so 0 would be a reserved tag for nvme. Right.

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-04-01 Thread Christoph Hellwig
On Wed, Mar 31, 2021 at 03:24:49PM -0700, Sagi Grimberg wrote: > >>> What we can do, though, is checking the 'state' field in the tcp >>> request, and only allow completions for commands which are in a state >>> allowing for completions. >>> >>> Let's see if I can whip up a patch. >> >> That would

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-31 Thread Sagi Grimberg
It is, but in this situation, the controller is sending a second completion that results in a use-after-free, which makes the transport irrelevant. Unless there is some other flow (which is unclear to me) that causes this which is a bug that needs to be fixed rather than hidden with a

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-31 Thread Sagi Grimberg
What we can do, though, is checking the 'state' field in the tcp request, and only allow completions for commands which are in a state allowing for completions. Let's see if I can whip up a patch. That would be great. BTW in the crash dump I am looking at now, it looks like pdu->command_id

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-31 Thread Ewan D. Milne
On Wed, 2021-03-31 at 09:11 +0200, Hannes Reinecke wrote: > On 3/31/21 1:28 AM, Keith Busch wrote: > > On Tue, Mar 30, 2021 at 10:34:25AM -0700, Sagi Grimberg wrote: > > > > > > > > It is, but in this situation, the controller is sending a > > > > > second > > > > > completion that results in a

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-31 Thread Hannes Reinecke
On 3/31/21 1:28 AM, Keith Busch wrote: > On Tue, Mar 30, 2021 at 10:34:25AM -0700, Sagi Grimberg wrote: >> It is, but in this situation, the controller is sending a second completion that results in a use-after-free, which makes the transport irrelevant. Unless there is some other

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-30 Thread Keith Busch
On Tue, Mar 30, 2021 at 10:34:25AM -0700, Sagi Grimberg wrote: > > > > It is, but in this situation, the controller is sending a second > > > completion that results in a use-after-free, which makes the > > > transport irrelevant. Unless there is some other flow (which is > > > unclear > > > to

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-30 Thread Sagi Grimberg
It is, but in this situation, the controller is sending a second completion that results in a use-after-free, which makes the transport irrelevant. Unless there is some other flow (which is unclear to me) that causes this which is a bug that needs to be fixed rather than hidden with a

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-30 Thread Ewan D. Milne
On Mon, 2021-03-15 at 10:16 -0700, Sagi Grimberg wrote: > > Hi Sagi, > > > > On Fri, Mar 05, 2021 at 11:57:30AM -0800, Sagi Grimberg wrote: > > > Daniel, again, there is nothing specific about this to nvme-tcp, > > > this is a safeguard against a funky controller (or a different > > > bug that is

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-15 Thread Sagi Grimberg
Hi Sagi, On Fri, Mar 05, 2021 at 11:57:30AM -0800, Sagi Grimberg wrote: Daniel, again, there is nothing specific about this to nvme-tcp, this is a safeguard against a funky controller (or a different bug that is hidden by this). As far I can tell, the main difference between nvme-tcp and

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-11 Thread Daniel Wagner
Hi Sagi, On Fri, Mar 05, 2021 at 11:57:30AM -0800, Sagi Grimberg wrote: > Daniel, again, there is nothing specific about this to nvme-tcp, > this is a safeguard against a funky controller (or a different > bug that is hidden by this). As far I can tell, the main difference between nvme-tcp and

Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-05 Thread Sagi Grimberg
blk_mq_tag_to_rq() always returns a request if the tag id is in a valid range [0...max_tags). If the target replies with a tag for which we don't have a request but it's not started, the host will likely corrupt data or simply crash. Add an additional check if the a request has been started

[PATCH v2] nvme-tcp: Check if request has started before processing it

2021-03-01 Thread Daniel Wagner
blk_mq_tag_to_rq() always returns a request if the tag id is in a valid range [0...max_tags). If the target replies with a tag for which we don't have a request but it's not started, the host will likely corrupt data or simply crash. Add an additional check if the a request has been started if