Re: twist: allow disabling reboot request

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

On 2020/06/03 21:44, Petr Mladek wrote:
> On Wed 2020-06-03 20:03:28, Tetsuo Handa wrote:
>> On 2020/05/29 22:26, Tetsuo Handa wrote:
>>>     By the way, I do worry that people forget to perform these steps when they do
>>>     their tests without asking syzbot...
>>
>> Here is a draft of boot-time switching. Since kconfig can handle string variable up to
>> 2048 characters, we could hold the content of the "your-config" file inside .config file
>> in order to avoid relying on external file in "syzkaller tree". But since only one kconfig
>> option is used, basically the way to temporarily include/exclude specific options (under
>> automated testing by syzbot) seems to remain directly patching apply_twist_flags(), for
>> https://github.com/google/syzkaller/blob/master/dashboard/config/util.sh will automatically
>> overwrite CONFIG_DEFAULT_TWIST_FLAGS settings. If each twist flag were using independent
>> kconfig option, the way to temporarily include/exclude specific options will become directly
>> patching Kconfig file.
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 82d91547d122..78fdbb4f17b1 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -1038,4 +1038,12 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>>  	 /* OTHER_WRITABLE?  Generally considered a bad idea. */		\
>>  	 BUILD_BUG_ON_ZERO((perms) & 2) +					\
>>  	 (perms))
>> +
>> +/* Flags for twisting kernel behavior. */
>> +struct twist_flags {
>> +	bool disable_kbd_k_spec_handler;
>> +	bool disable_reboot_request;
>> +};
>> +extern struct twist_flags twist_flags;
> 
> 
> Why all these options have to be in a single structure?

There will be many options (maybe some dozens).
Do we really want to expose so many options individually?

(If these options were build-time configuration, we won't need
this structure at all.)

> 
> 
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index 498d344ea53a..41cfabc74ad7 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -2338,4 +2338,9 @@ config HYPERV_TESTING
>>  
>>  endmenu # "Kernel Testing and Coverage"
>>  
>> +menuconfig DEFAULT_TWIST_FLAGS
>> +	string "Default twist options (DANGEROUS)"
>> +	help
>> +	  Don't specify anything unless you know what you are doing.
>> +
>>  endmenu # Kernel hacking
> 
> Why such a crazy build configure option?
> 
> I think that the only way to get this upstream is to:
> 
>    + Add separate boot options that might theoretically be used also
>      by other people.

Like you said

  I am afraid that many of them could not be normal options. They change or
  break some behavior that is necessary by seriously used system.

these options are meant for helping fuzzers to find bugs while protecting
the kernel from legitimate-but-stupid requests from fuzzers. Other people
can include projects other than syzbot, but basically only useful for
debugging projects. (And making these options boot-time configuration
increases garbage/overhead for non-debugging usage.)

> 
>    + Use boot parameters and not build configuration.

That sounds like a very tight restriction for syzbot. Relying on external
files breaks reproducibility; people can fail to specify intended options.
Saving intended options into the .config file is the most robust/reliable
approach.

> 
>    + Avoid the meaningless word "twist" !!!

Then, what do you suggest? I think that we need some keyword for grouping.
https://lkml.kernel.org/r/41a49d42-7119-62b9-085b-aa99cadc4dd1@i-love.sakura.ne.jp

> 
> 
> Best Regards,
> Petr
>