From: Matteo Croce <mcroce@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
linux-fsdevel@vger.kernel.org, Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v3] proc/sysctl: add shared variables for range check
Date: Fri, 19 Apr 2019 09:17:17 +0900 [thread overview]
Message-ID: <1109DAD2-E25B-47A3-8381-E02260FE51B9@redhat.com> (raw)
In-Reply-To: <20190418154045.2ad0bd50e73a6e71c0fac768@linux-foundation.org>
On April 19, 2019 7:40:45 AM GMT+09:00, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 17 Apr 2019 15:15:31 +0200 Matteo Croce <mcroce@redhat.com>
> wrote:
>
> > In the sysctl code the proc_dointvec_minmax() function is often used
> to
> > validate the user supplied value between an allowed range. This
> function
> > uses the extra1 and extra2 members from struct ctl_table as minimum
> and
> > maximum allowed value.
> >
> > On sysctl handler declaration, in every source file there are some
> readonly
> > variables containing just an integer which address is assigned to
> the
> > extra1 and extra2 members, so the sysctl range is enforced.
> >
> > The special values 0, 1 and INT_MAX are very often used as range
> boundary,
> > leading duplication of variables like zero=0, one=1, int_max=INT_MAX
> in
> > different source files:
> >
> > $ git grep -E '\.extra[12].*&(zero|one|int_max)\b' |wc -l
> > 245
> >
> > This patch adds three const variables for the most commonly used
> values,
> > and use them instead of creating a local one for every object file.
> >
> > ...
> >
> > --- a/arch/s390/appldata/appldata_base.c
> > +++ b/arch/s390/appldata/appldata_base.c
> > @@ -220,15 +220,13 @@ appldata_timer_handler(struct ctl_table *ctl,
> int write,
> > void __user *buffer, size_t *lenp, loff_t *ppos)
> > {
> > int timer_active = appldata_timer_active;
> > - int zero = 0;
> > - int one = 1;
> > int rc;
> > struct ctl_table ctl_entry = {
> > .procname = ctl->procname,
> > .data = &timer_active,
> > .maxlen = sizeof(int),
> > - .extra1 = &zero,
> > - .extra2 = &one,
> > + .extra1 = (void *)&sysctl_zero,
> > + .extra2 = (void *)&sysctl_one,
> > };
>
> Still not liking the casts :(
>
> Did we decide whether making extra1&2 const void*'s was feasible?
>
> I'm wondering if it would be better to do
>
> extern const int sysctl_zero;
> /* comment goes here */
> #define SYSCTL_ZERO ((void *)&sysctl_zero)
>
> and then use SYSCTL_ZERO everywhere. That centralizes the ugliness
> and
> makes it easier to switch over if/when extra1&2 are constified.
>
> But it's all a bit sad and lame :(
No, we didn't decide yet. I need to check for all extra1,2 assignment. Not an impossible task, anyway.
I agree that the casts are ugly. Your suggested macro moves the ugliness in a single point, which is good. Or maybe we can do a single macro like:
#define SYSCTL_VAL(x) ((void *)&sysctl_##x)
to avoid defining one for every value. And when we decide that everything can be const, we just update the macro.
Regards,
--
Matteo Croce
per aspera ad upstream
next prev parent reply other threads:[~2019-04-19 0:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 13:15 [PATCH v3] proc/sysctl: add shared variables for range check Matteo Croce
2019-04-17 15:49 ` Matthew Wilcox
2019-04-18 14:28 ` Matteo Croce
2019-04-18 22:40 ` Andrew Morton
2019-04-19 0:17 ` Matteo Croce [this message]
2019-04-19 1:07 ` Matthew Wilcox
2019-04-23 3:28 ` Matteo Croce
2019-04-29 22:21 ` Matteo Croce
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1109DAD2-E25B-47A3-8381-E02260FE51B9@redhat.com \
--to=mcroce@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).