| Message ID | 20171106133700.558647-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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
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 >
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) { }
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(+)