by Hannes Reinecke

[permalink] [raw]

Subject: Re: [PATCH 07/12] nvme: Implement In-Band authentication

On 11/23/21 2:11 PM, Sagi Grimberg wrote:
>
>> +int nvme_auth_generate_key(struct nvme_ctrl *ctrl, u8 *secret, bool
>> set_ctrl)
>
> Didn't we agree to pass the key pointer? i.e.
> int nvme_auth_generate_key(struct nvme_dhchap_key **key, u8 *secret)
>
Ah. That's what you had in mind.
Why, of course we can do that.

>> +{
>> +    struct nvme_dhchap_key *key;
>> +    u8 key_hash;
>> +
>> +    if (!secret)
>> +        return 0;
>> +
>> +    if (sscanf(secret, "DHHC-1:%hhd:%*s:", &key_hash) != 1)
>> +        return -EINVAL;
>> +
>> +    /* Pass in the secret without the 'DHHC-1:XX:' prefix */
>> +    key = nvme_auth_extract_key(secret + 10, key_hash);
>> +    if (IS_ERR(key)) {
>> +        dev_dbg(ctrl->device, "failed to extract key, error %ld\n",
>> +            PTR_ERR(key));
>
> The print here is slightly redundant - you already have prints inside
> nvme_auth_extract_key already.
>
Yeah; I really need to go through the code and remove the redundant
messages. Especially on the error paths.

>> +        return PTR_ERR(key);
>> +    }
>> +
>
> Then we instead just do:
>     *key = key;
>
>> +    if (set_ctrl)
>> +        ctrl->ctrl_key = key;
>> +    else
>> +        ctrl->host_key = key;
>> +
>> +    return 0;
>> +}
>
> ...
>
>> +EXPORT_SYMBOL_GPL(nvme_auth_generate_key);
>> diff --git a/drivers/nvme/host/auth.h b/drivers/nvme/host/auth.h
>> new file mode 100644
>> index 000000000000..16e3d893d54a
>> --- /dev/null
>> +++ b/drivers/nvme/host/auth.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2021 Hannes Reinecke, SUSE Software Solutions
>> + */
>> +
>> +#ifndef _NVME_AUTH_H
>> +#define _NVME_AUTH_H
>> +
>> +#include <crypto/kpp.h>
>> +
>> +struct nvme_dhchap_key {
>> +    u8 *key;
>> +    size_t key_len;
>> +    u8 key_hash;
>
> Why not just name it len and hash? don't think the key_
> prefix is useful...

True.
Will do so.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

2021-11-26 07:54:12

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH 09/12] nvmet: Parse fabrics commands on all queues

On Tue, Nov 23, 2021 at 01:37:58PM +0100, Hannes Reinecke wrote:
> Fabrics commands might be sent to all queues, not just the admin one.
>
> Signed-off-by: Hannes Reinecke <[email protected]>
> Reviewed-by: Sagi Grimberg <[email protected]>
> Reviewed-by: Himanshu Madhani <[email protected]>
> ---
> drivers/nvme/target/core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 5119c687de68..a3abbf50f7e0 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -943,6 +943,8 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
> if (unlikely(!req->sq->ctrl))
> /* will return an error for any non-connect command: */
> status = nvmet_parse_connect_cmd(req);
> + else if (nvme_is_fabrics(req->cmd))
> + status = nvmet_parse_fabrics_cmd(req);

This will allow all fabrics commands on the I/O queue, which is a bad
idea. Please please nvmet_parse_fabrics_cmd into
nvmet_parse_admin_fabrics_cmd and nvmet_parse_io_fabrics_cmd