Re: [PATCH v4] nvme: reject keep-alive passthrough on non-fabrics

[Chao S]

Hi Keith,

Thanks for the feedback, I understand your idea now. Patch v5 will
incorporate the advice.
Thanks again!

Chao

On Fri, May 22, 2026 at 3:32 PM Keith Busch <kbusch@xxxxxxxxxx> wrote:
>
> On Fri, May 22, 2026 at 12:26:39PM -0400, Chao Shi wrote:
> > +/*
> > + * Some Set Features commands change controller behaviour that the driver is
> > + * not prepared to handle on every transport.  Reject such commands from
> > + * userspace passthrough rather than letting them put the controller into a
> > + * state the driver cannot deal with.  The list can be extended as other
> > + * problematic features are identified.
> > + */
> > +static bool nvme_passthru_cmd_allowed(struct nvme_ctrl *ctrl,
> > +                                   struct nvme_ns *ns,
> > +                                   struct nvme_command *c)
> > +{
> > +     /*
> > +      * This only filters admin commands (ns == NULL).  I/O commands share
> > +      * the opcode space with admin commands - Dataset Management is 0x09,
> > +      * the same value as Set Features - so they must not be inspected here.
> > +      */
> > +     if (ns || c->common.opcode != nvme_admin_set_features)
> > +             return true;
> > +
> > +     switch (le32_to_cpu(c->common.cdw10) & 0xff) {
> > +     case NVME_FEAT_KATO:
> > +             /*
> > +              * Keep Alive is optional on PCIe (NVMe 2.0a 5.27.1.12) and the
> > +              * driver only arms keep-alive for fabrics.  Enabling it on
> > +              * other transports starts a keep-alive command the driver is
> > +              * not set up for and harms idle power states, so reject it.
> > +              */
> > +             return ctrl->ops->flags & NVME_F_FABRICS;
> > +     default:
> > +             return true;
> > +     }
> > +}
>
> This doesn't need to be its own function. You can add these checks to
> the existing nvme_cmd_allowed():
>
> ---
> @@ -50,6 +53,18 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
>                         case NVME_ID_CNS_CTRL:
>                                 return true;
>                         }
> +               } else if (c->common.opcode == nvme_admin_set_features) {
> +                       switch (cpu_to_le32(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:
> +                               return false;
> +                       }
>                 }
>                 goto admin;
>         }
> --