2018-07-12 13:09:03

by Adrian Reber

[permalink] [raw]
Subject: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
distribution kernels and also part of the defconfigs of various
architectures.

To make it easier for distributions to enable CHECKPOINT_RESTORE this
removes EXPERT and moves the configuration option out of the EXPERT
block.

Signed-off-by: Adrian Reber <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Andrei Vagin <[email protected]>
Cc: Hendrik Brueckner <[email protected]>
---
init/Kconfig | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 041f3a022122..9c529c763326 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -932,6 +932,18 @@ config NET_NS

endif # NAMESPACES

+config CHECKPOINT_RESTORE
+ bool "Checkpoint/restore support"
+ select PROC_CHILDREN
+ default n
+ help
+ Enables additional kernel features in a sake of checkpoint/restore.
+ In particular it adds auxiliary prctl codes to setup process text,
+ data and heap segment sizes, and a few additional /proc filesystem
+ entries.
+
+ If unsure, say N here.
+
config SCHED_AUTOGROUP
bool "Automatic process group scheduling"
select CGROUPS
@@ -1348,18 +1360,6 @@ config MEMBARRIER

If unsure, say Y.

-config CHECKPOINT_RESTORE
- bool "Checkpoint/restore support" if EXPERT
- select PROC_CHILDREN
- default n
- help
- Enables additional kernel features in a sake of checkpoint/restore.
- In particular it adds auxiliary prctl codes to setup process text,
- data and heap segment sizes, and a few additional /proc filesystem
- entries.
-
- If unsure, say N here.
-
config KALLSYMS
bool "Load all symbols for debugging/ksymoops" if EXPERT
default y
--
2.17.1



2018-07-12 13:48:01

by Oleg Nesterov

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On 07/12, Adrian Reber wrote:
>
> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> distribution kernels and also part of the defconfigs of various
> architectures.
>
> To make it easier for distributions to enable CHECKPOINT_RESTORE this
> removes EXPERT and moves the configuration option out of the EXPERT
> block.

Agreed.

Acked-by: Oleg Nesterov <[email protected]>


2018-07-12 13:50:24

by Hendrik Brueckner

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Thu, Jul 12, 2018 at 03:07:33PM +0200, Adrian Reber wrote:
> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> distribution kernels and also part of the defconfigs of various
> architectures.
>
> To make it easier for distributions to enable CHECKPOINT_RESTORE this
> removes EXPERT and moves the configuration option out of the EXPERT
> block.
>
> Signed-off-by: Adrian Reber <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Andrei Vagin <[email protected]>
> Cc: Hendrik Brueckner <[email protected]>
> ---
> init/Kconfig | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 041f3a022122..9c529c763326 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -932,6 +932,18 @@ config NET_NS
>
> endif # NAMESPACES
>
> +config CHECKPOINT_RESTORE
> + bool "Checkpoint/restore support"
> + select PROC_CHILDREN
> + default n
> + help
> + Enables additional kernel features in a sake of checkpoint/restore.
> + In particular it adds auxiliary prctl codes to setup process text,
> + data and heap segment sizes, and a few additional /proc filesystem
> + entries.
> +
> + If unsure, say N here.
> +
> config SCHED_AUTOGROUP
> bool "Automatic process group scheduling"
> select CGROUPS
> @@ -1348,18 +1360,6 @@ config MEMBARRIER
>
> If unsure, say Y.
>
> -config CHECKPOINT_RESTORE
> - bool "Checkpoint/restore support" if EXPERT
> - select PROC_CHILDREN
> - default n
> - help
> - Enables additional kernel features in a sake of checkpoint/restore.
> - In particular it adds auxiliary prctl codes to setup process text,
> - data and heap segment sizes, and a few additional /proc filesystem
> - entries.
> -
> - If unsure, say N here.
> -
> config KALLSYMS
> bool "Load all symbols for debugging/ksymoops" if EXPERT
> default y

Thanks!

Reviewed-by: Hendrik Brueckner <[email protected]>


2018-07-12 13:53:31

by Alice Frosi

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE



On 12.07.2018 15:47, Hendrik Brueckner wrote:
> On Thu, Jul 12, 2018 at 03:07:33PM +0200, Adrian Reber wrote:
>> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
>> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
>> distribution kernels and also part of the defconfigs of various
>> architectures.
>>
>> To make it easier for distributions to enable CHECKPOINT_RESTORE this
>> removes EXPERT and moves the configuration option out of the EXPERT
>> block.
>>
>> Signed-off-by: Adrian Reber <[email protected]>
>> Cc: Oleg Nesterov <[email protected]>
>> Cc: Pavel Emelyanov <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Eric W. Biederman <[email protected]>
>> Cc: Andrei Vagin <[email protected]>
>> Cc: Hendrik Brueckner <[email protected]>
>> ---
>> init/Kconfig | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 041f3a022122..9c529c763326 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -932,6 +932,18 @@ config NET_NS
>>
>> endif # NAMESPACES
>>
>> +config CHECKPOINT_RESTORE
>> + bool "Checkpoint/restore support"
>> + select PROC_CHILDREN
>> + default n
>> + help
>> + Enables additional kernel features in a sake of checkpoint/restore.
>> + In particular it adds auxiliary prctl codes to setup process text,
>> + data and heap segment sizes, and a few additional /proc filesystem
>> + entries.
>> +
>> + If unsure, say N here.
>> +
>> config SCHED_AUTOGROUP
>> bool "Automatic process group scheduling"
>> select CGROUPS
>> @@ -1348,18 +1360,6 @@ config MEMBARRIER
>>
>> If unsure, say Y.
>>
>> -config CHECKPOINT_RESTORE
>> - bool "Checkpoint/restore support" if EXPERT
>> - select PROC_CHILDREN
>> - default n
>> - help
>> - Enables additional kernel features in a sake of checkpoint/restore.
>> - In particular it adds auxiliary prctl codes to setup process text,
>> - data and heap segment sizes, and a few additional /proc filesystem
>> - entries.
>> -
>> - If unsure, say N here.
>> -
>> config KALLSYMS
>> bool "Load all symbols for debugging/ksymoops" if EXPERT
>> default y
> Thanks!
>
> Reviewed-by: Hendrik Brueckner <[email protected]>

Reviewed-by: Alice Frosi <[email protected]>


2018-07-12 16:34:48

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE


Adrian Reber <[email protected]> writes:

> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> distribution kernels and also part of the defconfigs of various
> architectures.
>
> To make it easier for distributions to enable CHECKPOINT_RESTORE this
> removes EXPERT and moves the configuration option out of the EXPERT
> block.

I think we should change the help text at the same time, to match
our improve understanding of the situation.

Does anyone remember why this option was added at all?
Why this option was placed under expert?

What is the value of disabling this functionality ever?

Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
entirely?

Certainly we are at a point where distro's are enabling this so hiding
it behind CONFIG_EXPERT with a default of N seems inapparopriate.

The only thing I can imagine might be sensible is changing the default
to Y and leaving it behind CONFIG_EXPERT.

I want to know what is the point of maintaining all of the complexity of
the ifdefs. If no one can come up with a reason I say let's just remove
CONFIG_CHECKPOINT_RESTORE entirely.

Eric


> Signed-off-by: Adrian Reber <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Andrei Vagin <[email protected]>
> Cc: Hendrik Brueckner <[email protected]>
> ---
> init/Kconfig | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 041f3a022122..9c529c763326 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -932,6 +932,18 @@ config NET_NS
>
> endif # NAMESPACES
>
> +config CHECKPOINT_RESTORE
> + bool "Checkpoint/restore support"
> + select PROC_CHILDREN
> + default n
> + help
> + Enables additional kernel features in a sake of checkpoint/restore.
> + In particular it adds auxiliary prctl codes to setup process text,
> + data and heap segment sizes, and a few additional /proc filesystem
> + entries.
> +
> + If unsure, say N here.
> +
> config SCHED_AUTOGROUP
> bool "Automatic process group scheduling"
> select CGROUPS
> @@ -1348,18 +1360,6 @@ config MEMBARRIER
>
> If unsure, say Y.
>
> -config CHECKPOINT_RESTORE
> - bool "Checkpoint/restore support" if EXPERT
> - select PROC_CHILDREN
> - default n
> - help
> - Enables additional kernel features in a sake of checkpoint/restore.
> - In particular it adds auxiliary prctl codes to setup process text,
> - data and heap segment sizes, and a few additional /proc filesystem
> - entries.
> -
> - If unsure, say N here.
> -
> config KALLSYMS
> bool "Load all symbols for debugging/ksymoops" if EXPERT
> default y

