[PATCH 0/3] nvme: Support PSK reauthentication (REPLACETLSPSK)

[PATCH 0/3] nvme: Support PSK reauthentication (REPLACETLSPSK)

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

Allow userspace on the host to trigger a reauth (REPLACETLSPSK) from
sysfs. This will replace the PSK for the admin queue when using
a secure concat connection.

This can be done by writing the queue ID to the sysfs file, for example
something like this

```shell
echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
```

`replace_psk` will only appear for concat connections as that is all that
is supported.

This series also include some fixes for the NVMe target code to ensure
this works against a Linux NVMe target.

Alistair Francis (3):
  nvmet-tcp: Don't error if TLS is enabed on a reset
  nvmet-tcp: Don't free SQ on authentication success
  nvme: Allow reauth from sysfs

 drivers/nvme/host/sysfs.c              | 31 ++++++++++++++++++++++++++
 drivers/nvme/target/auth.c             |  4 ++--
 drivers/nvme/target/core.c             |  2 +-
 drivers/nvme/target/fabrics-cmd-auth.c |  6 ++---
 drivers/nvme/target/nvmet.h            |  4 ++--
 5 files changed, 38 insertions(+), 9 deletions(-)

-- 
2.51.0

[PATCH 1/3] nvmet-tcp: Don't error if TLS is enabed on a reset

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

If the host sends a AUTH_Negotiate Message on the admin queue with
REPLACETLSPSK set then we expect and require a TLS connection and
shouldn't report an error if TLS is enabled.

This change only enforces the nvmet_queue_tls_keyid() check if we aren't
resetting the negotiation.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 drivers/nvme/target/auth.c             | 4 ++--
 drivers/nvme/target/core.c             | 2 +-
 drivers/nvme/target/fabrics-cmd-auth.c | 2 +-
 drivers/nvme/target/nvmet.h            | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index f3cf7236e080..3e7024899608 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -140,7 +140,7 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 	return ret;
 }
 
-u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
+u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, bool reset)
 {
 	int ret = 0;
 	struct nvmet_host_link *p;
@@ -166,7 +166,7 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 		goto out_unlock;
 	}
 
-	if (nvmet_queue_tls_keyid(sq)) {
+	if (!reset && nvmet_queue_tls_keyid(sq)) {
 		pr_debug("host %s tls enabled\n", ctrl->hostnqn);
 		goto out_unlock;
 	}
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 5d7d483bfbe3..bd9746715ffc 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1689,7 +1689,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 	if (args->hostid)
 		uuid_copy(&ctrl->hostid, args->hostid);
 
-	dhchap_status = nvmet_setup_auth(ctrl, args->sq);
+	dhchap_status = nvmet_setup_auth(ctrl, args->sq, false);
 	if (dhchap_status) {
 		pr_err("Failed to setup authentication, dhchap status %u\n",
 		       dhchap_status);
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 50639e6e31eb..f71456a94b66 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -293,7 +293,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 			pr_debug("%s: ctrl %d qid %d reset negotiation\n",
 				 __func__, ctrl->cntlid, req->sq->qid);
 			if (!req->sq->qid) {
-				dhchap_status = nvmet_setup_auth(ctrl, req->sq);
+				dhchap_status = nvmet_setup_auth(ctrl, req->sq, true);
 				if (dhchap_status) {
 					pr_err("ctrl %d qid 0 failed to setup re-authentication\n",
 					       ctrl->cntlid);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index f3b09f4099f0..20be2fe43307 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -896,7 +896,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req);
 int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
 		       bool set_ctrl);
 int nvmet_auth_set_host_hash(struct nvmet_host *host, const char *hash);
-u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq);
+u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, bool reset);
 void nvmet_auth_sq_init(struct nvmet_sq *sq);
 void nvmet_destroy_auth(struct nvmet_ctrl *ctrl);
 void nvmet_auth_sq_free(struct nvmet_sq *sq);
@@ -917,7 +917,7 @@ int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
 void nvmet_auth_insert_psk(struct nvmet_sq *sq);
 #else
 static inline u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl,
-				  struct nvmet_sq *sq)
+				  struct nvmet_sq *sq, bool reset)
 {
 	return 0;
 }
-- 
2.51.0

[PATCH 2/3] nvmet-tcp: Don't free SQ on authentication success

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

Curently after the host sends a REPLACETLSPSK we free the TLS keys as
part of calling nvmet_auth_sq_free() on success. This means when the
host sends a follow up REPLACETLSPSK we return CONCAT_MISMATCH as the
check for !nvmet_queue_tls_keyid(req->sq) fails.

