do_shrink_slab() returns unsigned long value, and
the placing into int variable cuts high bytes off.
Then we compare ret and 0xfffffffe (since SHRINK_EMPTY
is converted to ret type).
Thus, big number of objects returned by do_shrink_slab()
may be interpreted as SHRINK_EMPTY, if low bytes of
their value are equal to 0xfffffffe. Fix that
by declaration ret as unsigned long in these functions.
Reported-by: Cyrill Gorcunov <[email protected]>
Signed-off-by: Kirill Tkhai <[email protected]>
---
mm/vmscan.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 0b63d9a2dc17..8ea87586925e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -581,8 +581,8 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
struct mem_cgroup *memcg, int priority)
{
struct memcg_shrinker_map *map;
- unsigned long freed = 0;
- int ret, i;
+ unsigned long ret, freed = 0;
+ int i;
if (!memcg_kmem_enabled() || !mem_cgroup_online(memcg))
return 0;
@@ -678,9 +678,8 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
struct mem_cgroup *memcg,
int priority)
{
+ unsigned long ret, freed = 0;
struct shrinker *shrinker;
- unsigned long freed = 0;
- int ret;
if (!mem_cgroup_is_root(memcg))
return shrink_slab_memcg(gfp_mask, nid, memcg, priority);
On Fri, Sep 28, 2018 at 02:28:32PM +0300, Kirill Tkhai wrote:
> do_shrink_slab() returns unsigned long value, and
> the placing into int variable cuts high bytes off.
> Then we compare ret and 0xfffffffe (since SHRINK_EMPTY
> is converted to ret type).
>
> Thus, big number of objects returned by do_shrink_slab()
> may be interpreted as SHRINK_EMPTY, if low bytes of
> their value are equal to 0xfffffffe. Fix that
> by declaration ret as unsigned long in these functions.
>
> Reported-by: Cyrill Gorcunov <[email protected]>
> Signed-off-by: Kirill Tkhai <[email protected]>
Acked-by: Cyrill Gorcunov <[email protected]>
Thank you!
On Fri, Sep 28, 2018 at 02:28:32PM +0300, Kirill Tkhai wrote:
> do_shrink_slab() returns unsigned long value, and
> the placing into int variable cuts high bytes off.
> Then we compare ret and 0xfffffffe (since SHRINK_EMPTY
> is converted to ret type).
>
> Thus, big number of objects returned by do_shrink_slab()
> may be interpreted as SHRINK_EMPTY, if low bytes of
> their value are equal to 0xfffffffe. Fix that
> by declaration ret as unsigned long in these functions.
>
> Reported-by: Cyrill Gorcunov <[email protected]>
> Signed-off-by: Kirill Tkhai <[email protected]>
Reviewed-by: Josef Bacik <[email protected]>
Thanks,
Josef
On Fri, 28 Sep 2018 14:28:32 +0300 Kirill Tkhai <[email protected]> wrote:
> do_shrink_slab() returns unsigned long value, and
> the placing into int variable cuts high bytes off.
> Then we compare ret and 0xfffffffe (since SHRINK_EMPTY
> is converted to ret type).
>
> Thus, big number of objects returned by do_shrink_slab()
> may be interpreted as SHRINK_EMPTY, if low bytes of
> their value are equal to 0xfffffffe. Fix that
> by declaration ret as unsigned long in these functions.
Sigh. How many times has this happened.
> Reported-by: Cyrill Gorcunov <[email protected]>
What did he report? Was it code inspection? Did the kernel explode?
etcetera. I'm thinking that the fix should be backported but to
determine that, we need to understand the end-user runtime effects, as
always. Please.
On Fri, Sep 28, 2018 at 02:15:09PM -0700, Andrew Morton wrote:
> What did he report? Was it code inspection? Did the kernel explode?
> etcetera. I'm thinking that the fix should be backported but to
> determine that, we need to understand the end-user runtime effects, as
> always. Please.
I've been investigating unrelated but and occasionally found this nit.
Look, there should be over 4G of objects scanned to have it triggered,
so I don't expect it happen in real life but better be on a safe side
and fix it.
On 29.09.2018 00:15, Andrew Morton wrote:
> On Fri, 28 Sep 2018 14:28:32 +0300 Kirill Tkhai <[email protected]> wrote:
>
>> do_shrink_slab() returns unsigned long value, and
>> the placing into int variable cuts high bytes off.
>> Then we compare ret and 0xfffffffe (since SHRINK_EMPTY
>> is converted to ret type).
>>
>> Thus, big number of objects returned by do_shrink_slab()
>> may be interpreted as SHRINK_EMPTY, if low bytes of
>> their value are equal to 0xfffffffe. Fix that
>> by declaration ret as unsigned long in these functions.
>
> Sigh. How many times has this happened.
>
>> Reported-by: Cyrill Gorcunov <[email protected]>
>
> What did he report? Was it code inspection? Did the kernel explode?
> etcetera. I'm thinking that the fix should be backported but to
> determine that, we need to understand the end-user runtime effects, as
> always. Please.
Yeah, it was just code inspection. It's need to be a really unlucky person
to meet this in real life -- the probability is very small.
The runtime effect would be the following. Such the unlucky person would
have a single shrinker, which is never called for a single memory cgroup,
despite there are objects charged.
Thanks,
Kirill