2018-07-13 08:24:22

by Pavel Emelianov

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On 07/12/2018 04:07 PM, Adrian Reber wrote:
> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> distribution kernels and also part of the defconfigs of various
> architectures.
>
> To make it easier for distributions to enable CHECKPOINT_RESTORE this
> removes EXPERT and moves the configuration option out of the EXPERT
> block.
>
> Signed-off-by: Adrian Reber <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Andrei Vagin <[email protected]>
> Cc: Hendrik Brueckner <[email protected]>

Acked-by: Pavel Emelyanov <[email protected]>

> ---
> init/Kconfig | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 041f3a022122..9c529c763326 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -932,6 +932,18 @@ config NET_NS
>
> endif # NAMESPACES
>
> +config CHECKPOINT_RESTORE
> + bool "Checkpoint/restore support"
> + select PROC_CHILDREN
> + default n
> + help
> + Enables additional kernel features in a sake of checkpoint/restore.
> + In particular it adds auxiliary prctl codes to setup process text,
> + data and heap segment sizes, and a few additional /proc filesystem
> + entries.
> +
> + If unsure, say N here.
> +
> config SCHED_AUTOGROUP
> bool "Automatic process group scheduling"
> select CGROUPS
> @@ -1348,18 +1360,6 @@ config MEMBARRIER
>
> If unsure, say Y.
>
> -config CHECKPOINT_RESTORE
> - bool "Checkpoint/restore support" if EXPERT
> - select PROC_CHILDREN
> - default n
> - help
> - Enables additional kernel features in a sake of checkpoint/restore.
> - In particular it adds auxiliary prctl codes to setup process text,
> - data and heap segment sizes, and a few additional /proc filesystem
> - entries.
> -
> - If unsure, say N here.
> -
> config KALLSYMS
> bool "Load all symbols for debugging/ksymoops" if EXPERT
> default y
>


2018-07-13 08:36:49

by Pavel Emelianov

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On 07/12/2018 07:33 PM, Eric W. Biederman wrote:
>
> Adrian Reber <[email protected]> writes:
>
>> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
>> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
>> distribution kernels and also part of the defconfigs of various
>> architectures.
>>
>> To make it easier for distributions to enable CHECKPOINT_RESTORE this
>> removes EXPERT and moves the configuration option out of the EXPERT
>> block.
>
> I think we should change the help text at the same time, to match
> our improve understanding of the situation.
>
> Does anyone remember why this option was added at all?

Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail:

However I'm less confident than the developers that it will all
eventually work! So what I'm asking them to do is to wrap each piece
of new code inside CONFIG_CHECKPOINT_RESTORE. So if it all
eventually comes to tears and the project as a whole fails, it should
be a simple matter to go through and delete all trace of it.

the best link with full e-mail I googled for is
https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734

-- Pavel

