* [PATCH] sysctl: add register_sysctl() dummy helper @ 2017-11-06 13:36 Arnd Bergmann 2017-11-06 16:50 ` Dave Martin ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Arnd Bergmann @ 2017-11-06 13:36 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Luis R. Rodriguez, Kees Cook Cc: Alex Bennée, Dave Martin, Arnd Bergmann, Eric W. Biederman, Andrew Morton, Thomas Gleixner, linux-kernel, linux-fsdevel The register_sysctl() function has been around for five years with commit fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") but now that arm64 started using it, I ran into a compile error: arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' This adds a inline function like we already have for register_sysctl_paths() and register_sysctl_table(). Fixes: 38b9aeb32fa7 ("arm64: Port deprecated instruction emulation to new sysctl interface") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- This should probably get merged through the arm64 tree to avoid bisection problems. --- include/linux/sysctl.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index fabe15e51c56..992bc9948232 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -216,6 +216,11 @@ static inline struct ctl_table_header *register_sysctl_paths( return NULL; } +static inline struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table) +{ + return NULL; +} + static inline void unregister_sysctl_table(struct ctl_table_header * table) { } -- 2.9.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] sysctl: add register_sysctl() dummy helper 2017-11-06 13:36 [PATCH] sysctl: add register_sysctl() dummy helper Arnd Bergmann @ 2017-11-06 16:50 ` Dave Martin 2017-11-06 17:12 ` Arnd Bergmann 2017-11-06 16:51 ` Kees Cook 2017-11-07 1:01 ` Will Deacon 2 siblings, 1 reply; 7+ messages in thread From: Dave Martin @ 2017-11-06 16:50 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, Will Deacon, Luis R. Rodriguez, Kees Cook, Alex Bennée, Eric W. Biederman, Andrew Morton, Thomas Gleixner, linux-kernel, linux-fsdevel On Mon, Nov 06, 2017 at 01:36:40PM +0000, Arnd Bergmann wrote: > The register_sysctl() function has been around for five years with commit > fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") > but now that arm64 started using it, I ran into a compile error: > > arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': > arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' Hmmm, looks like I missed this combination in testing. I wonder whether ARMV8_DEPRECATED without SYSCTL is really a good idea though: in that config, we build a lot of dead code and leak some memory today. The default emulation is still potentially useful, but all the support for runtime twiddling of the emulation modes becomes useless. For parallel reasons, the SVE sysctl stuff is protected by #ifdef CONFIG_SYSCTL, which is why I didn't get a similar splat there. So, although this patch is superficially sensible, it may tend to hide bugs: code that calls register_sysctl() when CONFIG_SYSCTL=n is suspicious and probably needs review... no? Cheers ---Dave > This adds a inline function like we already have for register_sysctl_paths() > and register_sysctl_table(). > > Fixes: 38b9aeb32fa7 ("arm64: Port deprecated instruction emulation to new sysctl interface") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > This should probably get merged through the arm64 tree to avoid > bisection problems. > --- > include/linux/sysctl.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index fabe15e51c56..992bc9948232 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -216,6 +216,11 @@ static inline struct ctl_table_header *register_sysctl_paths( > return NULL; > } > > +static inline struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table) > +{ > + return NULL; > +} > + > static inline void unregister_sysctl_table(struct ctl_table_header * table) > { > } > -- > 2.9.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sysctl: add register_sysctl() dummy helper 2017-11-06 16:50 ` Dave Martin @ 2017-11-06 17:12 ` Arnd Bergmann 2017-11-06 17:41 ` Dave Martin 0 siblings, 1 reply; 7+ messages in thread From: Arnd Bergmann @ 2017-11-06 17:12 UTC (permalink / raw) To: Dave Martin Cc: Catalin Marinas, Will Deacon, Luis R. Rodriguez, Kees Cook, Alex Bennée, Eric W. Biederman, Andrew Morton, Thomas Gleixner, linux-kernel, linux-fsdevel On Mon, Nov 6, 2017 at 5:50 PM, Dave Martin <Dave.Martin@arm.com> wrote: > On Mon, Nov 06, 2017 at 01:36:40PM +0000, Arnd Bergmann wrote: >> The register_sysctl() function has been around for five years with commit >> fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") >> but now that arm64 started using it, I ran into a compile error: >> >> arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': >> arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' > > Hmmm, looks like I missed this combination in testing. > > I wonder whether ARMV8_DEPRECATED without SYSCTL is really a good idea > though: in that config, we build a lot of dead code and leak some > memory today. The default emulation is still potentially useful, but > all the support for runtime twiddling of the emulation modes becomes > useless. > > For parallel reasons, the SVE sysctl stuff is protected by #ifdef > CONFIG_SYSCTL, which is why I didn't get a similar splat there. > > > So, although this patch is superficially sensible, it may tend to hide > bugs: code that calls register_sysctl() when CONFIG_SYSCTL=n is > suspicious and probably needs review... no? I think your analysis for this code is correct, we waste a lot of memory if we do it like this. However in the general case where we register a statically allocated 'struct ctl_table', it wouldn't be an issue, because gcc could then eliminate all the dead code. Adding a CONFIG_SYSCTL #ifdef or Kconfig dependency would probably be reasonable for armv8_deprecated.c, for the rest of the kernel, having that wrapper is probably better. I don't really care how this gets fixed, as long as some solution gets merged. Arnd ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sysctl: add register_sysctl() dummy helper 2017-11-06 17:12 ` Arnd Bergmann @ 2017-11-06 17:41 ` Dave Martin 2017-11-06 20:36 ` Arnd Bergmann 0 siblings, 1 reply; 7+ messages in thread From: Dave Martin @ 2017-11-06 17:41 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, Will Deacon, Luis R. Rodriguez, Kees Cook, Alex Bennée, Eric W. Biederman, Andrew Morton, Thomas Gleixner, linux-kernel, linux-fsdevel On Mon, Nov 06, 2017 at 05:12:03PM +0000, Arnd Bergmann wrote: > On Mon, Nov 6, 2017 at 5:50 PM, Dave Martin <Dave.Martin@arm.com> wrote: > > On Mon, Nov 06, 2017 at 01:36:40PM +0000, Arnd Bergmann wrote: > >> The register_sysctl() function has been around for five years with commit > >> fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") > >> but now that arm64 started using it, I ran into a compile error: > >> > >> arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': > >> arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' > > > > Hmmm, looks like I missed this combination in testing. > > > > I wonder whether ARMV8_DEPRECATED without SYSCTL is really a good idea > > though: in that config, we build a lot of dead code and leak some > > memory today. The default emulation is still potentially useful, but > > all the support for runtime twiddling of the emulation modes becomes > > useless. > > > > For parallel reasons, the SVE sysctl stuff is protected by #ifdef > > CONFIG_SYSCTL, which is why I didn't get a similar splat there. > > > > > > So, although this patch is superficially sensible, it may tend to hide > > bugs: code that calls register_sysctl() when CONFIG_SYSCTL=n is > > suspicious and probably needs review... no? > > I think your analysis for this code is correct, we waste a lot of memory > if we do it like this. However in the general case where we register a > statically allocated 'struct ctl_table', it wouldn't be an issue, because > gcc could then eliminate all the dead code. > > Adding a CONFIG_SYSCTL #ifdef or Kconfig dependency would > probably be reasonable for armv8_deprecated.c, for the rest of > the kernel, having that wrapper is probably better. > > I don't really care how this gets fixed, as long as some solution gets > merged. Well, since your patch makes things more consistent, I'm happy to Reviewed-by: Dave Martin <Dave.Martin@arm.com> The armv8_deprecated code looks suspicious in any case, so I'll propose a fix for that separately -- adding a Kconfig dependency seems simplest. Cheers ---Dave ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sysctl: add register_sysctl() dummy helper 2017-11-06 17:41 ` Dave Martin @ 2017-11-06 20:36 ` Arnd Bergmann 0 siblings, 0 replies; 7+ messages in thread From: Arnd Bergmann @ 2017-11-06 20:36 UTC (permalink / raw) To: Dave Martin Cc: Catalin Marinas, Will Deacon, Luis R. Rodriguez, Kees Cook, Alex Bennée, Eric W. Biederman, Andrew Morton, Thomas Gleixner, linux-kernel, linux-fsdevel On Mon, Nov 6, 2017 at 6:41 PM, Dave Martin <Dave.Martin@arm.com> wrote: > On Mon, Nov 06, 2017 at 05:12:03PM +0000, Arnd Bergmann wrote: >> On Mon, Nov 6, 2017 at 5:50 PM, Dave Martin <Dave.Martin@arm.com> wrote: >> > On Mon, Nov 06, 2017 at 01:36:40PM +0000, Arnd Bergmann wrote: >> >> The register_sysctl() function has been around for five years with commit >> >> fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") >> >> but now that arm64 started using it, I ran into a compile error: >> >> >> >> arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': >> >> arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' >> > >> > Hmmm, looks like I missed this combination in testing. >> > >> > I wonder whether ARMV8_DEPRECATED without SYSCTL is really a good idea >> > though: in that config, we build a lot of dead code and leak some >> > memory today. The default emulation is still potentially useful, but >> > all the support for runtime twiddling of the emulation modes becomes >> > useless. >> > >> > For parallel reasons, the SVE sysctl stuff is protected by #ifdef >> > CONFIG_SYSCTL, which is why I didn't get a similar splat there. >> > >> > >> > So, although this patch is superficially sensible, it may tend to hide >> > bugs: code that calls register_sysctl() when CONFIG_SYSCTL=n is >> > suspicious and probably needs review... no? >> >> I think your analysis for this code is correct, we waste a lot of memory >> if we do it like this. However in the general case where we register a >> statically allocated 'struct ctl_table', it wouldn't be an issue, because >> gcc could then eliminate all the dead code. >> >> Adding a CONFIG_SYSCTL #ifdef or Kconfig dependency would >> probably be reasonable for armv8_deprecated.c, for the rest of >> the kernel, having that wrapper is probably better. >> >> I don't really care how this gets fixed, as long as some solution gets >> merged. > > Well, since your patch makes things more consistent, I'm happy to > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> > > > The armv8_deprecated code looks suspicious in any case, so I'll propose > a fix for that separately -- adding a Kconfig dependency seems simplest. Thanks, your other patch looks good too (replied with an Ack there). Andrew, can you pick up my patch into -mm then? It seems we still want it anyway, but there is no longer a dependency with the arm64 tree, so no reason to pick merge it there. Arnd ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sysctl: add register_sysctl() dummy helper 2017-11-06 13:36 [PATCH] sysctl: add register_sysctl() dummy helper Arnd Bergmann 2017-11-06 16:50 ` Dave Martin @ 2017-11-06 16:51 ` Kees Cook 2017-11-07 1:01 ` Will Deacon 2 siblings, 0 replies; 7+ messages in thread From: Kees Cook @ 2017-11-06 16:51 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, Will Deacon, Luis R. Rodriguez, Alex Bennée, Dave Martin, Eric W. Biederman, Andrew Morton, Thomas Gleixner, LKML, linux-fsdevel On Mon, Nov 6, 2017 at 5:36 AM, Arnd Bergmann <arnd@arndb.de> wrote: > The register_sysctl() function has been around for five years with commit > fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") > but now that arm64 started using it, I ran into a compile error: > > arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': > arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' > > This adds a inline function like we already have for register_sysctl_paths() > and register_sysctl_table(). > > Fixes: 38b9aeb32fa7 ("arm64: Port deprecated instruction emulation to new sysctl interface") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > This should probably get merged through the arm64 tree to avoid > bisection problems. Sure, that sounds fine. Acked-by: Kees Cook <keescook@chromium.org> Thanks! -Kees > --- > include/linux/sysctl.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index fabe15e51c56..992bc9948232 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -216,6 +216,11 @@ static inline struct ctl_table_header *register_sysctl_paths( > return NULL; > } > > +static inline struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table) > +{ > + return NULL; > +} > + > static inline void unregister_sysctl_table(struct ctl_table_header * table) > { > } > -- > 2.9.0 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sysctl: add register_sysctl() dummy helper 2017-11-06 13:36 [PATCH] sysctl: add register_sysctl() dummy helper Arnd Bergmann 2017-11-06 16:50 ` Dave Martin 2017-11-06 16:51 ` Kees Cook @ 2017-11-07 1:01 ` Will Deacon 2 siblings, 0 replies; 7+ messages in thread From: Will Deacon @ 2017-11-07 1:01 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, Luis R. Rodriguez, Kees Cook, Alex Bennée, Dave Martin, Eric W. Biederman, Andrew Morton, Thomas Gleixner, linux-kernel, linux-fsdevel On Mon, Nov 06, 2017 at 02:36:40PM +0100, Arnd Bergmann wrote: > The register_sysctl() function has been around for five years with commit > fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users") > but now that arm64 started using it, I ran into a compile error: > > arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl': > arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl' > > This adds a inline function like we already have for register_sysctl_paths() > and register_sysctl_table(). > > Fixes: 38b9aeb32fa7 ("arm64: Port deprecated instruction emulation to new sysctl interface") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > This should probably get merged through the arm64 tree to avoid > bisection problems. > --- > include/linux/sysctl.h | 5 +++++ > 1 file changed, 5 insertions(+) Assuming Andrew can pick this up, so: Acked-by: Will Deacon <will.deacon@arm.com> Will > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index fabe15e51c56..992bc9948232 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -216,6 +216,11 @@ static inline struct ctl_table_header *register_sysctl_paths( > return NULL; > } > > +static inline struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table) > +{ > + return NULL; > +} > + > static inline void unregister_sysctl_table(struct ctl_table_header * table) > { > } > -- > 2.9.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-07 1:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-06 13:36 [PATCH] sysctl: add register_sysctl() dummy helper Arnd Bergmann 2017-11-06 16:50 ` Dave Martin 2017-11-06 17:12 ` Arnd Bergmann 2017-11-06 17:41 ` Dave Martin 2017-11-06 20:36 ` Arnd Bergmann 2017-11-06 16:51 ` Kees Cook 2017-11-07 1:01 ` Will Deacon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.