All of lore.kernel.org
 help / color / mirror / Atom feed
* [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	[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 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 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: 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.