| Message ID | 153813407177.17544.14888305435570723973.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Fix int overflow in callers of do_shrink_slab() | expand |
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!
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
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.
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 <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
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);
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(-)