Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 3/4] uaccess: Check no rescheduling function is called
 in unsafe region
To:     Valentin Schneider <valentin.schneider@arm.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Cc:     mingo@redhat.com, peterz@infradead.org, catalin.marinas@arm.com,
        will.deacon@arm.com, james.morse@arm.com, hpa@zytor.com
References: <1547560709-56207-1-git-send-email-julien.thierry@arm.com>
 <1547560709-56207-4-git-send-email-julien.thierry@arm.com>
 <9ba3c7a7-80a8-da29-ffb7-3841ea8548b5@arm.com>
From:   Julien Thierry <julien.thierry@arm.com>
Message-ID: <f2e6ea55-40fa-6d4d-3262-6f6b3791c9d4@arm.com>
Date:   Mon, 4 Feb 2019 13:27:17 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <9ba3c7a7-80a8-da29-ffb7-3841ea8548b5@arm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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