This patch ensures we don't free the TLS key on success as we might need
it again in the future.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 drivers/nvme/target/fabrics-cmd-auth.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index f71456a94b66..5cb857d21dfd 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -574,9 +574,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
 	status = nvmet_copy_to_sgl(req, 0, d, al);
 	kfree(d);
 done:
-	if (req->sq->dhchap_step == NVME_AUTH_DHCHAP_MESSAGE_SUCCESS2)
-		nvmet_auth_sq_free(req->sq);
-	else if (req->sq->dhchap_step == NVME_AUTH_DHCHAP_MESSAGE_FAILURE1) {
+	if (req->sq->dhchap_step == NVME_AUTH_DHCHAP_MESSAGE_FAILURE1) {
 		nvmet_auth_sq_free(req->sq);
 		nvmet_ctrl_fatal_error(ctrl);
 	}
-- 
2.51.0

[PATCH 3/3] nvme: Allow reauth from sysfs

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
This can be done by writing the queue ID to te sysfs file.

echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk

Note that only QID 0 (admin queue) is supported.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 drivers/nvme/host/sysfs.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 29430949ce2f..f6994f35324f 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -246,6 +246,32 @@ static ssize_t nuse_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(nuse);
 
+static ssize_t nvme_sysfs_replace_psk(struct device *dev,
+				      struct device_attribute *attr, const char *buf,
+				      size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	int qid, rc;
+
+	rc = kstrtoint(buf, 10, &qid);
+	if (rc)
+		return rc;
+
+	if (qid >= ctrl->queue_count)
+		return -EINVAL;
+
+	rc = nvme_auth_negotiate(ctrl, qid);
+	if (rc < 0)
+		return rc;
+
+	rc = nvme_auth_wait(ctrl, qid);
+	if (rc < 0)
+		return rc;
+
+	return count;
+}
+static DEVICE_ATTR(replace_psk, S_IWUSR, NULL, nvme_sysfs_replace_psk);
+
 static struct attribute *nvme_ns_attrs[] = {
 	&dev_attr_wwid.attr,
 	&dev_attr_uuid.attr,
@@ -747,6 +773,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_dhchap_ctrl_secret.attr,
 #endif
 	&dev_attr_adm_passthru_err_log_enabled.attr,
+	&dev_attr_replace_psk.attr,
 	NULL
 };
 
@@ -776,6 +803,10 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
 	if (a == &dev_attr_dhchap_ctrl_secret.attr && !ctrl->opts)
 		return 0;
 #endif
+	if (a == &dev_attr_replace_psk.attr) {
+		if (!ctrl->opts || !ctrl->opts->concat)
+			return 0;
+	}
 
 	return a->mode;
 }
-- 
2.51.0

Re: [PATCH 1/3] nvmet-tcp: Don't error if TLS is enabed on a reset

[Christoph Hellwig]

On Thu, Oct 30, 2025 at 01:51:12PM +1000, alistair23@gmail.com wrote:
> +				dhchap_status = nvmet_setup_auth(ctrl, req->sq, true);

Please avoid the overly long line.

Re: [PATCH 2/3] nvmet-tcp: Don't free SQ on authentication success

[Christoph Hellwig]

On Thu, Oct 30, 2025 at 01:51:13PM +1000, alistair23@gmail.com wrote:
> Curently after the host sends a REPLACETLSPSK we free the TLS keys as
> part of calling nvmet_auth_sq_free() on success. This means when the
> host sends a follow up REPLACETLSPSK we return CONCAT_MISMATCH as the
> check for !nvmet_queue_tls_keyid(req->sq) fails.
> 
> This patch ensures we don't free the TLS key on success as we might need
> it again in the future.

I initially though we'd now keep it around for the lifetime of the
queue, but I think we'll still free it from nvmet_execute_auth_send,
right?

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Christoph Hellwig]

On Thu, Oct 30, 2025 at 01:51:14PM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
> This can be done by writing the queue ID to te sysfs file.
> 
> echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
> 
> Note that only QID 0 (admin queue) is supported.

Why pass the queue ID then instead of a boolean value?

> +static ssize_t nvme_sysfs_replace_psk(struct device *dev,
> +				      struct device_attribute *attr, const char *buf,
> +				      size_t count)

Overly long line.  And very inefficient annoyoing to modify indentation
compared to:

static ssize_t nvme_sysfs_replace_psk(struct device *dev,
		struct device_attribute *attr, const char *buf, size_t count)

> +	rc = kstrtoint(buf, 10, &qid);
> +	if (rc)
> +		return rc;

Nitpick, but nvme style is to use the slightly more descriptive
error and not "rc" which doesn't mean much in general.

