Re: [PATCH 2/2] fsverity: move sysctl registration out of signature.c

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]

On Wed, Jul 05, 2023 at 02:27:43PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently the registration of the fsverity sysctls happens in
> signature.c, which couples it to CONFIG_FS_VERITY_BUILTIN_SIGNATURES.
> 
> This makes it hard to add new sysctls unrelated to builtin signatures.
> 
> Also, some users have started checking whether the directory
> /proc/sys/fs/verity exists as a way to tell whether fsverity is
> supported.  This isn't the intended method; instead, the existence of
> /sys/fs/$fstype/features/verity should be checked, or users should just
> try to use the fsverity ioctls.  Regardlesss, it should be made to work
> as expected without a dependency on CONFIG_FS_VERITY_BUILTIN_SIGNATURES.
> 
> Therefore, move the sysctl registration into init.c.  With
> CONFIG_FS_VERITY_BUILTIN_SIGNATURES, nothing changes.  Without it, but
> with CONFIG_FS_VERITY, an empty list of sysctls is now registered.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/verity/fsverity_private.h |  1 +
>  fs/verity/init.c             | 32 ++++++++++++++++++++++++++++++++
>  fs/verity/signature.c        | 33 +--------------------------------
>  3 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> index c5ab9023dd2d3..d071a6e32581e 100644
> --- a/fs/verity/fsverity_private.h
> +++ b/fs/verity/fsverity_private.h
> @@ -123,6 +123,7 @@ void __init fsverity_init_info_cache(void);
>  /* signature.c */
>  
>  #ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> +extern int fsverity_require_signatures;
>  int fsverity_verify_signature(const struct fsverity_info *vi,
>  			      const u8 *signature, size_t sig_size);
>  
> diff --git a/fs/verity/init.c b/fs/verity/init.c
> index bcd11d63eb1ca..a29f062f6047b 100644
> --- a/fs/verity/init.c
> +++ b/fs/verity/init.c
> @@ -9,6 +9,37 @@
>  
>  #include <linux/ratelimit.h>
>  
> +#ifdef CONFIG_SYSCTL
> +static struct ctl_table_header *fsverity_sysctl_header;
> +
> +static struct ctl_table fsverity_sysctl_table[] = {
> +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> +	{
> +		.procname       = "require_signatures",
> +		.data           = &fsverity_require_signatures,
> +		.maxlen         = sizeof(int),
> +		.mode           = 0644,
> +		.proc_handler   = proc_dointvec_minmax,
> +		.extra1         = SYSCTL_ZERO,
> +		.extra2         = SYSCTL_ONE,
> +	},
> +#endif
> +	{ }
> +};
> +
Just to double check: When CONFIG_FS_VERITY_BUILTIN_SIGNATURES is not
defined then you expect an empty directory under "fs/verity". right?

I'm double checking as part of the ongoing "removing the sentinel" patch
set. Latest patchset here
https://lore.kernel.org/all/20230906-jag-sysctl_remove_empty_elem_arch-v1-0-3935d4854248@samsung.com/.
The filesystem chunk has not yet been publish, but I'm making it ready
for when that time comes.

> +static void __init fsverity_init_sysctl(void)
> +{
> +	fsverity_sysctl_header = register_sysctl("fs/verity",
> +						 fsverity_sysctl_table);
> +	if (!fsverity_sysctl_header)
> +		panic("fsverity sysctl registration failed");
> +}
> +#else /* CONFIG_SYSCTL */
> +static inline void fsverity_init_sysctl(void)
> +{
> +}
> +#endif /* !CONFIG_SYSCTL */
> +
>  void fsverity_msg(const struct inode *inode, const char *level,
>  		  const char *fmt, ...)
>  {
> @@ -36,6 +67,7 @@ static int __init fsverity_init(void)
>  	fsverity_check_hash_algs();
...

Best
-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]