[RFC PATCH] nvmet: passthru: use Invalid Field, not Invalid Opcode, for unhandled Get/Set Features FIDs

[RFC PATCH] nvmet: passthru: use Invalid Field, not Invalid Opcode, for unhandled Get/Set Features FIDs

[Marko Ahvenainen]

Hello all,

My first kernel patch, be gentle ;)

nvmet_passthru_get_set_features() rejects any FID that reaches its
default case - whether intentionally blocked (IRQ_COALESCE,
IRQ_CONFIG, HOST_MEM_BUF, SW_PROGRESS, RESV_MASK, RESV_PERSIST) or
simply unlisted (e.g. FID 0, unassigned by the spec) - with
NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR. That's the wrong class: the
Get/Set Features opcode is always valid here, it's the FID that's
rejected, so NVME_SC_INVALID_FIELD is correct.

This surfaced while debugging why nvme-cli's "dump all known
features" mode (get-feature -f 0) completes against a native
controller but aborts over nvmet passthru: natively, FID 0 gets a
plain Invalid Field rejection (no DNR) that nvme-cli tolerates and
continues past; nvmet's Invalid Opcode + DNR is not tolerated the
same way.

Verified with this fix applied: get-feature -f 0 now enumerates every
FID over nvmet passthru (NVMe/TCP) without aborting early. The DNR
flag was not the obstacle - only the status class was.

This is a status-code fix only, not a policy change: Get/Set Features
FIDs are still intercepted at more than one layer before reaching
this function (nvmet_parse_admin_cmd()/nvmet_parse_passthru_admin_cmd()
already service ASYNC_EVENT, KATO, NUM_QUEUES, HOST_ID and FDP with
nvmet's own emulation), so native and passthru queries still won't
return identical results for every FID - only this early-abort
failure mode is fixed. I'd rather leave any broader forwarding-policy
change to a separate discussion.

Unrelated aside noticed while reading this code: the
NVME_FEAT_ASYNC_EVENT case (and propably some other) inside this
function is dead code, since the callers above already intercept
that FID before it can be reached.

Signed-off-by: Marko Ahvenainen <marko.ahvenainen@helsinkimemorytech.com>
---
 drivers/nvme/target/passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index e27f84e3cf2b..df1396081f87 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -483,7 +483,7 @@ static u16 nvmet_passthru_get_set_features(struct nvmet_req *req)
        case NVME_FEAT_RESV_PERSIST:
                /* No reservations, see nvmet_parse_passthru_io_cmd() */
        default:
-               return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
+               return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
        }
 }

--
2.43.0

This e-mail and any attachments may contain privileged or confidential information and is for the sole use of the intended recipient(s). Any unauthorized use or disclosure of this communication is prohibited. If you believe that you have received this e-mail in error, please notify the sender immediately and destroy all copies.

Re: [RFC PATCH] nvmet: passthru: use Invalid Field, not Invalid Opcode, for unhandled Get/Set Features FIDs

[Sagi Grimberg]

Hey Marko,

> Hello all,
>
> My first kernel patch, be gentle ;)
>
> nvmet_passthru_get_set_features() rejects any FID that reaches its
> default case - whether intentionally blocked (IRQ_COALESCE,
> IRQ_CONFIG, HOST_MEM_BUF, SW_PROGRESS, RESV_MASK, RESV_PERSIST) or
> simply unlisted (e.g. FID 0, unassigned by the spec) - with
> NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR. That's the wrong class: the
> Get/Set Features opcode is always valid here, it's the FID that's
> rejected, so NVME_SC_INVALID_FIELD is correct.
>
> This surfaced while debugging why nvme-cli's "dump all known
> features" mode (get-feature -f 0) completes against a native
> controller but aborts over nvmet passthru: natively, FID 0 gets a
> plain Invalid Field rejection (no DNR) that nvme-cli tolerates and
> continues past; nvmet's Invalid Opcode + DNR is not tolerated the
> same way.
>
> Verified with this fix applied: get-feature -f 0 now enumerates every
> FID over nvmet passthru (NVMe/TCP) without aborting early. The DNR
> flag was not the obstacle - only the status class was.
>
> This is a status-code fix only, not a policy change: Get/Set Features
> FIDs are still intercepted at more than one layer before reaching
> this function (nvmet_parse_admin_cmd()/nvmet_parse_passthru_admin_cmd()
> already service ASYNC_EVENT, KATO, NUM_QUEUES, HOST_ID and FDP with
> nvmet's own emulation), so native and passthru queries still won't
> return identical results for every FID - only this early-abort
> failure mode is fixed. I'd rather leave any broader forwarding-policy
> change to a separate discussion.
>
> Unrelated aside noticed while reading this code: the
> NVME_FEAT_ASYNC_EVENT case (and propably some other) inside this
> function is dead code, since the callers above already intercept
> that FID before it can be reached.
>
> Signed-off-by: Marko Ahvenainen <marko.ahvenainen@helsinkimemorytech.com>
> ---
>   drivers/nvme/target/passthru.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index e27f84e3cf2b..df1396081f87 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -483,7 +483,7 @@ static u16 nvmet_passthru_get_set_features(struct nvmet_req *req)
>          case NVME_FEAT_RESV_PERSIST:
>                  /* No reservations, see nvmet_parse_passthru_io_cmd() */
>          default:
> -               return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
> +               return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;

I think you want to set the error_loc such that it can be viewed from 
the error log page.

>          }
>   }
>
> --
> 2.43.0