Re: [PATCH 2/3] nvmet-tcp: Don't free SQ on authentication success

[Alistair Francis]

On Sat, Nov 1, 2025 at 12:03 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Oct 30, 2025 at 01:51:13PM +1000, alistair23@gmail.com wrote:
> > Curently after the host sends a REPLACETLSPSK we free the TLS keys as
> > part of calling nvmet_auth_sq_free() on success. This means when the
> > host sends a follow up REPLACETLSPSK we return CONCAT_MISMATCH as the
> > check for !nvmet_queue_tls_keyid(req->sq) fails.
> >
> > This patch ensures we don't free the TLS key on success as we might need
> > it again in the future.
>
> I initially though we'd now keep it around for the lifetime of the
> queue, but I think we'll still free it from nvmet_execute_auth_send,
> right?

Good point. In theory nvmet_execute_auth_send() can free it, I don't
see that actually happening as `req->sq->dhchap_step` is always
`NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE` for me.

I'll add another patch to remove the call to nvmet_auth_sq_free() on success.

Alistair

>

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Alistair Francis]

On Sat, Nov 1, 2025 at 12:05 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Oct 30, 2025 at 01:51:14PM +1000, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
> > This can be done by writing the queue ID to te sysfs file.
> >
> > echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
> >
> > Note that only QID 0 (admin queue) is supported.
>
> Why pass the queue ID then instead of a boolean value?

I liked the explicitness of passing a queue ID instead of a bool and
it allows supporting more queues in the future if that changes in the
spec.

I can change it to a bool instead if that's preferred?

Alistair

>
> > +static ssize_t nvme_sysfs_replace_psk(struct device *dev,
> > +                                   struct device_attribute *attr, const char *buf,
> > +                                   size_t count)
>
> Overly long line.  And very inefficient annoyoing to modify indentation
> compared to:
>
> static ssize_t nvme_sysfs_replace_psk(struct device *dev,
>                 struct device_attribute *attr, const char *buf, size_t count)
>
> > +     rc = kstrtoint(buf, 10, &qid);
> > +     if (rc)
> > +             return rc;
>
> Nitpick, but nvme style is to use the slightly more descriptive
> error and not "rc" which doesn't mean much in general.
>

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Chaitanya Kulkarni]

On 11/2/25 5:47 PM, Alistair Francis wrote:
> On Sat, Nov 1, 2025 at 12:05 AM Christoph Hellwig<hch@lst.de> wrote:
>> On Thu, Oct 30, 2025 at 01:51:14PM +1000,alistair23@gmail.com wrote:
>>> From: Alistair Francis<alistair.francis@wdc.com>
>>>
>>> Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
>>> This can be done by writing the queue ID to te sysfs file.
>>>
>>> echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
>>>
>>> Note that only QID 0 (admin queue) is supported.
>> Why pass the queue ID then instead of a boolean value?
> I liked the explicitness of passing a queue ID instead of a bool and
> it allows supporting more queues in the future if that changes in the
> spec.
>
> I can change it to a bool instead if that's preferred?
>
> Alistair

do you have any plans to add support for the I/O queues in future ?
OR
have a strong usecase for I/O queues to support this feature ?

-ck

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Alistair Francis]

On Mon, Nov 3, 2025 at 12:05 PM Chaitanya Kulkarni
<chaitanyak@nvidia.com> wrote:
>
> On 11/2/25 5:47 PM, Alistair Francis wrote:
> > On Sat, Nov 1, 2025 at 12:05 AM Christoph Hellwig<hch@lst.de> wrote:
> >> On Thu, Oct 30, 2025 at 01:51:14PM +1000,alistair23@gmail.com wrote:
> >>> From: Alistair Francis<alistair.francis@wdc.com>
> >>>
> >>> Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
> >>> This can be done by writing the queue ID to te sysfs file.
> >>>
> >>> echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
> >>>
> >>> Note that only QID 0 (admin queue) is supported.
> >> Why pass the queue ID then instead of a boolean value?
> > I liked the explicitness of passing a queue ID instead of a bool and
> > it allows supporting more queues in the future if that changes in the
> > spec.
> >
> > I can change it to a bool instead if that's preferred?
> >
> > Alistair
>
> do you have any plans to add support for the I/O queues in future ?

No, it would require a spec change

> OR
> have a strong usecase for I/O queues to support this feature ?

I do not, but it does seem like something that maybe should be supported

Alistair

>
> -ck
>

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Chaitanya Kulkarni]

