linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
@ 2022-03-14  6:40 Miaohe Lin
  2022-03-14 15:21 ` Eric W. Biederman
  0 siblings, 1 reply; 7+ messages in thread
From: Miaohe Lin @ 2022-03-14  6:40 UTC (permalink / raw)
  To: akpm, hughd; +Cc: linux-mm, linux-kernel, linmiaohe

user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
this by resetting allowed to 0.

Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Hugh Dickins <hughd@google.com>
---
v1->v2:
  correct Fixes tag and collect Acked-by tag
  Thanks Hugh for review!
---
 mm/mlock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/mlock.c b/mm/mlock.c
index 29372c0eebe5..efd2dd2943de 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
 	}
 	if (!get_ucounts(ucounts)) {
 		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
+		allowed = 0;
 		goto out;
 	}
 	allowed = 1;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
  2022-03-14  6:40 [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment Miaohe Lin
@ 2022-03-14 15:21 ` Eric W. Biederman
  2022-03-15 12:17   ` Miaohe Lin
  0 siblings, 1 reply; 7+ messages in thread
From: Eric W. Biederman @ 2022-03-14 15:21 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, hughd, linux-mm, linux-kernel, Alexey Gladkov

Miaohe Lin <linmiaohe@huawei.com> writes:

> user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
> the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
> this by resetting allowed to 0.

This fix looks correct.  But the ability for people to follow and read
the code seems questionable.  I saw in v1 of this patch Hugh originally
misread the logic.

Could we instead change the code to leave lock_limit at ULONG_MAX aka
RLIM_INFINITY, leave initialized to 0, and not even need a special case
of RLIM_INFINITY as nothing can be greater that ULONG_MAX?

Something like this?

diff --git a/mm/mlock.c b/mm/mlock.c
index 8f584eddd305..e7eabf5193ab 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -827,13 +827,12 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
 
 	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	lock_limit = rlimit(RLIMIT_MEMLOCK);
-	if (lock_limit == RLIM_INFINITY)
-		allowed = 1;
-	lock_limit >>= PAGE_SHIFT;
+	if (lock_limit != RLIM_INFINITY)
+		lock_limit >>= PAGE_SHIFT;
 	spin_lock(&shmlock_user_lock);
 	memlock = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
 
-	if (!allowed && (memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
+	if ((memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
 		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
 		goto out;
 	}

>
> Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> Acked-by: Hugh Dickins <hughd@google.com>
> ---
> v1->v2:
>   correct Fixes tag and collect Acked-by tag
>   Thanks Hugh for review!
> ---
>  mm/mlock.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 29372c0eebe5..efd2dd2943de 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>  	}
>  	if (!get_ucounts(ucounts)) {
>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
> +		allowed = 0;
>  		goto out;
>  	}
>  	allowed = 1;

Eric

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
  2022-03-14 15:21 ` Eric W. Biederman
@ 2022-03-15 12:17   ` Miaohe Lin
  2022-03-15 18:32     ` Eric W. Biederman
  0 siblings, 1 reply; 7+ messages in thread
From: Miaohe Lin @ 2022-03-15 12:17 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: akpm, hughd, linux-mm, linux-kernel, Alexey Gladkov

On 2022/3/14 23:21, Eric W. Biederman wrote:
> Miaohe Lin <linmiaohe@huawei.com> writes:
> 
>> user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
>> the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
>> this by resetting allowed to 0.
> 
> This fix looks correct.  But the ability for people to follow and read
> the code seems questionable.  I saw in v1 of this patch Hugh originally
> misread the logic.
> 
> Could we instead change the code to leave lock_limit at ULONG_MAX aka
> RLIM_INFINITY, leave initialized to 0, and not even need a special case
> of RLIM_INFINITY as nothing can be greater that ULONG_MAX?
> 

Many thanks for your advice. This looks good but it seems this results in different
behavior: When (memlock == LONG_MAX) && !capable(CAP_IPC_LOCK), we would fail now
while it will always success without this change. We should avoid this difference.
Or am I miss something? Maybe the origin patch is more suitable and simple?

Thanks.