You should make it a proper (non-rfc) patch and send it.
The below should be removed from the actual submission.

>
> This e-mail and any attachments may contain privileged or confidential information and is for the sole use of the intended recipient(s). Any unauthorized use or disclosure of this communication is prohibited. If you believe that you have received this e-mail in error, please notify the sender immediately and destroy all copies.

RE: [RFC PATCH] nvmet: passthru: use Invalid Field, not Invalid Opcode, for unhandled Get/Set Features FIDs

[Marko Ahvenainen]

Hello,

> Hey Marko,
>
> > Hello all,
> >
> > My first kernel patch, be gentle ;)
> >
> > nvmet_passthru_get_set_features() rejects any FID that reaches its
> > default case - whether intentionally blocked (IRQ_COALESCE,
> > IRQ_CONFIG, HOST_MEM_BUF, SW_PROGRESS, RESV_MASK, RESV_PERSIST)
> or
> > simply unlisted (e.g. FID 0, unassigned by the spec) - with
> > NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR. That's the wrong class:
> the
> > Get/Set Features opcode is always valid here, it's the FID that's
> > rejected, so NVME_SC_INVALID_FIELD is correct.
> >
> > This surfaced while debugging why nvme-cli's "dump all known features"
> > mode (get-feature -f 0) completes against a native controller but
> > aborts over nvmet passthru: natively, FID 0 gets a plain Invalid Field
> > rejection (no DNR) that nvme-cli tolerates and continues past; nvmet's
> > Invalid Opcode + DNR is not tolerated the same way.
> >
> > Verified with this fix applied: get-feature -f 0 now enumerates every
> > FID over nvmet passthru (NVMe/TCP) without aborting early. The DNR
> > flag was not the obstacle - only the status class was.
> >
> > This is a status-code fix only, not a policy change: Get/Set Features
> > FIDs are still intercepted at more than one layer before reaching this
> > function (nvmet_parse_admin_cmd()/nvmet_parse_passthru_admin_cmd()
> > already service ASYNC_EVENT, KATO, NUM_QUEUES, HOST_ID and FDP with
> > nvmet's own emulation), so native and passthru queries still won't
> > return identical results for every FID - only this early-abort failure
> > mode is fixed. I'd rather leave any broader forwarding-policy change
> > to a separate discussion.
> >
> > Unrelated aside noticed while reading this code: the
> > NVME_FEAT_ASYNC_EVENT case (and propably some other) inside this
> > function is dead code, since the callers above already intercept that
> > FID before it can be reached.
> >
> > Signed-off-by: Marko Ahvenainen
> > <marko.ahvenainen@helsinkimemorytech.com>
> > ---
> >   drivers/nvme/target/passthru.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/target/passthru.c
> > b/drivers/nvme/target/passthru.c index e27f84e3cf2b..df1396081f87
> > 100644
> > --- a/drivers/nvme/target/passthru.c
> > +++ b/drivers/nvme/target/passthru.c
> > @@ -483,7 +483,7 @@ static u16 nvmet_passthru_get_set_features(struct
> nvmet_req *req)
> >          case NVME_FEAT_RESV_PERSIST:
> >                  /* No reservations, see nvmet_parse_passthru_io_cmd() */
> >          default:
> > -               return NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
> > +               return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>
> I think you want to set the error_loc such that it can be viewed from the error
> log page.
>
> >          }
> >   }
> >
> > --
> > 2.43.0
>
> You should make it a proper (non-rfc) patch and send it.
> The below should be removed from the actual submission.
>

Thanks for the feedback. I sent this as an RFC precisely because I noticed multiple inconsistencies and potential dead code in this routine, but I don't feel confident enough with the driver internals to redesign it properly.

My patch was just a quick fix for the obviously wrong return status that caught my eye. If someone with deeper knowledge of this subsystem could take a look at the surrounding code and clean up the broader structure (including error_loc and the dead paths), that would be great. If not, then this fix will still work and it's inline with the current implementation, which doesn't seem to have error_loc set for other locations that return the same return status.

Best regards,
Marko
This e-mail and any attachments may contain privileged or confidential information and is for the sole use of the intended recipient(s). Any unauthorized use or disclosure of this communication is prohibited. If you believe that you have received this e-mail in error, please notify the sender immediately and destroy all copies.