> Why this option was placed under expert?
>
> What is the value of disabling this functionality ever?
>
> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
> entirely?
>
> Certainly we are at a point where distro's are enabling this so hiding
> it behind CONFIG_EXPERT with a default of N seems inapparopriate.
>
> The only thing I can imagine might be sensible is changing the default
> to Y and leaving it behind CONFIG_EXPERT.
>
> I want to know what is the point of maintaining all of the complexity of
> the ifdefs. If no one can come up with a reason I say let's just remove
> CONFIG_CHECKPOINT_RESTORE entirely.
>
> Eric
>
>
>> Signed-off-by: Adrian Reber <[email protected]>
>> Cc: Oleg Nesterov <[email protected]>
>> Cc: Pavel Emelyanov <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Eric W. Biederman <[email protected]>
>> Cc: Andrei Vagin <[email protected]>
>> Cc: Hendrik Brueckner <[email protected]>
>> ---
>> init/Kconfig | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 041f3a022122..9c529c763326 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -932,6 +932,18 @@ config NET_NS
>>
>> endif # NAMESPACES
>>
>> +config CHECKPOINT_RESTORE
>> + bool "Checkpoint/restore support"
>> + select PROC_CHILDREN
>> + default n
>> + help
>> + Enables additional kernel features in a sake of checkpoint/restore.
>> + In particular it adds auxiliary prctl codes to setup process text,
>> + data and heap segment sizes, and a few additional /proc filesystem
>> + entries.
>> +
>> + If unsure, say N here.
>> +
>> config SCHED_AUTOGROUP
>> bool "Automatic process group scheduling"
>> select CGROUPS
>> @@ -1348,18 +1360,6 @@ config MEMBARRIER
>>
>> If unsure, say Y.
>>
>> -config CHECKPOINT_RESTORE
>> - bool "Checkpoint/restore support" if EXPERT
>> - select PROC_CHILDREN
>> - default n
>> - help
>> - Enables additional kernel features in a sake of checkpoint/restore.
>> - In particular it adds auxiliary prctl codes to setup process text,
>> - data and heap segment sizes, and a few additional /proc filesystem
>> - entries.
>> -
>> - If unsure, say N here.
>> -
>> config KALLSYMS
>> bool "Load all symbols for debugging/ksymoops" if EXPERT
>> default y
> .
>


2018-07-13 13:47:40

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

Pavel Emelyanov <[email protected]> writes:

> On 07/12/2018 07:33 PM, Eric W. Biederman wrote:
>>
>> Adrian Reber <[email protected]> writes:
>>
>>> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
>>> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
>>> distribution kernels and also part of the defconfigs of various
>>> architectures.
>>>
>>> To make it easier for distributions to enable CHECKPOINT_RESTORE this
>>> removes EXPERT and moves the configuration option out of the EXPERT
>>> block.
>>
>> I think we should change the help text at the same time, to match
>> our improve understanding of the situation.
>>
>> Does anyone remember why this option was added at all?
>
> Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail:
>
> However I'm less confident than the developers that it will all
> eventually work! So what I'm asking them to do is to wrap each piece
> of new code inside CONFIG_CHECKPOINT_RESTORE. So if it all
> eventually comes to tears and the project as a whole fails, it should
> be a simple matter to go through and delete all trace of it.
>
> the best link with full e-mail I googled for is
> https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734

Good explanation. Thank you.

At this point we even have not CRIU users of some of the pieces.
The project as a whole has not failed.

The code is old enough an common enough (enabled in some distros) that
we need to do the whole watch out for regressions if we remove any part
of it.

Which is a long way of saying the original justifiction for
CONFIG_CHECKPOINT_RESTORE is gone. So please let's remove the entire
config option and simplify everyone's lives who has to test this stuff.

Unless someone can come up with a justification for keeping some of this
behind a config option.

Eric

2018-07-13 14:21:24

by Adrian Reber

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Fri, Jul 13, 2018 at 08:46:25AM -0500, Eric W. Biederman wrote:
> Pavel Emelyanov <[email protected]> writes:
>
> > On 07/12/2018 07:33 PM, Eric W. Biederman wrote:
> >>
> >> Adrian Reber <[email protected]> writes:
> >>
> >>> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> >>> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> >>> distribution kernels and also part of the defconfigs of various
> >>> architectures.
> >>>
> >>> To make it easier for distributions to enable CHECKPOINT_RESTORE this
> >>> removes EXPERT and moves the configuration option out of the EXPERT
> >>> block.
> >>
> >> I think we should change the help text at the same time, to match
> >> our improve understanding of the situation.
> >>
> >> Does anyone remember why this option was added at all?
> >
> > Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail:
> >
> > However I'm less confident than the developers that it will all
> > eventually work! So what I'm asking them to do is to wrap each piece
> > of new code inside CONFIG_CHECKPOINT_RESTORE. So if it all
> > eventually comes to tears and the project as a whole fails, it should
> > be a simple matter to go through and delete all trace of it.
> >
> > the best link with full e-mail I googled for is
> > https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734
>
> Good explanation. Thank you.
>
> At this point we even have not CRIU users of some of the pieces.
> The project as a whole has not failed.
>
> The code is old enough an common enough (enabled in some distros) that
> we need to do the whole watch out for regressions if we remove any part
> of it.
>
> Which is a long way of saying the original justifiction for
> CONFIG_CHECKPOINT_RESTORE is gone. So please let's remove the entire
> config option and simplify everyone's lives who has to test this stuff.

