| Message ID | 20220314064039.62972-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/mlock: fix potential imbalanced rlimit ucounts adjustment | expand |
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
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 > . >
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 >> . >>
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 >>> . >>> > . >
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 >>>> . >>>> >> . >>
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 >>>>> . >>>>> >>> . >>> > . >
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;
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(+)