[PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[Kirill Tkhai]

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 <gorcunov@openvz.org>
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 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);

Re: [PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[Cyrill Gorcunov]

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 <gorcunov@openvz.org>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

Thank you!

Re: [PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[Josef Bacik]

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 <gorcunov@openvz.org>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[Andrew Morton]

On Fri, 28 Sep 2018 14:28:32 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> 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 <gorcunov@openvz.org>

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.

Re: [PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[Cyrill Gorcunov]

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.

Re: [PATCH] mm: Fix int overflow in callers of do_shrink_slab()

[Kirill Tkhai]

On 29.09.2018 00:15, Andrew Morton wrote:
> On Fri, 28 Sep 2018 14:28:32 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> 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 <gorcunov@openvz.org>
> 
> 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