Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Chao S]

Sorry for the delay - I was analyzing crashes and preparing patches, and
had a paper deadline. I'll respond promptly from now on.

> I suspect we're best off splitting out the admin and ns-command set
> specific parts of nvme_cmd_allowed into separate helpers.  And maybe use
> a switch statement on the command as nested ifs become cumbersome in the
> long run.

Agreed. v6:

- Split nvme_cmd_allowed() into nvme_admin_cmd_allowed(ctrl, c) and
  nvme_ns_cmd_allowed(ns, c, open_for_write), leaving nvme_cmd_allowed() a
  thin dispatcher, and switch on the command in the admin helper. Semantics
  unchanged.
- That fixes the long comment (it moves into the admin helper at a
  shallower indent). The nvme_uring_cmd_io() line is a caller, not the
  switch, so I'll hoist a local open_for_write there.
- ctrl stays: on the admin path ns is NULL (Keith's point), so it can't
  come from ns. Tokunori, a flags bit would encode the transport, but flags
  is about how the ioctl was invoked (vector / partition), so I'd rather
  keep ctrl - it lives in the admin helper, and the I/O helper uses ns.
- Keeping NVME_FEAT_AUTO_PST rejected - agreed there's no real reason to
  allow it.

I'll send v6 shortly.

Thanks,
Chao

On Thu, May 28, 2026 at 4:43 AM Christoph Hellwig <hch@xxxxxx> wrote:
>
> On Wed, May 27, 2026 at 08:32:08AM -0600, Keith Busch wrote:
> > On Sat, May 23, 2026 at 06:56:29PM -0400, Chao Shi wrote:
> > > +                   switch (le32_to_cpu(c->features.fid) & 0xff) {
> > > +                   case NVME_FEAT_KATO:
> > > +                           if (ctrl->ops->flags & NVME_F_FABRICS)
> > > +                                   break;
> > > +                           fallthrough;
> > > +                   case NVME_FEAT_HOST_BEHAVIOR:
> > > +                   case NVME_FEAT_HOST_MEM_BUF:
> > > +                   case NVME_FEAT_NUM_QUEUES:
> > > +                   case NVME_FEAT_AUTO_PST:
> >
> > I may have been overly restrictive with suggesting AUTO_PST for this
> > filter. Messing with the other features will break something, but power
> > state is just user policy. The driver may undo the user setting on a
> > controller reset, but so what?
>
> There's really no point in allowing it.  Just as we should have never
> allowed low-level config of any kind, but unfortunately nvme admin
> commands are a horrible grab all.