Sounds good.

> Unless someone can come up with a justification for keeping some of this
> behind a config option.

I can provide a patch removing CONFIG_CHECKPOINT_RESTORE if there are no
further objections against it.

Adrian

2018-07-13 20:56:32

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Thu, 12 Jul 2018 11:33:33 -0500 [email protected] (Eric W. Biederman) wrote:

>
> Adrian Reber <[email protected]> writes:
>
> > The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> > combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> > distribution kernels and also part of the defconfigs of various
> > architectures.
> >
> > To make it easier for distributions to enable CHECKPOINT_RESTORE this
> > removes EXPERT and moves the configuration option out of the EXPERT
> > block.
>
> I think we should change the help text at the same time, to match
> our improve understanding of the situation.
>
> Does anyone remember why this option was added at all?

Because at the time it was quite unclear that the overall project would
produce a viable result. But the code is splattered over so many
places that getting it all going out-of-tree was impractical. So the
#ifdef CONFIG_CHECKPOINT_RESTORE markers were initially there to a)
identify places where we should go if we decide to rip the whole thing
out and to b) ensure that the kernel would still compile and work OK
after that removal.

This caution eventually proved to be unnecessary.

> Why this option was placed under expert?

Dunno.

> What is the value of disabling this functionality ever?
>
> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
> entirely?

For the vast number of Linux machines which aren't servers? Check out
some defconfigs - only one of arm's 119 defconfigs selects it.



2018-07-14 04:46:26

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Fri, Jul 13, 2018 at 1:55 PM, Andrew Morton
<[email protected]> wrote:
> On Thu, 12 Jul 2018 11:33:33 -0500 [email protected] (Eric W. Biederman) wrote:
>> What is the value of disabling this functionality ever?
>>
>> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
>> entirely?
>
> For the vast number of Linux machines which aren't servers? Check out
> some defconfigs - only one of arm's 119 defconfigs selects it.

Right, and I would bet the minification folks would like to keep it
out of their builds too. I think we should keep the config.

-Kees

--
Kees Cook
Pixel Security

2018-07-14 06:03:47

by Josh Triplett

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Fri, Jul 13, 2018 at 09:44:15PM -0700, Kees Cook wrote:
> On Fri, Jul 13, 2018 at 1:55 PM, Andrew Morton
> <[email protected]> wrote:
> > On Thu, 12 Jul 2018 11:33:33 -0500 [email protected] (Eric W. Biederman) wrote:
> >> What is the value of disabling this functionality ever?
> >>
> >> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
> >> entirely?
> >
> > For the vast number of Linux machines which aren't servers? Check out
> > some defconfigs - only one of arm's 119 defconfigs selects it.
>
> Right, and I would bet the minification folks would like to keep it
> out of their builds too. I think we should keep the config.

Thank you, Kees. Yes, please.

- Josh Triplett

2018-07-14 19:07:03

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

Kees Cook <[email protected]> writes:

> On Fri, Jul 13, 2018 at 1:55 PM, Andrew Morton
> <[email protected]> wrote:
>> On Thu, 12 Jul 2018 11:33:33 -0500 [email protected] (Eric W. Biederman) wrote:
>>> What is the value of disabling this functionality ever?
>>>
>>> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
>>> entirely?
>>
>> For the vast number of Linux machines which aren't servers? Check out
>> some defconfigs - only one of arm's 119 defconfigs selects it.
>
> Right, and I would bet the minification folks would like to keep it
> out of their builds too. I think we should keep the config.

