support keyrings for NFS TLS mounts v2

support keyrings for NFS TLS mounts v2

[Christoph Hellwig]

Hi all,

this series allows storing the key and certificate for NFS over
TLS mounts in the keyring and be specified using a mount option.
This way they don't need to be hardcoded in the global tlshd.conf
configuration file and can even be different per-mount.

Note that for now the .nfs keyring still needs to be added to
tlshd.conf, but that should go away with the handshake enhacement
from Hannes.

Changes since v1:
 - don't depend on nfsv4 for the keyring
 - fix compile when the kernel keyring is disabled

[PATCH 1/2] NFS: support the kernel keyring for TLS

[Christoph Hellwig]

Allow tlshd to use a per-mount key from the kernel keyring similar
to NVMe over TCP.

Note that tlshd expects keys and certificates stored in the kernel
keyring to be in DER format, not the PEM format used for file based keys
and certificates, so they need to be converted before they are added
to the keyring, which is a bit unexpected.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/fs_context.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 13f71ca8c974..9e94d18448ff 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -96,6 +96,8 @@ enum nfs_param {
 	Opt_wsize,
 	Opt_write,
 	Opt_xprtsec,
+	Opt_cert_serial,
+	Opt_privkey_serial,
 };
 
 enum {
@@ -221,6 +223,8 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
 	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
 	fsparam_u32   ("wsize",		Opt_wsize),
 	fsparam_string("xprtsec",	Opt_xprtsec),
+	fsparam_s32("cert_serial",	Opt_cert_serial),
+	fsparam_s32("privkey_serial",	Opt_privkey_serial),
 	{}
 };
 
@@ -551,6 +555,32 @@ static int nfs_parse_version_string(struct fs_context *fc,
 	return 0;
 }
 
+#ifdef CONFIG_KEYS
+static int nfs_tls_key_verify(key_serial_t key_id)
+{
+	struct key *key = key_lookup(key_id);
+	int error = 0;
+
+	if (IS_ERR(key)) {
+		pr_err("key id %08x not found\n", key_id);
+		return PTR_ERR(key);
+	}
+	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
+	    test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
+		pr_err("key id %08x revoked\n", key_id);
+		error = -EKEYREVOKED;
+	}
+
+	key_put(key);
+	return error;
+}
+#else
+static inline int nfs_tls_key_verify(key_serial_t key_id)
+{
+	return -ENOENT;
+}
+#endif /* CONFIG_KEYS */
+
 /*
  * Parse a single mount parameter.
  */
@@ -807,6 +837,18 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		if (ret < 0)
 			return ret;
 		break;
+	case Opt_cert_serial:
+		ret = nfs_tls_key_verify(result.int_32);
+		if (ret < 0)
+			return ret;
+		ctx->xprtsec.cert_serial = result.int_32;
+		break;
+	case Opt_privkey_serial:
+		ret = nfs_tls_key_verify(result.int_32);
+		if (ret < 0)
+			return ret;
+		ctx->xprtsec.privkey_serial = result.int_32;
+		break;
 
 	case Opt_proto:
 		if (!param->string)
-- 
2.47.2

Re: [PATCH 1/2] NFS: support the kernel keyring for TLS

[Jarkko Sakkinen]

On Thu, May 15, 2025 at 01:50:55PM +0200, Christoph Hellwig wrote:
> Allow tlshd to use a per-mount key from the kernel keyring similar
> to NVMe over TCP.
> 
> Note that tlshd expects keys and certificates stored in the kernel
> keyring to be in DER format, not the PEM format used for file based keys
> and certificates, so they need to be converted before they are added
> to the keyring, which is a bit unexpected.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/fs_context.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 13f71ca8c974..9e94d18448ff 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -96,6 +96,8 @@ enum nfs_param {
>  	Opt_wsize,
>  	Opt_write,
>  	Opt_xprtsec,
> +	Opt_cert_serial,
> +	Opt_privkey_serial,
>  };
>  
>  enum {
> @@ -221,6 +223,8 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
>  	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
>  	fsparam_u32   ("wsize",		Opt_wsize),
>  	fsparam_string("xprtsec",	Opt_xprtsec),
> +	fsparam_s32("cert_serial",	Opt_cert_serial),
> +	fsparam_s32("privkey_serial",	Opt_privkey_serial),
>  	{}
>  };
>  
> @@ -551,6 +555,32 @@ static int nfs_parse_version_string(struct fs_context *fc,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_KEYS
> +static int nfs_tls_key_verify(key_serial_t key_id)
> +{
> +	struct key *key = key_lookup(key_id);
> +	int error = 0;
> +
> +	if (IS_ERR(key)) {
> +		pr_err("key id %08x not found\n", key_id);
> +		return PTR_ERR(key);
> +	}
> +	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
> +	    test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
> +		pr_err("key id %08x revoked\n", key_id);
> +		error = -EKEYREVOKED;
> +	}
> +
> +	key_put(key);
> +	return error;
> +}

