linux-hardening - Re: [PATCH 1/2] ocfs2: Replace module_param_call with module_param

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Aug 2023 16:00:37 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Azeem Shaikh <azeemshaikh38@...il.com>
Cc:     Mark Fasheh <mark@...heh.com>, Joel Becker <jlbec@...lplan.org>,
        Joseph Qi <joseph.qi@...ux.alibaba.com>,
        linux-hardening@...r.kernel.org, ocfs2-devel@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Christian Brauner <brauner@...nel.org>,
        Dave Chinner <dchinner@...hat.com>,
        Jeff Layton <jlayton@...nel.org>
Subject: Re: [PATCH 1/2] ocfs2: Replace module_param_call with module_param_cb

On Wed, Aug 30, 2023 at 09:54:25PM +0000, Azeem Shaikh wrote:
> module_param_call has been marked obsolete [1], so replacing its usage with
> module_param_cb instead.
> 
> [1] https://elixir.bootlin.com/linux/v6.5/source/include/linux/moduleparam.h#L296
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@...il.com>
> ---
>  fs/ocfs2/dlmfs/dlmfs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
> index 81265123ce6c..33e529de93b2 100644
> --- a/fs/ocfs2/dlmfs/dlmfs.c
> +++ b/fs/ocfs2/dlmfs/dlmfs.c
> @@ -83,8 +83,11 @@ static int param_get_dlmfs_capabilities(char *buffer,
>  	return strlcpy(buffer, DLMFS_CAPABILITIES,
>  		       strlen(DLMFS_CAPABILITIES) + 1);
>  }
> -module_param_call(capabilities, param_set_dlmfs_capabilities,
> -		  param_get_dlmfs_capabilities, NULL, 0444);
> +static const struct kernel_param_ops dlmfs_capabilities_ops = {
> +	.set = param_set_dlmfs_capabilities,
> +	.get = param_get_dlmfs_capabilities,
> +};
> +module_param_cb(capabilities, &dlmfs_capabilities_ops, NULL, 0444);
>  MODULE_PARM_DESC(capabilities, DLMFS_CAPABILITIES);

Oh, hm. Yeah, that's not good documentation. It was originally added in
9bbb9e5a33109b2832e2e63dcc7a132924ab374b, but that was doing some
casting and other things during the conversion to an ops structure.

I cleaned up all the last of those back in 2017, and I should have
dropped the "Obsolete" comment:
ece1996a21eeb344b49200e627c6660111009c10
b2f270e8747387335d80428c576118e7d87f69cc

The resulting patch you sent ends up literally open coding what it
already does...

#define module_param_call(name, _set, _get, arg, perm)                  \
        static const struct kernel_param_ops __param_ops_##name =       \
                { .flags = 0, .set = _set, .get = _get };               \
        __module_param_call(MODULE_PARAM_PREFIX,                        \
                            name, &__param_ops_##name, arg, perm, -1, 0)

#define module_param_cb(name, ops, arg, perm)                                 \
        __module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0)

Looking at usage, it's still common:

$ git grep '^module_param_call(' | wc -l
54
$ git grep '^module_param_cb(' | wc -l
93

And the users of module_param_cb() appear to be almost universally
open-coding the result. Only a few initialize struct members that aren't
.get and .set:

$ git grep -B6 ^module_param_cb

I'd say drop this patch and instead patch moduleparam.h to not say it's
deprecated. :P

-Kees

-- 
Kees Cook