Re: [PATCH v3 3/4] uaccess: Check no rescheduling function is called in unsafe region

Julien Thierry
2019-02-04T13:27:27Z

On 30/01/2019 16:58, Valentin Schneider wrote:
> On 15/01/2019 13:58, Julien Thierry wrote:
> [...]> @@ -6151,6 +6159,20 @@ void ___might_sleep(const char *file, int line, int preempt_offset)
>> EXPORT_SYMBOL(___might_sleep);
>> #endif
>>
>> +#ifdef CONFIG_DEBUG_UACCESS_SLEEP
>> +void __might_resched(const char *file, int line)
>> +{
>> + if (!unsafe_user_region_active())
>> + return;
>> +
>> + printk(KERN_ERR
>> + "BUG: rescheduling function called from user access context at %s:%d\n",
>> + file, line);
>> + dump_stack();
>
> Since I've been staring intensely at ___might_sleep() lately, I was thinking
> we could "copy" it a bit more closely (sorry for going back on what I said
> earlier).
>
> Coming back to the double warnings (__might_resched() + schedule_debug()),
> it might be better to drop the schedule_debug() warning and just have the
> one in __might_resched() - if something goes wrong, there'll already be a
> "BUG" in the log.
>

My only worry with that approach is that if someone has a function that
does resched but does not include the annotation __might_resched() we'd
miss the fact that something went wrong.

But that might be a lot of "if"s since that assumes that something does
go wrong.

Could I have a maintainers opinion on this to know if I respin it?

>> +}
>> +EXPORT_SYMBOL(__might_resched);
>> +#endif
>> +
>> #ifdef CONFIG_MAGIC_SYSRQ
>> void normalize_rt_tasks(void)
>> {
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index d4df5b2..d030e31 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -2069,6 +2069,14 @@ config IO_STRICT_DEVMEM
>>
>> If in doubt, say Y.
>>
>> +config DEBUG_UACCESS_SLEEP
>> + bool "Check sleep inside a user access region"
>> + depends on DEBUG_KERNEL
>> + help
>> + If you say Y here, various routines which may sleep will become very
>> + noisy if they are called inside a user access region (i.e. between
>> + a user_access_begin() and a user_access_end())
>
> If it does get noisy, we should go for some ratelimiting - it's probably
> good practice even if it is not noisy actually.
>
> ___might_sleep() has this:
>
> if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
> return;
> prev_jiffy = jiffies;
>

I guess the noisiness could depend on the arch specific handling of user
accesses. So yes I guess it might be a good idea to add this.

Thanks,

--
Julien Thierry