On 11/2/25 6:24 PM, Alistair Francis wrote:
> On Mon, Nov 3, 2025 at 12:05 PM Chaitanya Kulkarni
> <chaitanyak@nvidia.com> wrote:
>> On 11/2/25 5:47 PM, Alistair Francis wrote:
>>> On Sat, Nov 1, 2025 at 12:05 AM Christoph Hellwig<hch@lst.de> wrote:
>>>> On Thu, Oct 30, 2025 at 01:51:14PM +1000,alistair23@gmail.com wrote:
>>>>> From: Alistair Francis<alistair.francis@wdc.com>
>>>>>
>>>>> Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
>>>>> This can be done by writing the queue ID to te sysfs file.
>>>>>
>>>>> echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
>>>>>
>>>>> Note that only QID 0 (admin queue) is supported.
>>>> Why pass the queue ID then instead of a boolean value?
>>> I liked the explicitness of passing a queue ID instead of a bool and
>>> it allows supporting more queues in the future if that changes in the
>>> spec.
>>>
>>> I can change it to a bool instead if that's preferred?
>>>
>>> Alistair
>> do you have any plans to add support for the I/O queues in future ?
> No, it would require a spec change


okay so it's going to be significant change of functionality via spec.


>> OR
>> have a strong usecase for I/O queues to support this feature ?
> I do not, but it does seem like something that maybe should be supported


it does sound useful and it's a spac change so how about we add separate 
I/O queues interface with spec change and keep this purely admin queue ?

> Alistair
>
>> -ck
>>

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Christoph Hellwig]

On Mon, Nov 03, 2025 at 11:47:23AM +1000, Alistair Francis wrote:
> On Sat, Nov 1, 2025 at 12:05 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Thu, Oct 30, 2025 at 01:51:14PM +1000, alistair23@gmail.com wrote:
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
> > > This can be done by writing the queue ID to te sysfs file.
> > >
> > > echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
> > >
> > > Note that only QID 0 (admin queue) is supported.
> >
> > Why pass the queue ID then instead of a boolean value?
> 
> I liked the explicitness of passing a queue ID instead of a bool and
> it allows supporting more queues in the future if that changes in the
> spec.
> 
> I can change it to a bool instead if that's preferred?

I find an "echo 0" for a simple one-shot sysfs file rather confusing.
Given that we're not likely to grow anything else I'd vote against
doing it, but this is no hard NAK if there is consensus to do it
this way by others.

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Hannes Reinecke]

On 10/30/25 04:51, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
> This can be done by writing the queue ID to te sysfs file.
> 
> echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
> 
> Note that only QID 0 (admin queue) is supported.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   drivers/nvme/host/sysfs.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
Please, don't. Currently we are using the same key for all queues,
and we really should keep it that way as we don't have a way of
specifying the key based on the queue ID (the TLS identification
is identical for all queues).
So we really need to trigger a replacepsk operation for all queues.

I would suggest just allow writes to the 'tls_key' attribute; any
writes to that would trigger a replacepsk operation.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Alistair Francis]

On Mon, Nov 3, 2025 at 10:08 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/30/25 04:51, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > Allow userspace to trigger a reauth (REPLACETLSPSK) from sysfs.
> > This can be done by writing the queue ID to te sysfs file.
> >
> > echo 0 > /sys/devices/virtual/nvme-fabrics/ctl/nvme0/replace_psk
> >
> > Note that only QID 0 (admin queue) is supported.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   drivers/nvme/host/sysfs.c | 31 +++++++++++++++++++++++++++++++
> >   1 file changed, 31 insertions(+)
> >
> Please, don't. Currently we are using the same key for all queues,
> and we really should keep it that way as we don't have a way of
> specifying the key based on the queue ID (the TLS identification
> is identical for all queues).
> So we really need to trigger a replacepsk operation for all queues.
>
> I would suggest just allow writes to the 'tls_key' attribute; any
> writes to that would trigger a replacepsk operation.

I think the `tls_configured_key` is actually the better attribute to
write to as that is the one that updates after a REPLACETLSPSK
operation, see v2 patches which I'm sending now.

Alistair

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Christoph Hellwig]

On Wed, Nov 12, 2025 at 09:32:00AM +1000, Alistair Francis wrote:
> > I would suggest just allow writes to the 'tls_key' attribute; any
> > writes to that would trigger a replacepsk operation.
> 
> I think the `tls_configured_key` is actually the better attribute to
> write to as that is the one that updates after a REPLACETLSPSK
> operation, see v2 patches which I'm sending now.

Just saw Hannes reply here and saw why you did the current version
the way I did.  Hannes, please don't recommend weird ABIs that
make error checking and future extensibility impossible.

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Hannes Reinecke]