> Something like this?
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 8f584eddd305..e7eabf5193ab 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -827,13 +827,12 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>  
>  	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>  	lock_limit = rlimit(RLIMIT_MEMLOCK);
> -	if (lock_limit == RLIM_INFINITY)
> -		allowed = 1;
> -	lock_limit >>= PAGE_SHIFT;
> +	if (lock_limit != RLIM_INFINITY)
> +		lock_limit >>= PAGE_SHIFT;
>  	spin_lock(&shmlock_user_lock);
>  	memlock = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>  
> -	if (!allowed && (memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
> +	if ((memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>  		goto out;
>  	}
> 
>>
>> Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> Acked-by: Hugh Dickins <hughd@google.com>
>> ---
>> v1->v2:
>>   correct Fixes tag and collect Acked-by tag
>>   Thanks Hugh for review!
>> ---
>>  mm/mlock.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 29372c0eebe5..efd2dd2943de 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>  	}
>>  	if (!get_ucounts(ucounts)) {
>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>> +		allowed = 0;
>>  		goto out;
>>  	}
>>  	allowed = 1;
> 
> Eric
> .
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
  2022-03-15 12:17   ` Miaohe Lin
@ 2022-03-15 18:32     ` Eric W. Biederman
  2022-03-16  6:55       ` Miaohe Lin
  0 siblings, 1 reply; 7+ messages in thread
From: Eric W. Biederman @ 2022-03-15 18:32 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, hughd, linux-mm, linux-kernel, Alexey Gladkov

Miaohe Lin <linmiaohe@huawei.com> writes:

> On 2022/3/14 23:21, Eric W. Biederman wrote:
>> Miaohe Lin <linmiaohe@huawei.com> writes:
>> 
>>> user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
>>> the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
>>> this by resetting allowed to 0.
>> 
>> This fix looks correct.  But the ability for people to follow and read
>> the code seems questionable.  I saw in v1 of this patch Hugh originally
>> misread the logic.
>> 
>> Could we instead change the code to leave lock_limit at ULONG_MAX aka
>> RLIM_INFINITY, leave initialized to 0, and not even need a special case
>> of RLIM_INFINITY as nothing can be greater that ULONG_MAX?
>> 
>
> Many thanks for your advice. This looks good but it seems this results in different
> behavior: When (memlock == LONG_MAX) && !capable(CAP_IPC_LOCK), we would fail now
> while it will always success without this change. We should avoid this difference.
> Or am I miss something? Maybe the origin patch is more suitable and
> simple?

Interesting.  I think that is an unintended and necessary bug fix.

When memlock == LONG_MAX that means inc_rlimit_ucounts failed.

It either failed because at another level the limit was exceeded or
because the counter wrapped.  In either case it is not appropriate to
succeed if inc_rlimit_ucounts detects a failure.

Which is a long way of saying I think we really want the simplification
because it found and fixed another bug as well.

Without the simplification I don't think I will be confident the code is
correct.

Eric


> Thanks.
>
>> Something like this?
>> 
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 8f584eddd305..e7eabf5193ab 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -827,13 +827,12 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>  
>>  	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>  	lock_limit = rlimit(RLIMIT_MEMLOCK);
>> -	if (lock_limit == RLIM_INFINITY)
>> -		allowed = 1;
>> -	lock_limit >>= PAGE_SHIFT;
>> +	if (lock_limit != RLIM_INFINITY)
>> +		lock_limit >>= PAGE_SHIFT;
>>  	spin_lock(&shmlock_user_lock);
>>  	memlock = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>  
>> -	if (!allowed && (memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>> +	if ((memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>  		goto out;
>>  	}
>> 
>>>
>>> Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>> Acked-by: Hugh Dickins <hughd@google.com>
>>> ---
>>> v1->v2:
>>>   correct Fixes tag and collect Acked-by tag
>>>   Thanks Hugh for review!
>>> ---
>>>  mm/mlock.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>> index 29372c0eebe5..efd2dd2943de 100644
>>> --- a/mm/mlock.c
>>> +++ b/mm/mlock.c
>>> @@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>  	}
>>>  	if (!get_ucounts(ucounts)) {
>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>> +		allowed = 0;
>>>  		goto out;
>>>  	}
>>>  	allowed = 1;
>> 
>> Eric
>> .
>> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
  2022-03-15 18:32     ` Eric W. Biederman
@ 2022-03-16  6:55       ` Miaohe Lin
  2022-03-16 14:11         ` Eric W. Biederman
  0 siblings, 1 reply; 7+ messages in thread
From: Miaohe Lin @ 2022-03-16  6:55 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: akpm, hughd, linux-mm, linux-kernel, Alexey Gladkov

On 2022/3/16 2:32, Eric W. Biederman wrote:
> Miaohe Lin <linmiaohe@huawei.com> writes:
> 
>> On 2022/3/14 23:21, Eric W. Biederman wrote:
>>> Miaohe Lin <linmiaohe@huawei.com> writes:
>>>
>>>> user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
>>>> the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
>>>> this by resetting allowed to 0.
>>>
>>> This fix looks correct.  But the ability for people to follow and read
>>> the code seems questionable.  I saw in v1 of this patch Hugh originally
>>> misread the logic.
>>>
>>> Could we instead change the code to leave lock_limit at ULONG_MAX aka
>>> RLIM_INFINITY, leave initialized to 0, and not even need a special case
>>> of RLIM_INFINITY as nothing can be greater that ULONG_MAX?
>>>
>>
>> Many thanks for your advice. This looks good but it seems this results in different
>> behavior: When (memlock == LONG_MAX) && !capable(CAP_IPC_LOCK), we would fail now
>> while it will always success without this change. We should avoid this difference.
>> Or am I miss something? Maybe the origin patch is more suitable and
>> simple?
> 
> Interesting.  I think that is an unintended and necessary bug fix.
> 
> When memlock == LONG_MAX that means inc_rlimit_ucounts failed.
> 
> It either failed because at another level the limit was exceeded or
> because the counter wrapped.  In either case it is not appropriate to
> succeed if inc_rlimit_ucounts detects a failure.
> 
> Which is a long way of saying I think we really want the simplification
> because it found and fixed another bug as well.
> 
> Without the simplification I don't think I will be confident the code is
> correct.

Agree with you. This is a potential bug and you just catch it with the
code simplification. :)

Am I supposed to do this altogether or will you do this simplification part?
Many thanks.

> 
> Eric
> 
> 
>> Thanks.
>>
>>> Something like this?
>>>
>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>> index 8f584eddd305..e7eabf5193ab 100644
>>> --- a/mm/mlock.c
>>> +++ b/mm/mlock.c
>>> @@ -827,13 +827,12 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>  
>>>  	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>>  	lock_limit = rlimit(RLIMIT_MEMLOCK);
>>> -	if (lock_limit == RLIM_INFINITY)
>>> -		allowed = 1;
>>> -	lock_limit >>= PAGE_SHIFT;
>>> +	if (lock_limit != RLIM_INFINITY)
>>> +		lock_limit >>= PAGE_SHIFT;
>>>  	spin_lock(&shmlock_user_lock);
>>>  	memlock = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>  
>>> -	if (!allowed && (memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>> +	if ((memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>  		goto out;
>>>  	}
>>>
>>>>
>>>> Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>> Acked-by: Hugh Dickins <hughd@google.com>
>>>> ---
>>>> v1->v2:
>>>>   correct Fixes tag and collect Acked-by tag
>>>>   Thanks Hugh for review!
>>>> ---
>>>>  mm/mlock.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>> index 29372c0eebe5..efd2dd2943de 100644
>>>> --- a/mm/mlock.c
>>>> +++ b/mm/mlock.c
>>>> @@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>>  	}
>>>>  	if (!get_ucounts(ucounts)) {
>>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>> +		allowed = 0;
>>>>  		goto out;
>>>>  	}
>>>>  	allowed = 1;
>>>
>>> Eric
>>> .
>>>
> .
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
  2022-03-16  6:55       ` Miaohe Lin
@ 2022-03-16 14:11         ` Eric W. Biederman
  2022-03-17  1:50           ` Miaohe Lin
  0 siblings, 1 reply; 7+ messages in thread
From: Eric W. Biederman @ 2022-03-16 14:11 UTC (permalink / raw)
  To: Miaohe Lin; +Cc: akpm, hughd, linux-mm, linux-kernel, Alexey Gladkov

Miaohe Lin <linmiaohe@huawei.com> writes:

> On 2022/3/16 2:32, Eric W. Biederman wrote:
>> Miaohe Lin <linmiaohe@huawei.com> writes:
>> 
>>> On 2022/3/14 23:21, Eric W. Biederman wrote:
>>>> Miaohe Lin <linmiaohe@huawei.com> writes:
>>>>
>>>>> user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
>>>>> the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
>>>>> this by resetting allowed to 0.
>>>>
>>>> This fix looks correct.  But the ability for people to follow and read
>>>> the code seems questionable.  I saw in v1 of this patch Hugh originally
>>>> misread the logic.
>>>>
>>>> Could we instead change the code to leave lock_limit at ULONG_MAX aka
>>>> RLIM_INFINITY, leave initialized to 0, and not even need a special case
>>>> of RLIM_INFINITY as nothing can be greater that ULONG_MAX?
>>>>
>>>
>>> Many thanks for your advice. This looks good but it seems this results in different
>>> behavior: When (memlock == LONG_MAX) && !capable(CAP_IPC_LOCK), we would fail now
>>> while it will always success without this change. We should avoid this difference.
>>> Or am I miss something? Maybe the origin patch is more suitable and
>>> simple?
>> 
>> Interesting.  I think that is an unintended and necessary bug fix.
>> 
>> When memlock == LONG_MAX that means inc_rlimit_ucounts failed.
>> 
>> It either failed because at another level the limit was exceeded or
>> because the counter wrapped.  In either case it is not appropriate to
>> succeed if inc_rlimit_ucounts detects a failure.
>> 
>> Which is a long way of saying I think we really want the simplification
>> because it found and fixed another bug as well.
>> 
>> Without the simplification I don't think I will be confident the code is
>> correct.
>
> Agree with you. This is a potential bug and you just catch it with the
> code simplification. :)
>
> Am I supposed to do this altogether or will you do this simplification part?
> Many thanks.

If you can that would be great, and you can have the credit.

Otherwise I will make my proposed changes into a proper patch.  At this
point we just need to dot the i's and cross the t's and get this fix in.

Eric

>>>> Something like this?
>>>>
>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>> index 8f584eddd305..e7eabf5193ab 100644
>>>> --- a/mm/mlock.c
>>>> +++ b/mm/mlock.c
>>>> @@ -827,13 +827,12 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>>  
>>>>  	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>>>  	lock_limit = rlimit(RLIMIT_MEMLOCK);
>>>> -	if (lock_limit == RLIM_INFINITY)
>>>> -		allowed = 1;
>>>> -	lock_limit >>= PAGE_SHIFT;
>>>> +	if (lock_limit != RLIM_INFINITY)
>>>> +		lock_limit >>= PAGE_SHIFT;
>>>>  	spin_lock(&shmlock_user_lock);
>>>>  	memlock = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>>  
>>>> -	if (!allowed && (memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>>> +	if ((memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>>  		goto out;
>>>>  	}
>>>>
>>>>>
>>>>> Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
>>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>>> Acked-by: Hugh Dickins <hughd@google.com>
>>>>> ---
>>>>> v1->v2:
>>>>>   correct Fixes tag and collect Acked-by tag
>>>>>   Thanks Hugh for review!
>>>>> ---
>>>>>  mm/mlock.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>> index 29372c0eebe5..efd2dd2943de 100644
>>>>> --- a/mm/mlock.c
>>>>> +++ b/mm/mlock.c
>>>>> @@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>>>  	}
>>>>>  	if (!get_ucounts(ucounts)) {
>>>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>>> +		allowed = 0;
>>>>>  		goto out;
>>>>>  	}
>>>>>  	allowed = 1;
>>>>
>>>> Eric
>>>> .
>>>>
>> .
>> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment
  2022-03-16 14:11         ` Eric W. Biederman
@ 2022-03-17  1:50           ` Miaohe Lin
  0 siblings, 0 replies; 7+ messages in thread
From: Miaohe Lin @ 2022-03-17  1:50 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: akpm, hughd, linux-mm, linux-kernel, Alexey Gladkov

On 2022/3/16 22:11, Eric W. Biederman wrote:
> Miaohe Lin <linmiaohe@huawei.com> writes:
> 
>> On 2022/3/16 2:32, Eric W. Biederman wrote:
>>> Miaohe Lin <linmiaohe@huawei.com> writes:
>>>
>>>> On 2022/3/14 23:21, Eric W. Biederman wrote:
>>>>> Miaohe Lin <linmiaohe@huawei.com> writes:
>>>>>
>>>>>> user_shm_lock forgets to set allowed to 0 when get_ucounts fails. So
>>>>>> the later user_shm_unlock might do the extra dec_rlimit_ucounts. Fix
>>>>>> this by resetting allowed to 0.
>>>>>
>>>>> This fix looks correct.  But the ability for people to follow and read
>>>>> the code seems questionable.  I saw in v1 of this patch Hugh originally
>>>>> misread the logic.
>>>>>
>>>>> Could we instead change the code to leave lock_limit at ULONG_MAX aka
>>>>> RLIM_INFINITY, leave initialized to 0, and not even need a special case
>>>>> of RLIM_INFINITY as nothing can be greater that ULONG_MAX?
>>>>>
>>>>
>>>> Many thanks for your advice. This looks good but it seems this results in different
>>>> behavior: When (memlock == LONG_MAX) && !capable(CAP_IPC_LOCK), we would fail now
>>>> while it will always success without this change. We should avoid this difference.
>>>> Or am I miss something? Maybe the origin patch is more suitable and
>>>> simple?
>>>
>>> Interesting.  I think that is an unintended and necessary bug fix.
>>>
>>> When memlock == LONG_MAX that means inc_rlimit_ucounts failed.
>>>
>>> It either failed because at another level the limit was exceeded or
>>> because the counter wrapped.  In either case it is not appropriate to
>>> succeed if inc_rlimit_ucounts detects a failure.
>>>
>>> Which is a long way of saying I think we really want the simplification
>>> because it found and fixed another bug as well.
>>>
>>> Without the simplification I don't think I will be confident the code is
>>> correct.
>>
>> Agree with you. This is a potential bug and you just catch it with the
>> code simplification. :)
>>
>> Am I supposed to do this altogether or will you do this simplification part?
>> Many thanks.
> 
> If you can that would be great, and you can have the credit.
> 
> Otherwise I will make my proposed changes into a proper patch.  At this
> point we just need to dot the i's and cross the t's and get this fix in.

I will try to do this. Many thanks!

> 
> Eric
> 
>>>>> Something like this?
>>>>>
>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>> index 8f584eddd305..e7eabf5193ab 100644
>>>>> --- a/mm/mlock.c
>>>>> +++ b/mm/mlock.c
>>>>> @@ -827,13 +827,12 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>>>  
>>>>>  	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>>>>  	lock_limit = rlimit(RLIMIT_MEMLOCK);
>>>>> -	if (lock_limit == RLIM_INFINITY)
>>>>> -		allowed = 1;
>>>>> -	lock_limit >>= PAGE_SHIFT;
>>>>> +	if (lock_limit != RLIM_INFINITY)
>>>>> +		lock_limit >>= PAGE_SHIFT;
>>>>>  	spin_lock(&shmlock_user_lock);
>>>>>  	memlock = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>>>  
>>>>> -	if (!allowed && (memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>>>> +	if ((memlock == LONG_MAX || memlock > lock_limit) && !capable(CAP_IPC_LOCK)) {
>>>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>>>  		goto out;
>>>>>  	}
>>>>>
>>>>>>
>>>>>> Fixes: d7c9e99aee48 ("Reimplement RLIMIT_MEMLOCK on top of ucounts")
>>>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>>>> Acked-by: Hugh Dickins <hughd@google.com>
>>>>>> ---
>>>>>> v1->v2:
>>>>>>   correct Fixes tag and collect Acked-by tag
>>>>>>   Thanks Hugh for review!
>>>>>> ---
>>>>>>  mm/mlock.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>>> index 29372c0eebe5..efd2dd2943de 100644
>>>>>> --- a/mm/mlock.c
>>>>>> +++ b/mm/mlock.c
>>>>>> @@ -733,6 +733,7 @@ int user_shm_lock(size_t size, struct ucounts *ucounts)
>>>>>>  	}
>>>>>>  	if (!get_ucounts(ucounts)) {
>>>>>>  		dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_MEMLOCK, locked);
>>>>>> +		allowed = 0;
>>>>>>  		goto out;
>>>>>>  	}
>>>>>>  	allowed = 1;
>>>>>
>>>>> Eric
>>>>> .
>>>>>
>>> .
>>>
> .
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-03-17  1:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14  6:40 [PATCH v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment Miaohe Lin
2022-03-14 15:21 ` Eric W. Biederman
2022-03-15 12:17   ` Miaohe Lin
2022-03-15 18:32     ` Eric W. Biederman
2022-03-16  6:55       ` Miaohe Lin
2022-03-16 14:11         ` Eric W. Biederman
2022-03-17  1:50           ` Miaohe Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).