Signed-off-by: Jan Kara <[email protected]>
---
mm/process_vm_access.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index fd26d0433509..c1bc47d8ed90 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -64,12 +64,8 @@ static int process_vm_rw_pages(struct task_struct *task,
*bytes_copied = 0;
/* Get the pages we're interested in */
- down_read(&mm->mmap_sem);
- pages_pinned = get_user_pages(task, mm, pa,
- nr_pages_to_copy,
- vm_write, 0, process_pages, NULL);
- up_read(&mm->mmap_sem);
-
+ pages_pinned = get_user_pages_unlocked(task, mm, pa, nr_pages_to_copy,
+ vm_write, 0, process_pages);
if (pages_pinned != nr_pages_to_copy) {
rc = -EFAULT;
goto end;
--
1.8.1.4
(10/2/13 10:27 AM), Jan Kara wrote:
> Signed-off-by: Jan Kara <[email protected]>
> ---
> mm/process_vm_access.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
> index fd26d0433509..c1bc47d8ed90 100644
> --- a/mm/process_vm_access.c
> +++ b/mm/process_vm_access.c
> @@ -64,12 +64,8 @@ static int process_vm_rw_pages(struct task_struct *task,
> *bytes_copied = 0;
>
> /* Get the pages we're interested in */
> - down_read(&mm->mmap_sem);
> - pages_pinned = get_user_pages(task, mm, pa,
> - nr_pages_to_copy,
> - vm_write, 0, process_pages, NULL);
> - up_read(&mm->mmap_sem);
> -
> + pages_pinned = get_user_pages_unlocked(task, mm, pa, nr_pages_to_copy,
> + vm_write, 0, process_pages);
> if (pages_pinned != nr_pages_to_copy) {
> rc = -EFAULT;
> goto end;
This is wrong because original code is wrong. In this function, page may be pointed to
anon pages. Then, you should keep to take mmap_sem until finish to copying. Otherwise
concurrent fork() makes nasty COW issue.
On Wed 02-10-13 12:32:33, KOSAKI Motohiro wrote:
> (10/2/13 10:27 AM), Jan Kara wrote:
> > Signed-off-by: Jan Kara <[email protected]>
> > ---
> > mm/process_vm_access.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
> > index fd26d0433509..c1bc47d8ed90 100644
> > --- a/mm/process_vm_access.c
> > +++ b/mm/process_vm_access.c
> > @@ -64,12 +64,8 @@ static int process_vm_rw_pages(struct task_struct *task,
> > *bytes_copied = 0;
> >
> > /* Get the pages we're interested in */
> > - down_read(&mm->mmap_sem);
> > - pages_pinned = get_user_pages(task, mm, pa,
> > - nr_pages_to_copy,
> > - vm_write, 0, process_pages, NULL);
> > - up_read(&mm->mmap_sem);
> > -
> > + pages_pinned = get_user_pages_unlocked(task, mm, pa, nr_pages_to_copy,
> > + vm_write, 0, process_pages);
> > if (pages_pinned != nr_pages_to_copy) {
> > rc = -EFAULT;
> > goto end;
>
> This is wrong because original code is wrong. In this function, page may
> be pointed to anon pages. Then, you should keep to take mmap_sem until
> finish to copying. Otherwise concurrent fork() makes nasty COW issue.
Hum, can you be more specific? I suppose you are speaking about situation
when the remote task we are copying data from/to does fork while
process_vm_rw_pages() runs. If we are copying data from remote task, I
don't see how COW could cause any problem. If we are copying to remote task
and fork happens after get_user_pages() but before copy_to_user() then I
can see we might be having some trouble. copy_to_user() would then copy
data into both original remote process and its child thus essentially
bypassing COW. If the child process manages to COW some of the pages before
copy_to_user() happens, it can even see only some of the pages. Is that what
you mean?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
(10/2/13 3:36 PM), Jan Kara wrote:
> On Wed 02-10-13 12:32:33, KOSAKI Motohiro wrote:
>> (10/2/13 10:27 AM), Jan Kara wrote:
>>> Signed-off-by: Jan Kara <[email protected]>
>>> ---
>>> mm/process_vm_access.c | 8 ++------
>>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
>>> index fd26d0433509..c1bc47d8ed90 100644
>>> --- a/mm/process_vm_access.c
>>> +++ b/mm/process_vm_access.c
>>> @@ -64,12 +64,8 @@ static int process_vm_rw_pages(struct task_struct *task,
>>> *bytes_copied = 0;
>>>
>>> /* Get the pages we're interested in */
>>> - down_read(&mm->mmap_sem);
>>> - pages_pinned = get_user_pages(task, mm, pa,
>>> - nr_pages_to_copy,
>>> - vm_write, 0, process_pages, NULL);
>>> - up_read(&mm->mmap_sem);
>>> -
>>> + pages_pinned = get_user_pages_unlocked(task, mm, pa, nr_pages_to_copy,
>>> + vm_write, 0, process_pages);
>>> if (pages_pinned != nr_pages_to_copy) {
>>> rc = -EFAULT;
>>> goto end;
>>
>> This is wrong because original code is wrong. In this function, page may
>> be pointed to anon pages. Then, you should keep to take mmap_sem until
>> finish to copying. Otherwise concurrent fork() makes nasty COW issue.
> Hum, can you be more specific? I suppose you are speaking about situation
> when the remote task we are copying data from/to does fork while
> process_vm_rw_pages() runs. If we are copying data from remote task, I
> don't see how COW could cause any problem. If we are copying to remote task
> and fork happens after get_user_pages() but before copy_to_user() then I
> can see we might be having some trouble. copy_to_user() would then copy
> data into both original remote process and its child thus essentially
> bypassing COW. If the child process manages to COW some of the pages before
> copy_to_user() happens, it can even see only some of the pages. Is that what
> you mean?
scenario 1: vm_write==0
Process P1 call get_user_pages(pa, process_pages) in process_vm_rw_pages
P1 unlock mmap_sem.
Process P2 call fork(). and make P3.
P2 write memory pa. now the "process_pages" is owned by P3 (the child process)
P3 write memory pa. and then the content of "process_pages" is changed.
P1 read process_pages as P2's page. but actually, it is P3's data. Then, P1 observe garbage, at least unintended, data was read.
scenario 2: vm_write==1
Process P1 call get_user_pages(pa, process_pages) in process_vm_rw_pages. It makes COW break and any anon page sharing broke.
P1 unlock mmap_sem.
P2 call fork(). and make P3. And, now COW page sharing is restored.
P2 write memory pa. now the "process_pages" is owned by P3.
P3 write memory pa. it mean P3 changes "process_pages".
P1 write process_pages as P2's page. but actually, it is P3's. It override above P3's write and then P3 observe data corruption.
The solution is to keep holding mmap_sem until finishing process_pages access
because mmap_sem prevent fork. and then race never be happen. I mean you cann't use
get_user_pages_unlock() if target address point to anon pages.
I'm not sure these story match your explanation.
On Thu 03-10-13 18:40:06, KOSAKI Motohiro wrote:
> (10/2/13 3:36 PM), Jan Kara wrote:
> >On Wed 02-10-13 12:32:33, KOSAKI Motohiro wrote:
> >>(10/2/13 10:27 AM), Jan Kara wrote:
> >>>Signed-off-by: Jan Kara <[email protected]>
> >>>---
> >>> mm/process_vm_access.c | 8 ++------
> >>> 1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
> >>>index fd26d0433509..c1bc47d8ed90 100644
> >>>--- a/mm/process_vm_access.c
> >>>+++ b/mm/process_vm_access.c
> >>>@@ -64,12 +64,8 @@ static int process_vm_rw_pages(struct task_struct *task,
> >>> *bytes_copied = 0;
> >>>
> >>> /* Get the pages we're interested in */
> >>>- down_read(&mm->mmap_sem);
> >>>- pages_pinned = get_user_pages(task, mm, pa,
> >>>- nr_pages_to_copy,
> >>>- vm_write, 0, process_pages, NULL);
> >>>- up_read(&mm->mmap_sem);
> >>>-
> >>>+ pages_pinned = get_user_pages_unlocked(task, mm, pa, nr_pages_to_copy,
> >>>+ vm_write, 0, process_pages);
> >>> if (pages_pinned != nr_pages_to_copy) {
> >>> rc = -EFAULT;
> >>> goto end;
> >>
> >>This is wrong because original code is wrong. In this function, page may
> >>be pointed to anon pages. Then, you should keep to take mmap_sem until
> >>finish to copying. Otherwise concurrent fork() makes nasty COW issue.
> > Hum, can you be more specific? I suppose you are speaking about situation
> >when the remote task we are copying data from/to does fork while
> >process_vm_rw_pages() runs. If we are copying data from remote task, I
> >don't see how COW could cause any problem. If we are copying to remote task
> >and fork happens after get_user_pages() but before copy_to_user() then I
> >can see we might be having some trouble. copy_to_user() would then copy
> >data into both original remote process and its child thus essentially
> >bypassing COW. If the child process manages to COW some of the pages before
> >copy_to_user() happens, it can even see only some of the pages. Is that what
> >you mean?
>
> scenario 1: vm_write==0
>
> Process P1 call get_user_pages(pa, process_pages) in process_vm_rw_pages
> P1 unlock mmap_sem.
> Process P2 call fork(). and make P3.
> P2 write memory pa. now the "process_pages" is owned by P3 (the child process)
> P3 write memory pa. and then the content of "process_pages" is changed.
> P1 read process_pages as P2's page. but actually, it is P3's data. Then,
> P1 observe garbage, at least unintended, data was read.
Yeah, this really looks buggy because P1 can see data in (supposedly)
P2's address space which P2 never wrote there.
> scenario 2: vm_write==1
>
> Process P1 call get_user_pages(pa, process_pages) in process_vm_rw_pages.
> It makes COW break and any anon page sharing broke.
> P1 unlock mmap_sem.
> P2 call fork(). and make P3. And, now COW page sharing is restored.
> P2 write memory pa. now the "process_pages" is owned by P3.
> P3 write memory pa. it mean P3 changes "process_pages".
> P1 write process_pages as P2's page. but actually, it is P3's. It
> override above P3's write and then P3 observe data corruption.
Yep, this is a similar problem as above. Thanks for pointing this out.
> The solution is to keep holding mmap_sem until finishing process_pages
> access because mmap_sem prevent fork. and then race never be happen. I
> mean you cann't use get_user_pages_unlock() if target address point to
> anon pages.
Yeah, if you are accessing third party mm, you've convinced me you
currently need mmap_sem to avoid problems with COW on anon pages. I'm just
thinking that this "hold mmap_sem to prevent fork" is somewhat subtle
(definitely would deserve a comment) and if it would be needed in more
places we might be better off if we have a more explicit mechanism for that
(like a special lock, fork sequence count, or something like that). Anyway
I'll have this in mind and if I see other places that need this, I'll try
to come up with some solution. Thanks again for explanation.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
(10/7/13 4:55 PM), Jan Kara wrote:
> On Thu 03-10-13 18:40:06, KOSAKI Motohiro wrote:
>> (10/2/13 3:36 PM), Jan Kara wrote:
>>> On Wed 02-10-13 12:32:33, KOSAKI Motohiro wrote:
>>>> (10/2/13 10:27 AM), Jan Kara wrote:
>>>>> Signed-off-by: Jan Kara <[email protected]>
>>>>> ---
>>>>> mm/process_vm_access.c | 8 ++------
>>>>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
>>>>> index fd26d0433509..c1bc47d8ed90 100644
>>>>> --- a/mm/process_vm_access.c
>>>>> +++ b/mm/process_vm_access.c
>>>>> @@ -64,12 +64,8 @@ static int process_vm_rw_pages(struct task_struct *task,
>>>>> *bytes_copied = 0;
>>>>>
>>>>> /* Get the pages we're interested in */
>>>>> - down_read(&mm->mmap_sem);
>>>>> - pages_pinned = get_user_pages(task, mm, pa,
>>>>> - nr_pages_to_copy,
>>>>> - vm_write, 0, process_pages, NULL);
>>>>> - up_read(&mm->mmap_sem);
>>>>> -
>>>>> + pages_pinned = get_user_pages_unlocked(task, mm, pa, nr_pages_to_copy,
>>>>> + vm_write, 0, process_pages);
>>>>> if (pages_pinned != nr_pages_to_copy) {
>>>>> rc = -EFAULT;
>>>>> goto end;
>>>>
>>>> This is wrong because original code is wrong. In this function, page may
>>>> be pointed to anon pages. Then, you should keep to take mmap_sem until
>>>> finish to copying. Otherwise concurrent fork() makes nasty COW issue.
>>> Hum, can you be more specific? I suppose you are speaking about situation
>>> when the remote task we are copying data from/to does fork while
>>> process_vm_rw_pages() runs. If we are copying data from remote task, I
>>> don't see how COW could cause any problem. If we are copying to remote task
>>> and fork happens after get_user_pages() but before copy_to_user() then I
>>> can see we might be having some trouble. copy_to_user() would then copy
>>> data into both original remote process and its child thus essentially
>>> bypassing COW. If the child process manages to COW some of the pages before
>>> copy_to_user() happens, it can even see only some of the pages. Is that what
>>> you mean?
>>
>> scenario 1: vm_write==0
>>
>> Process P1 call get_user_pages(pa, process_pages) in process_vm_rw_pages
>> P1 unlock mmap_sem.
>> Process P2 call fork(). and make P3.
>> P2 write memory pa. now the "process_pages" is owned by P3 (the child process)
>> P3 write memory pa. and then the content of "process_pages" is changed.
>> P1 read process_pages as P2's page. but actually, it is P3's data. Then,
>> P1 observe garbage, at least unintended, data was read.
> Yeah, this really looks buggy because P1 can see data in (supposedly)
> P2's address space which P2 never wrote there.
>
>> scenario 2: vm_write==1
>>
>> Process P1 call get_user_pages(pa, process_pages) in process_vm_rw_pages.
>> It makes COW break and any anon page sharing broke.
>> P1 unlock mmap_sem.
>> P2 call fork(). and make P3. And, now COW page sharing is restored.
>> P2 write memory pa. now the "process_pages" is owned by P3.
>> P3 write memory pa. it mean P3 changes "process_pages".
>> P1 write process_pages as P2's page. but actually, it is P3's. It
>> override above P3's write and then P3 observe data corruption.
> Yep, this is a similar problem as above. Thanks for pointing this out.
>
>> The solution is to keep holding mmap_sem until finishing process_pages
>> access because mmap_sem prevent fork. and then race never be happen. I
>> mean you cann't use get_user_pages_unlock() if target address point to
>> anon pages.
> Yeah, if you are accessing third party mm,
one correction. the condition is,
- third party mm, or
- current process have multiple threads and other threads makes fork() and COW break
>you've convinced me you
> currently need mmap_sem to avoid problems with COW on anon pages. I'm just
> thinking that this "hold mmap_sem to prevent fork" is somewhat subtle
> (definitely would deserve a comment) and if it would be needed in more
> places we might be better off if we have a more explicit mechanism for that
> (like a special lock, fork sequence count, or something like that).
Hmm..
Actually, I tried this several years ago. But MM people disliked it because
they prefer faster kernel than simple code. Any additional lock potentially
makes slower the kernel.
However, I fully agree the current mechanism is too complex and misleading,
or at least, very hard to understanding. So, any improvement idea is welcome.
> Anyway
> I'll have this in mind and if I see other places that need this, I'll try
> to come up with some solution. Thanks again for explanation.
NP.
I tried to explain this at MM summit. but my English is too poor and I couldn't
explain enough to you. Sorry about that.
Anyway, I'd like to fix process_vm_rw_pages() before my memory will be flushed again.
Thank you for helping remember this bug.