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.0On 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
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
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.
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
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
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.
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.
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. >
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.
© 2016 - 2025 Red Hat, Inc.