All of lore.kernel.org
 help / color / mirror / Atom feed
From: Keith Busch <kbusch@kernel.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Sagi Grimberg <sagi@grimberg.me>,
	linux-nvme@lists.infradead.org, Christoph Hellwig <hch@lst.de>,
	Daniel Wagner <dwagner@suse.de>
Subject: Re: [PATCH 3/3] nvme: code command_id with a genctr for use-after-free validation
Date: Mon, 17 May 2021 12:46:13 -0700	[thread overview]
Message-ID: <20210517194613.GA2709569@dhcp-10-100-145-180.wdc.com> (raw)
In-Reply-To: <0bd3f659-9691-4f25-6d3b-bdd19546186c@acm.org>

On Mon, May 17, 2021 at 12:09:46PM -0700, Bart Van Assche wrote:
> On 5/17/21 10:59 AM, Sagi Grimberg wrote:
> > We cannot detect a (perhaps buggy) controller that is sending us
> > a completion for a request that was already completed (for example
> > sending a completion twice), this phenomenon was seen in the wild
> > a few times.
> > 
> > So to protect against this, we use the upper 4 msbits of the nvme sqe
> > command_id to use as a 4-bit generation counter and verify it matches
> > the existing request generation that is incrementing on every execution.
> > 
> > The 16-bit command_id structure now is constructed by:
> > | xxxx | xxxxxxxxxxxx |
> >   gen    request tag
> > 
> > This means that we are giving up some possible queue depth as 12 bits
> > allow for a maximum queue depth of 4095 instead of 65536, however we
> > never create such long queues anyways so no real harm done.
> 
> Is a four bit generation counter sufficient? Shouldn't such a counter be
> at least 32 bits to provide a reasonable protection against e.g.
> duplicate packets generated by retry mechanisms in networking stacks?

It has to fit in the protocol's command identifier, and that's only 16
bits. Most of the bits for the tag, so the implementation uses the most
available. More could be better, but that would require a spec change.
 
> Additionally, I do not agree with the statement "we never create such
> long queues anyways". I have already done this myself.

Why? That won't improve bandwidth, and will increase latency. We already
have timeout problems with the current default 1k qdepth on some
devices.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2021-05-17 19:46 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-17 17:59 [PATCH 0/3] nvme: protect against possible request reference after completion Sagi Grimberg
2021-05-17 17:59 ` [PATCH 1/3] nvme-tcp: don't check blk_mq_tag_to_rq when receiving pdu data Sagi Grimberg
2021-05-18  6:59   ` Christoph Hellwig
2021-05-17 17:59 ` [PATCH 2/3] nvme-pci: limit maximum queue depth to 4095 Sagi Grimberg
2021-05-18  7:01   ` Christoph Hellwig
2021-05-17 17:59 ` [PATCH 3/3] nvme: code command_id with a genctr for use-after-free validation Sagi Grimberg
2021-05-17 19:04   ` Keith Busch
2021-05-17 20:23     ` Sagi Grimberg
2021-05-17 19:09   ` Bart Van Assche
2021-05-17 19:46     ` Keith Busch [this message]
2021-05-17 20:27       ` Sagi Grimberg
2021-05-17 20:28       ` Bart Van Assche
2021-05-17 21:50         ` Sagi Grimberg
2021-05-17 22:06           ` Bart Van Assche
2021-05-17 22:15             ` Sagi Grimberg
2021-05-17 18:47 ` [PATCH 0/3] nvme: protect against possible request reference after completion Keith Busch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210517194613.GA2709569@dhcp-10-100-145-180.wdc.com \
    --to=kbusch@kernel.org \
    --cc=bvanassche@acm.org \
    --cc=dwagner@suse.de \
    --cc=hch@lst.de \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.