Re: [PATCH 1/2] mm: close race between do_fault_around() and fault_around_bytes_set()

[Andrey Ryabinin <ryabinin.a.a@gmail.com>]

2014-07-29 18:27 GMT+04:00 Kirill A. Shutemov <kirill.shutemov@linux.intel.com>:
> Andrey Ryabinin wrote:
>> On 07/29/14 15:33, Kirill A. Shutemov wrote:
>> > Things can go wrong if fault_around_bytes will be changed under
>> > do_fault_around(): between fault_around_mask() and fault_around_pages().
>> >
>> > Let's read fault_around_bytes only once during do_fault_around() and
>> > calculate mask based on the reading.
>> >
>> > Note: fault_around_bytes can only be updated via debug interface. Also
>> > I've tried but was not able to trigger a bad behaviour without the
>> > patch. So I would not consider this patch as urgent.
>> >
>> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> > ---
>> >  mm/memory.c | 17 +++++++++++------
>> >  1 file changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/mm/memory.c b/mm/memory.c
>> > index 9d66bc66f338..2ce07dc9b52b 100644
>> > --- a/mm/memory.c
>> > +++ b/mm/memory.c
>> > @@ -2772,12 +2772,12 @@ static unsigned long fault_around_bytes = rounddown_pow_of_two(65536);
>> >
>> >  static inline unsigned long fault_around_pages(void)
>> >  {
>> > -   return fault_around_bytes >> PAGE_SHIFT;
>> > +   return ACCESS_ONCE(fault_around_bytes) >> PAGE_SHIFT;
>> >  }
>> >
>> > -static inline unsigned long fault_around_mask(void)
>> > +static inline unsigned long fault_around_mask(unsigned long nr_pages)
>> >  {
>> > -   return ~(fault_around_bytes - 1) & PAGE_MASK;
>> > +   return ~(nr_pages * PAGE_SIZE - 1) & PAGE_MASK;
>> >  }
>> >
>> >
>> > @@ -2844,12 +2844,17 @@ late_initcall(fault_around_debugfs);
>> >  static void do_fault_around(struct vm_area_struct *vma, unsigned long address,
>> >             pte_t *pte, pgoff_t pgoff, unsigned int flags)
>> >  {
>> > -   unsigned long start_addr;
>> > +   unsigned long start_addr, nr_pages;
>> >     pgoff_t max_pgoff;
>> >     struct vm_fault vmf;
>> >     int off;
>> >
>> > -   start_addr = max(address & fault_around_mask(), vma->vm_start);
>> > +   nr_pages = fault_around_pages();
>> > +   /* race with fault_around_bytes_set() */
>> > +   if (nr_pages <= 1)
>>
>> unlikely() ?
>
> Yep.
>

Btw, do we need this check at all? nr_pages can't be 0, and code below
seems able to handle nr_page == 1.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>