[PATCH] sysctl: add register_sysctl() dummy helper

[PATCH] sysctl: add register_sysctl() dummy helper

[Arnd Bergmann]

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

Re: [PATCH] sysctl: add register_sysctl() dummy helper

[Dave Martin]

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

Re: [PATCH] sysctl: add register_sysctl() dummy helper

[Kees Cook]

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

Re: [PATCH] sysctl: add register_sysctl() dummy helper

[Arnd Bergmann]

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

Re: [PATCH] sysctl: add register_sysctl() dummy helper

[Dave Martin]

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

Re: [PATCH] sysctl: add register_sysctl() dummy helper

[Arnd Bergmann]

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

Re: [PATCH] sysctl: add register_sysctl() dummy helper

[Will Deacon]

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
>