This is equivalent nvme_tls_key_lookup() so would it be more senseful
to call it nfs_tls_key_lookup()? I'm also a bit puzzled how the code
will associate nfs_keyring to all this (e.g., with keyring_search as
done in nvme_tls_psk_lookup())?

> +#else
> +static inline int nfs_tls_key_verify(key_serial_t key_id)
> +{
> +	return -ENOENT;
> +}
> +#endif /* CONFIG_KEYS */
> +
>  /*
>   * Parse a single mount parameter.
>   */
> @@ -807,6 +837,18 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		if (ret < 0)
>  			return ret;
>  		break;
> +	case Opt_cert_serial:
> +		ret = nfs_tls_key_verify(result.int_32);
> +		if (ret < 0)
> +			return ret;
> +		ctx->xprtsec.cert_serial = result.int_32;
> +		break;
> +	case Opt_privkey_serial:
> +		ret = nfs_tls_key_verify(result.int_32);
> +		if (ret < 0)
> +			return ret;
> +		ctx->xprtsec.privkey_serial = result.int_32;
> +		break;
>  
>  	case Opt_proto:
>  		if (!param->string)
> -- 
> 2.47.2
> 

I get the change i.e., keep keys opaque, and it is a reasonable goal.
However, the keyring-key association is where I get lost, so if you
could help me out with that a bit, we could make progress :-)

BR, Jarkko

Re: [PATCH 1/2] NFS: support the kernel keyring for TLS

[Hannes Reinecke]

On 5/15/25 14:51, Jarkko Sakkinen wrote:
> On Thu, May 15, 2025 at 01:50:55PM +0200, Christoph Hellwig wrote:
>> Allow tlshd to use a per-mount key from the kernel keyring similar
>> to NVMe over TCP.
>>
>> Note that tlshd expects keys and certificates stored in the kernel
>> keyring to be in DER format, not the PEM format used for file based keys
>> and certificates, so they need to be converted before they are added
>> to the keyring, which is a bit unexpected.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   fs/nfs/fs_context.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>> index 13f71ca8c974..9e94d18448ff 100644
>> --- a/fs/nfs/fs_context.c
>> +++ b/fs/nfs/fs_context.c
>> @@ -96,6 +96,8 @@ enum nfs_param {
>>   	Opt_wsize,
>>   	Opt_write,
>>   	Opt_xprtsec,
>> +	Opt_cert_serial,
>> +	Opt_privkey_serial,
>>   };
>>   
>>   enum {
>> @@ -221,6 +223,8 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
>>   	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
>>   	fsparam_u32   ("wsize",		Opt_wsize),
>>   	fsparam_string("xprtsec",	Opt_xprtsec),
>> +	fsparam_s32("cert_serial",	Opt_cert_serial),
>> +	fsparam_s32("privkey_serial",	Opt_privkey_serial),
>>   	{}
>>   };
>>   
>> @@ -551,6 +555,32 @@ static int nfs_parse_version_string(struct fs_context *fc,
>>   	return 0;
>>   }
>>   
>> +#ifdef CONFIG_KEYS
>> +static int nfs_tls_key_verify(key_serial_t key_id)
>> +{
>> +	struct key *key = key_lookup(key_id);
>> +	int error = 0;
>> +
>> +	if (IS_ERR(key)) {
>> +		pr_err("key id %08x not found\n", key_id);
>> +		return PTR_ERR(key);
>> +	}
>> +	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
>> +	    test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
>> +		pr_err("key id %08x revoked\n", key_id);
>> +		error = -EKEYREVOKED;
>> +	}
>> +
>> +	key_put(key);
>> +	return error;
>> +}
> 
> This is equivalent nvme_tls_key_lookup() so would it be more senseful
> to call it nfs_tls_key_lookup()? I'm also a bit puzzled how the code
> will associate nfs_keyring to all this (e.g., with keyring_search as
> done in nvme_tls_psk_lookup())?
> 
With this patch the keyring is pretty much immaterial; the interface
is passing in a serial number which is unique across all keyrings.
Where the keyring comes in when looking up keys on the TLS server,
as there the TLS client hello only transports the key description
(which are not required to be unique across all keyrings).
So there we'll need the keyring to be specified.
But for the client we really don't.

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 1/2] NFS: support the kernel keyring for TLS

