linux-kernel - Re: [PATCH -next] mm: mark a intentional data race in page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Feb 2020 15:18:38 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Marco Elver <elver@...gle.com>, Qian Cai <cai@....pw>
CC:     Andrew Morton <akpm@...ux-foundation.org>, <ira.weiny@...el.com>,
        <dan.j.williams@...el.com>, <jack@...e.cz>,
        Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -next] mm: mark a intentional data race in page_zonenum()

On 2/6/20 6:35 AM, Marco Elver wrote:
...
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 52269e56c514..cafccad584c2 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -920,7 +920,7 @@ vm_fault_t finish_mkwrite_fault(struct vm_fault *vmf);
>>>>
>>>>   static inline enum zone_type page_zonenum(const struct page *page)
>>>>   {
>>>> -   return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK;
>>>> +   return data_race((page->flags >> ZONES_PGSHIFT) & ZONES_MASK);
>>>
>>>
>>> I don't know about this. Lots of the kernel is written to do this sort
>>> of thing, and adding a load of "data_race()" everywhere is...well, I'm not
>>> sure if it's really the best way.  I wonder: could we maybe teach this
>>> kcsan thing to understand a few of the key idioms, particularly about page
>>> flags, instead of annotating all over the place?
>>
>> My understanding is that it is rather difficult to change the compilers, but it
>> is a good question and I Cc Marco who is the maintainer for KCSAN that might
>> give you a definite answer.
> 
> The problem is that there is no general idiom where we could say with
> confidence that a data race is safe across the whole kernel. Here it


Yes. I'm grasping at straws now, but...what about the idiom that page_zonenum()
uses: a set of bits that are "always" (after a certain early point) read-only?
What are your thoughts on that?


> might not matter, but somewhere else it might matter a lot.
> 
> If you think that it turns out the entire file may be littered with
> 'data_race()', and you do not want to use annotations, you can
> blacklist the file. I already had to do this for other files in mm/,
> because concurrent flag modification/checking is pervasive and a lot
> of them seem 'benign'. We decided to revisit those files later.
> 
> Feel free to add 'KCSAN_SANITIZE_memory.o := n' or whatever other
> files you think are full of these to mm/Makefile.
> 
> The only problem I see with that is that it's not obvious what is
> concurrently modified and what isn't. The annotations would have
> helped document what is happening.
> 
> Thanks,
> -- Marco
> 


thanks,
-- 
John Hubbard
NVIDIA