Re: [PATCH v3] proc/sysctl: add shared variables for range check
On Tue, Apr 23, 2019 at 5:28 AM Matteo Croce wrote:
>
> On April 19, 2019 10:07:14 AM GMT+09:00, Matthew Wilcox
> wrote:
> > On Fri, Apr 19, 2019 at 09:17:17AM +0900, Matteo Croce wrote:
> > > > extern const int sysctl_zero;
> > > > /* comment goes here */
> > > > #define SYSCTL_ZERO ((void *)_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 *)_##x)
> > >
> > > to avoid defining one for every value. And when we decide that
> > everything can be const, we just update the macro.
> >
> > If we're going to do that, we can save two EXPORTs and do:
> >
> > const int sysctl_vals[] = { 0, 1, -1 };
> > EXPORT_SYMBOL(sysctl_vals);
> >
> > #define SYSCTL_ZERO ((void *)_vals[0])
>
> Hi Matthew,
>
> I like this approach, regardless of the const or not const extra1.
>
> I'll be AFK for a few days, then I will investigate if extra1,2 can be made
> const and then prepare a v4 with the single export.
Hi all,
I turned extra{1,2) to const and I see no issues.
I'm sending a v4 with extra{1,2} const, a single export for all vars
as suggested by Matthew, and the define suggested by Andrew.
Comments are welcome as usual.
Regards,
--
Matteo Croce
per aspera ad upstream
Re: [PATCH v3] proc/sysctl: add shared variables for range check
On April 19, 2019 10:07:14 AM GMT+09:00, Matthew Wilcox
wrote:
> On Fri, Apr 19, 2019 at 09:17:17AM +0900, Matteo Croce wrote:
> > > extern const int sysctl_zero;
> > > /* comment goes here */
> > > #define SYSCTL_ZERO ((void *)_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 *)_##x)
> >
> > to avoid defining one for every value. And when we decide that
> everything can be const, we just update the macro.
>
> If we're going to do that, we can save two EXPORTs and do:
>
> const int sysctl_vals[] = { 0, 1, -1 };
> EXPORT_SYMBOL(sysctl_vals);
>
> #define SYSCTL_ZERO ((void *)_vals[0])
Hi Matthew,
I like this approach, regardless of the const or not const extra1.
I'll be AFK for a few days, then I will investigate if extra1,2 can be made
const and then prepare a v4 with the single export.
Thanks,
--
Matteo Croce
per aspera ad upstream
Re: [PATCH v3] proc/sysctl: add shared variables for range check
On Fri, Apr 19, 2019 at 09:17:17AM +0900, Matteo Croce wrote:
> > extern const int sysctl_zero;
> > /* comment goes here */
> > #define SYSCTL_ZERO ((void *)_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 *)_##x)
>
> to avoid defining one for every value. And when we decide that everything can
> be const, we just update the macro.
If we're going to do that, we can save two EXPORTs and do:
const int sysctl_vals[] = { 0, 1, -1 };
EXPORT_SYMBOL(sysctl_vals);
#define SYSCTL_ZERO ((void *)_vals[0])
Re: [PATCH v3] proc/sysctl: add shared variables for range check
On April 19, 2019 7:40:45 AM GMT+09:00, Andrew Morton
wrote:
> On Wed, 17 Apr 2019 15:15:31 +0200 Matteo Croce
> 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 = _active,
> > .maxlen = sizeof(int),
> > - .extra1 = ,
> > - .extra2 = ,
> > + .extra1 = (void *)_zero,
> > + .extra2 = (void *)_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 *)_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 *)_##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
Re: [PATCH v3] proc/sysctl: add shared variables for range check
On Wed, 17 Apr 2019 15:15:31 +0200 Matteo Croce 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 = _active,
> .maxlen = sizeof(int),
> - .extra1 = ,
> - .extra2 = ,
> + .extra1 = (void *)_zero,
> + .extra2 = (void *)_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 *)_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 :(
Re: [PATCH v3] proc/sysctl: add shared variables for range check
On April 18, 2019 12:49:00 AM GMT+09:00, Matthew Wilcox wrote: > On Wed, Apr 17, 2019 at 03:15:31PM +0200, Matteo Croce 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. > > Does this actually cause the kernel size to shrink? EXPORT_SYMBOL > isn't > free, you know. Hi Matthew, In this case we have three __kstrtab_ symbols of size 11,12 and 15, and a bunch of u32 removed, so the size should shrink anyway. I will try to calculate the exact saving with bloat-o-meter. Regards, -- Matteo Croce per aspera ad upstream
Re: [PATCH v3] proc/sysctl: add shared variables for range check
On Wed, Apr 17, 2019 at 03:15:31PM +0200, Matteo Croce 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. Does this actually cause the kernel size to shrink? EXPORT_SYMBOL isn't free, you know.
[PATCH v3] proc/sysctl: add shared variables for range check
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.
Signed-off-by: Matteo Croce
---
arch/s390/appldata/appldata_base.c| 15 +-
arch/s390/kernel/topology.c | 6 +-
arch/x86/entry/vdso/vdso32-setup.c| 7 +-
arch/x86/kernel/itmt.c| 6 +-
drivers/base/firmware_loader/fallback_table.c | 11 +-
drivers/gpu/drm/i915/i915_perf.c | 8 +-
drivers/hv/vmbus_drv.c| 6 +-
drivers/s390/char/sclp_async.c| 7 +-
drivers/tty/tty_ldisc.c | 6 +-
drivers/xen/balloon.c | 7 +-
fs/eventpoll.c| 3 +-
fs/notify/inotify/inotify_user.c | 8 +-
fs/proc/proc_sysctl.c | 10 +
include/linux/sysctl.h| 4 +
ipc/ipc_sysctl.c | 35 ++--
kernel/pid_namespace.c| 3 +-
kernel/sysctl.c | 193 +-
kernel/ucount.c | 6 +-
net/core/neighbour.c | 20 +-
net/core/sysctl_net_core.c| 34 ++-
net/dccp/sysctl.c | 16 +-
net/ipv4/sysctl_net_ipv4.c| 58 +++---
net/ipv6/addrconf.c | 6 +-
net/ipv6/route.c | 7 +-
net/ipv6/sysctl_net_ipv6.c| 8 +-
net/mpls/af_mpls.c| 10 +-
net/netfilter/ipvs/ip_vs_ctl.c| 3 +-
net/rxrpc/sysctl.c| 9 +-
net/sctp/sysctl.c | 35 ++--
net/sunrpc/xprtrdma/transport.c | 3 +-
security/keys/sysctl.c| 26 ++-
security/loadpin/loadpin.c| 6 +-
security/yama/yama_lsm.c | 3 +-
33 files changed, 266 insertions(+), 319 deletions(-)
diff --git a/arch/s390/appldata/appldata_base.c
b/arch/s390/appldata/appldata_base.c
index e4b58240ec53..82ae75b5ead6 100644
--- 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 = _active,
.maxlen = sizeof(int),
- .extra1 = ,
- .extra2 = ,
+ .extra1 = (void *)_zero,
+ .extra2 = (void *)_one,
};
rc = proc_douintvec_minmax(_entry, write, buffer, lenp, ppos);
@@ -255,13 +253,12 @@ appldata_interval_handler(struct ctl_table *ctl, int
write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
int interval = appldata_interval;
- int one = 1;
int rc;
struct ctl_table ctl_entry = {
.procname = ctl->procname,
.data = ,
.maxlen = sizeof(int),
- .extra1 = ,
+ .extra1 = (void *)_one,
};
rc = proc_dointvec_minmax(_entry, write, buffer, lenp, ppos);
@@ -289,13 +286,11 @@ appldata_generic_handler(struct ctl_table *ctl, int write,
struct list_head *lh;
int rc, found;
int active;
- int zero = 0;
- int one = 1;
struct ctl_table ctl_entry = {
.data = ,
.maxlen = sizeof(int),
- .extra1 = ,
- .extra2 = ,
+ .extra1 = (void *)_zero,
+ .extra2 = (void *)_one,
};
found = 0;
diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 8964a3f60aad..347eb433c0a5 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -587,15 +587,13
