[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202308301546.5C789E5EC@keescook>
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
Powered by blists - more mailing lists