lkml.org 
[lkml]   [2018]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [v2 PATCH] mm: introduce arg_lock to protect arg_start|end and env_start|end in mm_struct
From
Date


On 3/26/18 3:21 PM, Cyrill Gorcunov wrote:
> On Mon, Mar 26, 2018 at 11:37:25AM -0700, Matthew Wilcox wrote:
>> On Tue, Mar 27, 2018 at 02:20:39AM +0800, Yang Shi wrote:
>>> +++ b/kernel/sys.c
>>> @@ -1959,7 +1959,7 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
>>> return error;
>>> }
>>>
>>> - down_write(&mm->mmap_sem);
>>> + down_read(&mm->mmap_sem);
>>>
>>> /*
>>> * We don't validate if these members are pointing to
>>> @@ -1980,10 +1980,13 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
>>> mm->start_brk = prctl_map.start_brk;
>>> mm->brk = prctl_map.brk;
>>> mm->start_stack = prctl_map.start_stack;
>>> +
>>> + spin_lock(&mm->arg_lock);
>>> mm->arg_start = prctl_map.arg_start;
>>> mm->arg_end = prctl_map.arg_end;
>>> mm->env_start = prctl_map.env_start;
>>> mm->env_end = prctl_map.env_end;
>>> + spin_unlock(&mm->arg_lock);
>>>
>>> /*
>>> * Note this update of @saved_auxv is lockless thus
>> I see the argument for the change to a write lock was because of a BUG
>> validating arg_start and arg_end, but more generally, we are updating these
>> values, so a write-lock is probably a good idea, and this is a very rare
>> operation to do, so we don't care about making this more parallel. I would
>> not make this change (but if other more knowledgable people in this area
>> disagree with me, I will withdraw my objection to this part).
> Say we've two syscalls running prctl_set_mm_map in parallel, and imagine
> one have @start_brk = 20 @brk = 10 and second caller has @start_brk = 30
> and @brk = 20. Since now the call is guarded by _read_ the both calls
> unlocked and due to OO engine it may happen then when both finish
> we have @start_brk = 30 and @brk = 10. In turn "write" semaphore
> has been take to have consistent data on exit, either you have [20;10]
> or [30;20] assigned not something mixed.
>
> That said I think using read-lock here would be a bug.

Yes it sounds so. However, it was down_read before
ddf1d398e517e660207e2c807f76a90df543a217 ("prctl: take mmap sem for
writing to protect against others"). And, that commit is for fixing the
concurrent writing to arg_* and env_*. I just checked that commit, but
omitted the brk part. The potential issue mentioned by you should exist
before that commit, but might be just not discovered or very rare to hit.

I will change it back to down_write.

Thanks,
Yang

>
> Cyrill

\
 
 \ /
  Last update: 2018-03-27 00:00    [W:0.061 / U:40.612 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site