Re: [PATCH v2] panic: setup panic_timeout early

[Levente Kurusa <levex@linux.com>]

2013-11-14 23:32 keltezéssel, Felipe Contreras írta:
> Hi,
> 
> Sending again since the previous one was rejected by the server.
> 
> On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Levente Kurusa wrote:
>>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>>>> On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>>>>
>>>>> * Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>>>>
>>>>>> Otherwise we might not reboot when the user needs it the most (early
>>>>>> on).
>>>>>>
>>>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>>>> ---
>>>>>>
>>>>> [...]
>>>>>>
>>>>>> diff --git a/kernel/panic.c b/kernel/panic.c
>>>>>> index b6c482c..d865263 100644
>>>>>> --- a/kernel/panic.c
>>>>>> +++ b/kernel/panic.c
>>>>>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>>>>>
>>>>>>  #endif
>>>>>>
>>>>>> -core_param(panic, panic_timeout, int, 0644);
>>>>>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>>>>>
>>>>>> +static int __init set_panic_timeout(char *val)
>>>>>> +{
>>>>>> +     long timeout;
>>>>>> +     int ret;
>>>>>> +
>>>>>> +     ret = kstrtol(val, 0, &timeout);
>>>>>> +     if (ret < 0)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     panic_timeout = timeout;
>>>>>> +     return 0;
>>>>>> +}
>>>>>
>>>>> I think the type of the 'timeout' local variable should match the type of
>>>>> 'panic_timeout' (which is 'int', not 'long').
>>>>
>>>> So you would rather have this?
>>>>
>>>>   kstrtol(val, 0, (long *)&timeout);
>>>>
>>>> Couldn't that potentially write the value beyond the memory allocated
>>>> to 'timeout'?
>>>>
>>>
>>> No, 'panic_timeout' is a variable of type 'int'.
>>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>>
>> Oh really? Something like this?
>>
>>   --- a/kernel/panic.c
>>   +++ b/kernel/panic.c
>>   @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>>    static int __init set_panic_timeout(char *val)
>>    {
>>   -       long timeout;
>>   +       int timeout;
>>           int ret;
>>
>>           ret = kstrtol(val, 0, &timeout);
>>
>> And then what happens?
>>
>>   kernel/panic.c: In function ‘set_panic_timeout’:
>>   kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from incompatible pointer type [enabled by default]
>>     ret = kstrtol(val, 0, &timeout);
>>     ^
>>   In file included from include/linux/debug_locks.h:4:0,
>>                    from kernel/panic.c:11:
>>   include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is of type ‘int *’
>>    static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
>>
>> To fix that warning you need this:
>>                                                                                                  ^
>>   --- a/kernel/panic.c
>>   +++ b/kernel/panic.c
>>   @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>>    static int __init set_panic_timeout(char *val)
>>    {
>>   -       long timeout;
>>   +       int timeout;
>>           int ret;
>>
>>   -       ret = kstrtol(val, 0, &timeout);
>>   +       ret = kstrtol(val, 0, (long *)&timeout);
>>           if (ret < 0)
>>                   return ret;
>>
>>
>> Which is the only logical conclusion I arrived to. What else do you suggest to
>> fix the problem that kstrtol() expects a long? And since this fix is not what
>> we want because we would be writing to the wrong memory, we don't want 'timeout'
>> to be int.
>>
>> Therefore 'timeout' should be a long. How is that not clear?
>>
>> You can even see that it's already done this way for parameters:
>>
>>   STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
>>
>>   #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)             \
>>           int param_set_##name(const char *val, const struct kernel_param *kp) \
>>           {                                                             \
>>                   tmptype l;                                            \
>>                   int ret;                                              \
>>                                                                           \
>>                   ret = strtolfn(val, 0, &l);                           \
>>                   if (ret < 0 || ((type)l != l))                                \
>>                           return ret < 0 ? ret : -EINVAL;                       \
>>                   *((type *)kp->arg) = l;                                       \
>>                   return 0;                                             \
>>           }                                                             \
>>
>> So yes, we obviously want the temporary variable 'timeout' to be a long, even
>> though the final destination is an int.
> 

Hi,

No, you don't want timeout to be a long, and instead of kstrtol, you should use
kstrtoint(char *s, int base, int *res)
which is defined in lib/ktstrtox.c

Also, a bit of advice: Calm down. If you continue to act childish nobody will take
you seriously. Before acting like you are the king here, please at least take the time
to research other options.

-- 
Regards,
Levente Kurusa