BUG? misused atomic instructions in mm/swapfile.c

BUG? misused atomic instructions in mm/swapfile.c

[홍신 shin hong]

Hello. I am reporting atomic instructions usages
which are suspected to be misused in mm/swapfile.c
of Linux 2.6.30.5.

I do not have much background on mm
so that I am not certain whether it is correct or not.
But I hope this report is helpful. Please examine the code.

In try_to_use(), setup_swap_extents(), and SYSCALL_DEFINE2(),
there are following codes:

    if (atomic_read(&start_mm->mm_users) == 1) {
        mmput(start_mm) ;
        start_mm = &init_mm ;
        atomic_inc(&init_mm.mm_users) ;
    }

It first checks start_mm->mm_users and then increments its value by one.

If one of these functions is executed in two different threads
for the same start_mm concurrently,
mmput(start_mm) can be executed twice as result of race.

I think it would be better to combine two atomic operations
into one atomic operation (e.g. atomic_cmpxchg).

Thank you.

Sincerely
Shin Hong

--
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>

Re: BUG? misused atomic instructions in mm/swapfile.c

[Minchan Kim]

On Thu, 3 Sep 2009 14:55:22 +0900
i??i?  shin hong <hongshin@gmail.com> wrote:

> Hello. I am reporting atomic instructions usages
> which are suspected to be misused in mm/swapfile.c
> of Linux 2.6.30.5.
> 
> I do not have much background on mm
> so that I am not certain whether it is correct or not.
> But I hope this report is helpful. Please examine the code.
> 
> In try_to_use(), setup_swap_extents(), and SYSCALL_DEFINE2(),
> there are following codes:

First of all, I can find it only in try_to_use. 
Do I miss somewhere?

> 
>     if (atomic_read(&start_mm->mm_users) == 1) {
>         mmput(start_mm) ;
>         start_mm = &init_mm ;
>         atomic_inc(&init_mm.mm_users) ;
>     }
> 
> It first checks start_mm->mm_users and then increments its value by one.
> 
> If one of these functions is executed in two different threads
> for the same start_mm concurrently,
> mmput(start_mm) can be executed twice as result of race.
Is is a matter? 

I looked over the code. 
Couldn't atomic_dec_and_test avoid your concern?

void mmput(struct mm_struct *mm) 
{
        might_sleep();

        if (atomic_dec_and_test(&mm->mm_users)) {
...


> 
> I think it would be better to combine two atomic operations
> into one atomic operation (e.g. atomic_cmpxchg).
> 
> Thank you.
> 
> Sincerely
> Shin Hong
> 
> --
> 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>


-- 
Kind regards,
Minchan Kim

--
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>

Re: BUG? misused atomic instructions in mm/swapfile.c

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2738 bytes --]

On Thu, 3 Sep 2009, 홍신 shin hong wrote:

> Hello. I am reporting atomic instructions usages
> which are suspected to be misused in mm/swapfile.c
> of Linux 2.6.30.5.
> 
> I do not have much background on mm
> so that I am not certain whether it is correct or not.
> But I hope this report is helpful. Please examine the code.
> 
> In try_to_use(), setup_swap_extents(), and SYSCALL_DEFINE2(),
> there are following codes:

It's only in try_to_unuse(), and let's add in the comment above it:
      /*
       * Don't hold on to start_mm if it looks like exiting.
       */
>     if (atomic_read(&start_mm->mm_users) == 1) {
>         mmput(start_mm) ;
>         start_mm = &init_mm ;
>         atomic_inc(&init_mm.mm_users) ;
>     }
> 
> It first checks start_mm->mm_users and then increments its value by one.
                                     and if it happens to be 1, decrements
  it in mmput, points start_mm to init_mm and increments that's mm_users.

> 
> If one of these functions is executed in two different threads
> for the same start_mm concurrently,

Yes, that could happen, if swapoff is run concurrently on two different
swap areas: I usually forget that's even a possibility, and it's not an
efficient way to work, but we don't exclude it - thanks for reminding me.

> mmput(start_mm) can be executed twice as result of race.

That would be okay.  The juggling between init_mm, start_mm, new_start_mm,
prev_mm and mm is intricate and hard to follow!  but the reference that
that mmput puts started off with our atomic_inc_not_zero(&mm->mm_users)
lower down: this swapoff is mmput'ting a reference it acquired itself,
now associated with start_mm, and it's entitled to do so when resetting
start_mm, whether mm_users is 1 at that moment or not.

But given that, the "race" you describe cannot occur: if a concurrent
swapoff is going through the same code with the same start_mm, mm_users
will be at least 2.  The "problem" that can occur is the reverse of the
one you saw: the start_mm process may be exiting, but neither swapoff
sees mm_users 1, so together they hold that mm from being freed.

That too is okay: exit_mmap can free a lot of swap much faster than
swapoff can do it, so we prefer to get out of its way if we can; but
if occasionally we don't notice, no big deal.  After all, mm_users
might go down to 1 just a moment after that check there (or perhaps
even up to 2): it's nothing more than a heuristic.

> 
> I think it would be better to combine two atomic operations
> into one atomic operation (e.g. atomic_cmpxchg).

That's not necessary here at all, but is important in the
atomic_inc_not_zero we got our first reference from.

Hugh