I take it then you are volunteering to test with and without the config
option?

Even if the config option is kept I intend to rip it out every time I
wind up touching code with it in. Config options have a real cost in
testing and development.

For a config option that no one has come forward with an actual real
world use case for disabling, that cost seems much too high.

Eric


2018-07-14 19:40:55

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

Josh Triplett <[email protected]> writes:

> On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
>> For a config option that no one has come forward with an actual real
>> world use case for disabling, that cost seems much too high.
>
> The real-world use case is precisely as stated: code size, both storage
> and RAM.

That is theoretical. Which platform will break or feel distressed if we
make it unconditional. That is real world.

> I regularly encounter systems I'd *like* to put Linux in that have
> around 1MB of storage and 1MB of RAM, or even less.

Yes. There is so little code behind CONFIG_CHECKPOINT_RESTART that it
won't help with that.

But if minification is the actual requirement for disabling
CONFIG_CHECKPOINT_RESTART than CONFIG_CHECKPIONT_RESTART is properly
behind expert and it needs to be default y instead of default n.

Eric

2018-07-14 19:51:36

by Josh Triplett

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
> For a config option that no one has come forward with an actual real
> world use case for disabling, that cost seems much too high.

The real-world use case is precisely as stated: code size, both storage
and RAM.

I regularly encounter systems I'd *like* to put Linux in that have
around 1MB of storage and 1MB of RAM, or even less.

2018-07-14 20:18:33

by Josh Triplett

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Sat, Jul 14, 2018 at 02:39:24PM -0500, Eric W. Biederman wrote:
> Josh Triplett <[email protected]> writes:
>
> > On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
> >> For a config option that no one has come forward with an actual real
> >> world use case for disabling, that cost seems much too high.
> >
> > The real-world use case is precisely as stated: code size, both storage
> > and RAM.
>
> That is theoretical.

No, it isn't. I've *watched* the kernel's size trend steadily upward
over time. And it largely happens in individual features that don't
think *their* contribution to size is all that large.

> > I regularly encounter systems I'd *like* to put Linux in that have
> > around 1MB of storage and 1MB of RAM, or even less.
>
> Yes. There is so little code behind CONFIG_CHECKPOINT_RESTART that it
> won't help with that.

It adds up; there are hundreds more small features like it.

> But if minification is the actual requirement for disabling
> CONFIG_CHECKPOINT_RESTART than CONFIG_CHECKPIONT_RESTART is properly
> behind expert and it needs to be default y instead of default n.

I don't have any objection to *that*, as long as the option remains.

2018-07-14 20:20:48

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

On Sat, Jul 14, 2018 at 02:39:24PM -0500, Eric W. Biederman wrote:
> Josh Triplett <[email protected]> writes:
>
> > On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
> >> For a config option that no one has come forward with an actual real
> >> world use case for disabling, that cost seems much too high.
> >
> > The real-world use case is precisely as stated: code size, both storage
> > and RAM.
>
> That is theoretical. Which platform will break or feel distressed if we
> make it unconditional. That is real world.
>
> > I regularly encounter systems I'd *like* to put Linux in that have
> > around 1MB of storage and 1MB of RAM, or even less.
>
> Yes. There is so little code behind CONFIG_CHECKPOINT_RESTART that it
> won't help with that.
>
> But if minification is the actual requirement for disabling
> CONFIG_CHECKPOINT_RESTART than CONFIG_CHECKPIONT_RESTART is properly
> behind expert and it needs to be default y instead of default n.

I happened to miss this thread, sorry. So as far as I remember it
was me who introduced this option in first place, and initially
I placed it under expert so it won't be enabled by default. Lately
we found that some of functionality introduced for criu sake actually
pretty convenient for other tools (for example vmflags reported in
procfs) so we dropped CONFIG_ option out for such blocks and merged
them into kernel directly. I won't mind if left is merged into the
kernel, there should not be that many places.