[Christoph Hellwig]

On Thu, May 15, 2025 at 04:46:43PM +0200, Hannes Reinecke wrote:
> With this patch the keyring is pretty much immaterial; the interface
> is passing in a serial number which is unique across all keyrings.
> Where the keyring comes in when looking up keys on the TLS server,
> as there the TLS client hello only transports the key description
> (which are not required to be unique across all keyrings).
> So there we'll need the keyring to be specified.
> But for the client we really don't.

Yes.  Patch 1 on it's own actually works fine-ish.  The big difference
is that the keys would have to be made user-readable as without the
keyring, tlshd would not be the possesor of the key.

Re: [PATCH 1/2] NFS: support the kernel keyring for TLS

[Jarkko Sakkinen]

On Thu, May 15, 2025 at 04:46:43PM +0200, Hannes Reinecke wrote:
> > This is equivalent nvme_tls_key_lookup() so would it be more senseful
> > to call it nfs_tls_key_lookup()? I'm also a bit puzzled how the code
> > will associate nfs_keyring to all this (e.g., with keyring_search as
> > done in nvme_tls_psk_lookup())?
> > 
> With this patch the keyring is pretty much immaterial; the interface
> is passing in a serial number which is unique across all keyrings.
> Where the keyring comes in when looking up keys on the TLS server,
> as there the TLS client hello only transports the key description
> (which are not required to be unique across all keyrings).
> So there we'll need the keyring to be specified.
> But for the client we really don't.

I did not see anything that would depend on having 2/2 at all, or
it getting populated.

> 
> 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

BR, Jarkko

Re: [PATCH 1/2] NFS: support the kernel keyring for TLS

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 2/2] nfs: create a kernel keyring

[Christoph Hellwig]

Create a kernel .nfs keyring similar to the nvme .nvme one.  Unlike for
a userspace-created keyrind, tlshd is a possesor of the keys with this
and thus the keys don't need user read permissions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/inode.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 119e447758b9..e7a519f5b6bc 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -2571,6 +2571,35 @@ static struct pernet_operations nfs_net_ops = {
 	.size = sizeof(struct nfs_net),
 };
 
+#ifdef CONFIG_KEYS
+static struct key *nfs_keyring;
+
+static int __init nfs_init_keyring(void)
+{
+	nfs_keyring = keyring_alloc(".nfs",
+			     GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+			     current_cred(),
+			     (KEY_POS_ALL & ~KEY_POS_SETATTR) |
+			     (KEY_USR_ALL & ~KEY_USR_SETATTR),
+			     KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
+	return PTR_ERR_OR_ZERO(nfs_keyring);
+}
+
+static void __exit nfs_exit_keyring(void)
+{
+	key_put(nfs_keyring);
+}
+#else
+static inline int nfs_init_keyring(void)
+{
+	return 0;
+}
+
+static inline void nfs_exit_keyring(void)
+{
+}
+#endif /* CONFIG_KEYS */
+
 /*
  * Initialize NFS
  */
@@ -2578,6 +2607,10 @@ static int __init init_nfs_fs(void)
 {
 	int err;
 
+	err = nfs_init_keyring();
+	if (err)
+		return err;
+
 	err = nfs_sysfs_init();
 	if (err < 0)
 		goto out10;
@@ -2638,6 +2671,7 @@ static int __init init_nfs_fs(void)
 out9:
 	nfs_sysfs_exit();
 out10:
+	nfs_exit_keyring();
 	return err;
 }
 
@@ -2653,6 +2687,7 @@ static void __exit exit_nfs_fs(void)
 	nfs_fs_proc_exit();
 	nfsiod_stop();
 	nfs_sysfs_exit();
+	nfs_exit_keyring();
 }
 
 /* Not quite true; I just maintain it */
-- 
2.47.2

Re: [PATCH 2/2] nfs: create a kernel keyring

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/2] nfs: create a kernel keyring

[Jarkko Sakkinen]

On Fri, May 16, 2025 at 02:47:18PM +0300, Sagi Grimberg wrote:
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Based on?

BR, Jarkko

Re: [PATCH 2/2] nfs: create a kernel keyring

[Sagi Grimberg]

On 16/05/2025 20:03, Jarkko Sakkinen wrote:
> On Fri, May 16, 2025 at 02:47:18PM +0300, Sagi Grimberg wrote:
>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Based on?

Based on the same that nvme is doing. The only reason I see to have it
is to avoid having the user explicitly set perms on the key for tlshd to be
able to load it. nvme creates its own keyring that possessors can use, 
so makes
sense that nfs has this keyring as well.

Re: [PATCH 2/2] nfs: create a kernel keyring

[Christoph Hellwig]

On Sat, May 17, 2025 at 12:45:02PM +0300, Sagi Grimberg wrote:
>
>
> On 16/05/2025 20:03, Jarkko Sakkinen wrote:
>> On Fri, May 16, 2025 at 02:47:18PM +0300, Sagi Grimberg wrote:
>>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>> Based on?
>
> Based on the same that nvme is doing. The only reason I see to have it
> is to avoid having the user explicitly set perms on the key for tlshd to be
> able to load it. nvme creates its own keyring that possessors can use, so 
> makes
> sense that nfs has this keyring as well.

Jarkoo, can you please state your objections clearly?  You've only
asked this one liner question in response to Sagi's question but not
even commented the original patch.

Re: [PATCH 2/2] nfs: create a kernel keyring

[Jarkko Sakkinen]

On Mon, Jun 02, 2025 at 05:25:25PM +0200, Christoph Hellwig wrote:
> On Sat, May 17, 2025 at 12:45:02PM +0300, Sagi Grimberg wrote:
> >
> >
> > On 16/05/2025 20:03, Jarkko Sakkinen wrote:
> >> On Fri, May 16, 2025 at 02:47:18PM +0300, Sagi Grimberg wrote:
> >>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> >> Based on?
> >
> > Based on the same that nvme is doing. The only reason I see to have it
> > is to avoid having the user explicitly set perms on the key for tlshd to be
> > able to load it. nvme creates its own keyring that possessors can use, so 
> > makes
> > sense that nfs has this keyring as well.
> 
> Jarkoo, can you please state your objections clearly?  You've only
> asked this one liner question in response to Sagi's question but not
> even commented the original patch.

OK, I put this in simple terms, so perhaps I learn something from
nvme and nfs code:

1. The code change itself, if this keyring is needed, it looks
   reasonable.
2. However, I don't see any callers within the scope of patch set
   for this keyring.

I could quite quickly grab the idea how NVME uses nvme_keyring in TLS
handshake code from drivers/nvme/target/{configfs.c,tcp.c}. I guess
similar idea will be used in nfs code but I don't see any use for it
in the patch set.

Thus, it is hard to grasp the idea of having this patch applied without
any supplemental patch set.

BR, Jarkko

Re: [PATCH 2/2] nfs: create a kernel keyring

[Christoph Hellwig]

On Wed, Jun 04, 2025 at 07:42:52PM +0300, Jarkko Sakkinen wrote:
> OK, I put this in simple terms, so perhaps I learn something from
> nvme and nfs code:
> 
> 1. The code change itself, if this keyring is needed, it looks
>    reasonable.
> 2. However, I don't see any callers within the scope of patch set
>    for this keyring.
> 
> I could quite quickly grab the idea how NVME uses nvme_keyring in TLS
> handshake code from drivers/nvme/target/{configfs.c,tcp.c}. I guess
> similar idea will be used in nfs code but I don't see any use for it
> in the patch set.
> 
> Thus, it is hard to grasp the idea of having this patch applied without
> any supplemental patch set.

Maybe I'm missing something.  The reason I added the keyring was that
without it, tlshd is not the possesor of the keys and can't read them.

I guess you refer to the fact that nvme_tls_psk_lookup does a
keyring_search and nothing in the NFS code does?  nvme_tls_psk_lookup is
only used for the default key based on the server side identification in
NVMe, a concept that doesn't exist in NFS.  But the fact that the keys
aren't otherwise readable exists for both nvme and NFS.

Re: [PATCH 2/2] nfs: create a kernel keyring

[Jarkko Sakkinen]

On Thu, Jun 05, 2025 at 06:28:02AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 04, 2025 at 07:42:52PM +0300, Jarkko Sakkinen wrote:
> > OK, I put this in simple terms, so perhaps I learn something from
> > nvme and nfs code:
> > 
> > 1. The code change itself, if this keyring is needed, it looks
> >    reasonable.
> > 2. However, I don't see any callers within the scope of patch set
> >    for this keyring.
> > 
> > I could quite quickly grab the idea how NVME uses nvme_keyring in TLS
> > handshake code from drivers/nvme/target/{configfs.c,tcp.c}. I guess
> > similar idea will be used in nfs code but I don't see any use for it
> > in the patch set.
> > 
> > Thus, it is hard to grasp the idea of having this patch applied without
> > any supplemental patch set.
> 
> Maybe I'm missing something.  The reason I added the keyring was that
> without it, tlshd is not the possesor of the keys and can't read them.
> 
> I guess you refer to the fact that nvme_tls_psk_lookup does a
> keyring_search and nothing in the NFS code does?  nvme_tls_psk_lookup is
> only used for the default key based on the server side identification in
> NVMe, a concept that doesn't exist in NFS.  But the fact that the keys
> aren't otherwise readable exists for both nvme and NFS.

Ah, ok this cleared it up, thanks! Just learning these subsystem,
appreciate the patience with this one :-)

BR, Jarkko

Re: [PATCH 2/2] nfs: create a kernel keyring

[Christoph Hellwig]

On Fri, Jun 06, 2025 at 07:47:57PM +0300, Jarkko Sakkinen wrote:
> Ah, ok this cleared it up, thanks! Just learning these subsystem,
> appreciate the patience with this one :-)

I'm also just learning the keyring, so double checking from that
perspective is also always welcome..

Re: [PATCH 2/2] nfs: create a kernel keyring

[Jarkko Sakkinen]

On Mon, 2025-06-09 at 06:01 +0200, Christoph Hellwig wrote:
> On Fri, Jun 06, 2025 at 07:47:57PM +0300, Jarkko Sakkinen wrote:
> > Ah, ok this cleared it up, thanks! Just learning these subsystem,
> > appreciate the patience with this one :-)
> 
> I'm also just learning the keyring, so double checking from that
> perspective is also always welcome..

After quickly studying tlshd, my understanding is that the ".nfs" is a
"vault for transient stuff" passed to "keyrings" configuration option.
After that serials within that vault are passed to mount-options
defined in 1/2.

Stating the obvious, just checking that I understand the user story...

BR, Jarkko

Re: [PATCH 2/2] nfs: create a kernel keyring

[Christoph Hellwig]

On Tue, Jun 10, 2025 at 12:28:49AM +0300, Jarkko Sakkinen wrote:
> On Mon, 2025-06-09 at 06:01 +0200, Christoph Hellwig wrote:
> > On Fri, Jun 06, 2025 at 07:47:57PM +0300, Jarkko Sakkinen wrote:
> > > Ah, ok this cleared it up, thanks! Just learning these subsystem,
> > > appreciate the patience with this one :-)
> > 
> > I'm also just learning the keyring, so double checking from that
> > perspective is also always welcome..
> 
> After quickly studying tlshd, my understanding is that the ".nfs" is a
> "vault for transient stuff" passed to "keyrings" configuration option.
> After that serials within that vault are passed to mount-options
> defined in 1/2.

Yes.  I'll try to make it more clear for a resend, and I also plan
to write documents for using TLS and thus the keyrings with NFS and
nvme.

Re: [PATCH 2/2] nfs: create a kernel keyring

[kernel test robot]

Hi Christoph,

kernel test robot noticed the following build warnings:

[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on linus/master v6.15-rc6 next-20250516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christoph-Hellwig/nfs-create-a-kernel-keyring/20250515-205131
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250515115107.33052-3-hch%40lst.de
patch subject: [PATCH 2/2] nfs: create a kernel keyring
config: sh-sh03_defconfig (https://download.01.org/0day-ci/archive/20250518/202505180251.Qt2Rlaed-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250518/202505180251.Qt2Rlaed-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505180251.Qt2Rlaed-lkp@intel.com/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux: section mismatch in reference: init_nfs_fs+0x18c (section: .init.text) -> exit_misc_binfmt (section: .exit.text)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: support keyrings for NFS TLS mounts v2

[Chuck Lever]

On 5/15/25 7:50 AM, Christoph Hellwig wrote:
> Hi all,
> 
> this series allows storing the key and certificate for NFS over
> TLS mounts in the keyring and be specified using a mount option.
> This way they don't need to be hardcoded in the global tlshd.conf
> configuration file and can even be different per-mount.
> 
> Note that for now the .nfs keyring still needs to be added to
> tlshd.conf, but that should go away with the handshake enhacement
> from Hannes.

Just curious: Is there a downside to shipping a default /etc/tlshd.conf
with the NVMe and NFS keyrings already added?


> Changes since v1:
>  - don't depend on nfsv4 for the keyring
>  - fix compile when the kernel keyring is disabled


-- 
Chuck Lever

Re: support keyrings for NFS TLS mounts v2

[Christoph Hellwig]

On Thu, May 15, 2025 at 08:31:23AM -0400, Chuck Lever wrote:
> > Note that for now the .nfs keyring still needs to be added to
> > tlshd.conf, but that should go away with the handshake enhacement
> > from Hannes.
> 
> Just curious: Is there a downside to shipping a default /etc/tlshd.conf
> with the NVMe and NFS keyrings already added?

That's probably a good idea for the current situation, but doesn't
resolve solve the overall problem of having to add every new service
there.

Re: support keyrings for NFS TLS mounts v2

[Sagi Grimberg]

On 16/05/2025 8:16, Christoph Hellwig wrote:
> On Thu, May 15, 2025 at 08:31:23AM -0400, Chuck Lever wrote:
>>> Note that for now the .nfs keyring still needs to be added to
>>> tlshd.conf, but that should go away with the handshake enhacement
>>> from Hannes.
>> Just curious: Is there a downside to shipping a default /etc/tlshd.conf
>> with the NVMe and NFS keyrings already added?
> That's probably a good idea for the current situation, but doesn't
> resolve solve the overall problem of having to add every new service
> there.

IIRC Hannes told me that latest tlshd already does this for .nvme keyring

Re: support keyrings for NFS TLS mounts v2

[Christoph Hellwig]

Trond/Anna:

can you queue this up?  If not can you tell me what is missing?

Re: support keyrings for NFS TLS mounts v2

[Trond Myklebust]

On Thu, 2025-07-10 at 09:25 +0200, Christoph Hellwig wrote:
> Trond/Anna:
> 
> can you queue this up?  If not can you tell me what is missing?
> 

Sorry. That series managed to fly under the radar while I was on
vacation. It looks good to me, so I'm picking it into my 'testing'
branch now.

Thanks for the reminder!

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com

RFC: support keyrings for NFS TLS mounts

[Christoph Hellwig]

Hi all,

this series allows storing the key and certificate for NFS over
TLS mounts in the keyring and be specified using a mount option.
This way they don't need to be hardcoded in the global tlshd.conf
configuration file and can even be different per-mount.

Note that for now the .nfs keyring still needs to be added to
tlshd.conf, but I'm going to look into a way out of that.

This is in many ways based on how NVMe handles the keyring for
TLS, and I might not fully understand what I'm doing.

[PATCH 2/2] nfs: create a kernel keyring

[Christoph Hellwig]

Create a kernel .nfs keyring similar to the nvme .nvme one.  Unlike for
a userspace-created keyrind, tlshd is a possesor of the keys with this
and thus the keys don't need user read permissions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/inode.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 119e447758b9..fb1fe1bdfe92 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -2571,6 +2571,8 @@ static struct pernet_operations nfs_net_ops = {
 	.size = sizeof(struct nfs_net),
 };
 
+static struct key *nfs_keyring;
+
 /*
  * Initialize NFS
  */
@@ -2578,6 +2580,17 @@ static int __init init_nfs_fs(void)
 {
 	int err;
 
+	if (IS_ENABLED(CONFIG_NFS_V4)) {
+		nfs_keyring = keyring_alloc(".nfs",
+				     GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+				     current_cred(),
+				     (KEY_POS_ALL & ~KEY_POS_SETATTR) |
+				     (KEY_USR_ALL & ~KEY_USR_SETATTR),
+				     KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
+		if (IS_ERR(nfs_keyring))
+			return PTR_ERR(nfs_keyring);
+	}
+
 	err = nfs_sysfs_init();
 	if (err < 0)
 		goto out10;
@@ -2653,6 +2666,8 @@ static void __exit exit_nfs_fs(void)
 	nfs_fs_proc_exit();
 	nfsiod_stop();
 	nfs_sysfs_exit();
+	if (IS_ENABLED(CONFIG_NFS_V4))
+		key_put(nfs_keyring);
 }
 
 /* Not quite true; I just maintain it */
-- 
2.47.2

Re: [PATCH 2/2] nfs: create a kernel keyring

[Sagi Grimberg]

On 07/05/2025 11:09, Christoph Hellwig wrote:
> Create a kernel .nfs keyring similar to the nvme .nvme one.  Unlike for
> a userspace-created keyrind, tlshd is a possesor of the keys with this
> and thus the keys don't need user read permissions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/nfs/inode.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 119e447758b9..fb1fe1bdfe92 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -2571,6 +2571,8 @@ static struct pernet_operations nfs_net_ops = {
>   	.size = sizeof(struct nfs_net),
>   };
>   
> +static struct key *nfs_keyring;
> +
>   /*
>    * Initialize NFS
>    */
> @@ -2578,6 +2580,17 @@ static int __init init_nfs_fs(void)
>   {
>   	int err;
>   
> +	if (IS_ENABLED(CONFIG_NFS_V4)) {

xprtsec is sunrpc, meaning it is also supported with nfsv3.

> +		nfs_keyring = keyring_alloc(".nfs",
> +				     GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
> +				     current_cred(),
> +				     (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +				     (KEY_USR_ALL & ~KEY_USR_SETATTR),
> +				     KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
> +		if (IS_ERR(nfs_keyring))
> +			return PTR_ERR(nfs_keyring);
> +	}
> +
>   	err = nfs_sysfs_init();
>   	if (err < 0)
>   		goto out10;
> @@ -2653,6 +2666,8 @@ static void __exit exit_nfs_fs(void)
>   	nfs_fs_proc_exit();
>   	nfsiod_stop();
>   	nfs_sysfs_exit();
> +	if (IS_ENABLED(CONFIG_NFS_V4))
> +		key_put(nfs_keyring);

Same comment

>   }
>   
>   /* Not quite true; I just maintain it */

Re: [PATCH 2/2] nfs: create a kernel keyring

[kernel test robot]

Hi Christoph,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on linus/master v6.15-rc5 next-20250507]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christoph-Hellwig/nfs-create-a-kernel-keyring/20250507-171041
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250507080944.3947782-3-hch%40lst.de
patch subject: [PATCH 2/2] nfs: create a kernel keyring
config: hexagon-defconfig (https://download.01.org/0day-ci/archive/20250508/202505081720.vtCPwAc0-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project f819f46284f2a79790038e1f6649172789734ae8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250508/202505081720.vtCPwAc0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505081720.vtCPwAc0-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/nfs/inode.c:2584:17: error: call to undeclared function 'keyring_alloc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    2584 |                 nfs_keyring = keyring_alloc(".nfs",
         |                               ^
>> fs/nfs/inode.c:2587:11: error: use of undeclared identifier 'KEY_POS_ALL'
    2587 |                                      (KEY_POS_ALL & ~KEY_POS_SETATTR) |
         |                                       ^
>> fs/nfs/inode.c:2587:26: error: use of undeclared identifier 'KEY_POS_SETATTR'
    2587 |                                      (KEY_POS_ALL & ~KEY_POS_SETATTR) |
         |                                                      ^
>> fs/nfs/inode.c:2588:11: error: use of undeclared identifier 'KEY_USR_ALL'
    2588 |                                      (KEY_USR_ALL & ~KEY_USR_SETATTR),
         |                                       ^
>> fs/nfs/inode.c:2588:26: error: use of undeclared identifier 'KEY_USR_SETATTR'
    2588 |                                      (KEY_USR_ALL & ~KEY_USR_SETATTR),
         |                                                      ^
>> fs/nfs/inode.c:2589:10: error: use of undeclared identifier 'KEY_ALLOC_NOT_IN_QUOTA'
    2589 |                                      KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
         |                                      ^
   6 errors generated.


vim +/keyring_alloc +2584 fs/nfs/inode.c

  2575	
  2576	/*
  2577	 * Initialize NFS
  2578	 */
  2579	static int __init init_nfs_fs(void)
  2580	{
  2581		int err;
  2582	
  2583		if (IS_ENABLED(CONFIG_NFS_V4)) {
> 2584			nfs_keyring = keyring_alloc(".nfs",
  2585					     GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
  2586					     current_cred(),
> 2587					     (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> 2588					     (KEY_USR_ALL & ~KEY_USR_SETATTR),
> 2589					     KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
  2590			if (IS_ERR(nfs_keyring))
  2591				return PTR_ERR(nfs_keyring);
  2592		}
  2593	
  2594		err = nfs_sysfs_init();
  2595		if (err < 0)
  2596			goto out10;
  2597	
  2598		err = register_pernet_subsys(&nfs_net_ops);
  2599		if (err < 0)
  2600			goto out9;
  2601	
  2602		err = nfsiod_start();
  2603		if (err)
  2604			goto out7;
  2605	
  2606		err = nfs_fs_proc_init();
  2607		if (err)
  2608			goto out6;
  2609	
  2610		err = nfs_init_nfspagecache();
  2611		if (err)
  2612			goto out5;
  2613	
  2614		err = nfs_init_inodecache();
  2615		if (err)
  2616			goto out4;
  2617	
  2618		err = nfs_init_readpagecache();
  2619		if (err)
  2620			goto out3;
  2621	
  2622		err = nfs_init_writepagecache();
  2623		if (err)
  2624			goto out2;
  2625	
  2626		err = nfs_init_directcache();
  2627		if (err)
  2628			goto out1;
  2629	
  2630		err = register_nfs_fs();
  2631		if (err)
  2632			goto out0;
  2633	
  2634		return 0;
  2635	out0:
  2636		nfs_destroy_directcache();
  2637	out1:
  2638		nfs_destroy_writepagecache();
  2639	out2:
  2640		nfs_destroy_readpagecache();
  2641	out3:
  2642		nfs_destroy_inodecache();
  2643	out4:
  2644		nfs_destroy_nfspagecache();
  2645	out5:
  2646		nfs_fs_proc_exit();
  2647	out6:
  2648		nfsiod_stop();
  2649	out7:
  2650		unregister_pernet_subsys(&nfs_net_ops);
  2651	out9:
  2652		nfs_sysfs_exit();
  2653	out10:
  2654		return err;
  2655	}
  2656	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki