RFC: support keyrings for NFS TLS mounts

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 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 | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 13f71ca8c974..58845c414893 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,25 @@ static int nfs_parse_version_string(struct fs_context *fc,
 	return 0;
 }
 
+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;
+}
+
 /*
  * Parse a single mount parameter.
  */
@@ -807,6 +830,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

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

[Sagi Grimberg]

Pretty straight-forward I think.

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

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

[Chuck Lever]

On 5/7/25 4:09 AM, 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.

I proposed adding support to the kernel's x.509 implementation for PEM
format keys, and it was rejected. So DER it is.


> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/fs_context.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 13f71ca8c974..58845c414893 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,25 @@ static int nfs_parse_version_string(struct fs_context *fc,
>  	return 0;
>  }
>  
> +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;
> +}
> +
>  /*
>   * Parse a single mount parameter.
>   */
> @@ -807,6 +830,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)


-- 
Chuck Lever

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

[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-2-hch%40lst.de
patch subject: [PATCH 1/2] NFS: support the kernel keyring for TLS
config: hexagon-defconfig (https://download.01.org/0day-ci/archive/20250508/202505081535.3PS62D63-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/202505081535.3PS62D63-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/202505081535.3PS62D63-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/nfs/fs_context.c:567:37: error: incomplete definition of type 'struct key'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                                         ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:45:37: note: expanded from macro 'bitop'
      45 |           __builtin_constant_p((uintptr_t)(addr) != (uintptr_t)NULL) && \
         |                                            ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:567:37: error: incomplete definition of type 'struct key'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                                         ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:46:16: note: expanded from macro 'bitop'
      46 |           (uintptr_t)(addr) != (uintptr_t)NULL &&                       \
         |                       ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:567:37: error: incomplete definition of type 'struct key'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                                         ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:47:50: note: expanded from macro 'bitop'
      47 |           __builtin_constant_p(*(const unsigned long *)(addr))) ?       \
         |                                                         ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:567:15: error: use of undeclared identifier 'KEY_FLAG_REVOKED'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                      ^
>> fs/nfs/fs_context.c:567:37: error: incomplete definition of type 'struct key'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                                         ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:48:17: note: expanded from macro 'bitop'
      48 |          const##op(nr, addr) : op(nr, addr))
         |                        ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:567:37: error: incomplete definition of type 'struct key'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                                         ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:48:32: note: expanded from macro 'bitop'
      48 |          const##op(nr, addr) : op(nr, addr))
         |                                       ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:567:15: error: use of undeclared identifier 'KEY_FLAG_REVOKED'
     567 |         if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
         |                      ^
>> fs/nfs/fs_context.c:567:15: error: use of undeclared identifier 'KEY_FLAG_REVOKED'
   fs/nfs/fs_context.c:568:41: error: incomplete definition of type 'struct key'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                                             ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:45:37: note: expanded from macro 'bitop'
      45 |           __builtin_constant_p((uintptr_t)(addr) != (uintptr_t)NULL) && \
         |                                            ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
   fs/nfs/fs_context.c:568:41: error: incomplete definition of type 'struct key'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                                             ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:46:16: note: expanded from macro 'bitop'
      46 |           (uintptr_t)(addr) != (uintptr_t)NULL &&                       \
         |                       ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
   fs/nfs/fs_context.c:568:41: error: incomplete definition of type 'struct key'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                                             ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:47:50: note: expanded from macro 'bitop'
      47 |           __builtin_constant_p(*(const unsigned long *)(addr))) ?       \
         |                                                         ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:568:15: error: use of undeclared identifier 'KEY_FLAG_INVALIDATED'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                      ^
   fs/nfs/fs_context.c:568:41: error: incomplete definition of type 'struct key'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                                             ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:48:17: note: expanded from macro 'bitop'
      48 |          const##op(nr, addr) : op(nr, addr))
         |                        ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
   fs/nfs/fs_context.c:568:41: error: incomplete definition of type 'struct key'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                                             ~~~^
   include/linux/bitops.h:61:50: note: expanded from macro 'test_bit'
      61 | #define test_bit(nr, addr)              bitop(_test_bit, nr, addr)
         |                                                              ^~~~
   include/linux/bitops.h:48:32: note: expanded from macro 'bitop'
      48 |          const##op(nr, addr) : op(nr, addr))
         |                                       ^~~~
   include/linux/key.h:33:8: note: forward declaration of 'struct key'
      33 | struct key;
         |        ^
>> fs/nfs/fs_context.c:568:15: error: use of undeclared identifier 'KEY_FLAG_INVALIDATED'
     568 |             test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
         |                      ^
>> fs/nfs/fs_context.c:568:15: error: use of undeclared identifier 'KEY_FLAG_INVALIDATED'
   16 errors generated.


vim +567 fs/nfs/fs_context.c

   557	
   558	static int nfs_tls_key_verify(key_serial_t key_id)
   559	{
   560		struct key *key = key_lookup(key_id);
   561		int error = 0;
   562	
   563		if (IS_ERR(key)) {
   564			pr_err("key id %08x not found\n", key_id);
   565			return PTR_ERR(key);
   566		}
 > 567		if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
 > 568		    test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
   569			pr_err("key id %08x revoked\n", key_id);
   570			error = -EKEYREVOKED;
   571		}
   572	
   573		key_put(key);
   574		return error;
   575	}
   576	

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

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

[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

[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: [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.