On 11/12/25 08:02, Christoph Hellwig wrote:
> On Wed, Nov 12, 2025 at 09:32:00AM +1000, Alistair Francis wrote:
>>> I would suggest just allow writes to the 'tls_key' attribute; any
>>> writes to that would trigger a replacepsk operation.
>>
>> I think the `tls_configured_key` is actually the better attribute to
>> write to as that is the one that updates after a REPLACETLSPSK
>> operation, see v2 patches which I'm sending now.
> 
> Just saw Hannes reply here and saw why you did the current version
> the way I did.  Hannes, please don't recommend weird ABIs that
> make error checking and future extensibility impossible.
> 
Hmm.

'tls_configured_key' prints out the value of
ctrl->opts->tls_key, ie the key passed in from the 'connect'
string. Normally this value will be empty,
as the 'connect' command will pick up the TLS key from
the keyring automatically.

'tls_key' prints out the value of
ctrl->tls_pskid, ie the value of the _negotiated_ key.

So why is 'tls_configured_key' key the better option?
Personally I think that 'tls_key' is more 'natural',
as we want to replace the negotiated key, not the
configured key ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Christoph Hellwig]

On Wed, Nov 12, 2025 at 08:21:41AM +0100, Hannes Reinecke wrote:
>
> 'tls_configured_key' prints out the value of
> ctrl->opts->tls_key, ie the key passed in from the 'connect'
> string. Normally this value will be empty,
> as the 'connect' command will pick up the TLS key from
> the keyring automatically.
>
> 'tls_key' prints out the value of
> ctrl->tls_pskid, ie the value of the _negotiated_ key.
>
> So why is 'tls_configured_key' key the better option?
> Personally I think that 'tls_key' is more 'natural',
> as we want to replace the negotiated key, not the
> configured key ...

I've not even looked into what is better, but accepting anything
without validity checking tends to always bite us later.

Re: [PATCH 3/3] nvme: Allow reauth from sysfs

[Alistair Francis]

On Wed, Nov 12, 2025 at 5:21 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 11/12/25 08:02, Christoph Hellwig wrote:
> > On Wed, Nov 12, 2025 at 09:32:00AM +1000, Alistair Francis wrote:
> >>> I would suggest just allow writes to the 'tls_key' attribute; any
> >>> writes to that would trigger a replacepsk operation.
> >>
> >> I think the `tls_configured_key` is actually the better attribute to
> >> write to as that is the one that updates after a REPLACETLSPSK
> >> operation, see v2 patches which I'm sending now.
> >
> > Just saw Hannes reply here and saw why you did the current version
> > the way I did.  Hannes, please don't recommend weird ABIs that
> > make error checking and future extensibility impossible.
> >
> Hmm.
>
> 'tls_configured_key' prints out the value of
> ctrl->opts->tls_key, ie the key passed in from the 'connect'
> string. Normally this value will be empty,
> as the 'connect' command will pick up the TLS key from
> the keyring automatically.

`tls_configured_key` does print out the value of
`ctrl->opts->tls_key`. That's the key that is generated in
`nvme_auth_secure_concat()`.

`ctrl->opts->tls_key` is also the key that changes after a
REPLACETLSPSK operation. `ctrl->opts->tls_key` is what's used in
nvme_auth_set_dhchap_negotiate_data() to determine if we should issue
a NEWTLSPSK or a REPLACETLSPSK.

So `ctrl->opts->tls_key` (and hence `tls_configured_key`) seems like
the way to go

>
> 'tls_key' prints out the value of
> ctrl->tls_pskid, ie the value of the _negotiated_ key.

Is it possible you have `ctrl->tls_pskid` and `ctrl->opts->tls_key` mixed up?

`ctrl->tls_pskid` is set by userspace via the nvme_tcp_tls_done()
callback. It's really more of the "configured" key as it's supplied by
userspace and doesn't change after a REPLACETLSPSK operation.

I'm not sure why the sysfs names are what they are, but
`tls_configured_key` seems like the negotiated key and `tls_key` is
configured by userspace.

>
> So why is 'tls_configured_key' key the better option?

`tls_configured_key` is the key that changes after a REPLACETLSPSK, so
it feels like that's the better place to trigger a REPLACETLSPSK.

> Personally I think that 'tls_key' is more 'natural',
> as we want to replace the negotiated key, not the
> configured key ...

I really think writing to the `tls_configured_key` to trigger a
REPLACETLSPSK is the way to go. It's the sysfs entry that changes
after a REPLACETLSPSK command. Writing to 'tls_key' and having it not
update just seems confusing.

Alistair

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich