[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <79335db3-4528-446e-a839-272645133e19@suse.cz>
Date: Mon, 4 Nov 2024 12:45:51 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Feng Tang <feng.tang@...el.com>, Kees Cook <keescook@...omium.org>
Cc: Marco Elver <elver@...gle.com>, Andrew Morton
<akpm@...ux-foundation.org>, Christoph Lameter <cl@...ux.com>,
Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Hyeonggon Yoo <42.hyeyoo@...il.com>, Andrey Konovalov
<andreyknvl@...il.com>, Shuah Khan <skhan@...uxfoundation.org>,
David Gow <davidgow@...gle.com>, Danilo Krummrich <dakr@...nel.org>,
Alexander Potapenko <glider@...gle.com>,
Andrey Ryabinin <ryabinin.a.a@...il.com>, Dmitry Vyukov
<dvyukov@...gle.com>, Vincenzo Frascino <vincenzo.frascino@....com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v2 0/5] mm/slub: Improve data handling of krealloc() when
orig_size is enabled
On 11/4/24 12:28, Feng Tang wrote:
> On Mon, Oct 14, 2024 at 10:20:36PM +0800, Tang, Feng wrote:
>> On Mon, Oct 14, 2024 at 03:12:09PM +0200, Vlastimil Babka wrote:
>> > >
>> > >> So I think in __do_krealloc() we should do things manually to determine ks
>> > >> and not call ksize(). Just not break any of the cases ksize() handles
>> > >> (kfence, large kmalloc).
>> > >
>> > > OK, originally I tried not to expose internals of __ksize(). Let me
>> > > try this way.
>> >
>> > ksize() makes assumptions that a user outside of slab itself is calling it.
>> >
>> > But we (well mostly Kees) also introduced kmalloc_size_roundup() to avoid
>> > querying ksize() for the purposes of writing beyond the original
>> > kmalloc(size) up to the bucket size. So maybe we can also investigate if the
>> > skip_orig_size_check() mechanism can be removed now?
>>
>> I did a quick grep, and fortunately it seems that the ksize() user are
>> much less than before. We used to see some trouble in network code, which
>> is now very clean without the need to skip orig_size check. Will check
>> other call site later.
>
>
> I did more further check about ksize() usage, and there are still some
> places to be handled. The thing stands out is kfree_sensitive(), and
> another potential one is sound/soc/codecs/cs-amp-lib-test.c
>
> Some details:
>
> * Thanks to Kees Cook, who has cured many cases of ksize() as below:
>
> drivers/base/devres.c: total_old_size = ksize(container_of(ptr, struct devres, data));
> drivers/net/ethernet/intel/igb/igb_main.c: } else if (size > ksize(q_vector)) {
> net/core/skbuff.c: *size = ksize(data);
> net/openvswitch/flow_netlink.c: new_acts_size = max(next_offset + req_size, ksize(*sfa) * 2);
> kernel/bpf/verifier.c: alloc_bytes = max(ksize(orig), kmalloc_size_roundup(bytes));
>
> * Some callers use ksize() mostly for calculation or sanity check,
> and not for accessing those extra space, which are fine:
>
> drivers/gpu/drm/drm_managed.c: WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container)));
> lib/kunit/string-stream-test.c: actual_bytes_used = ksize(stream);
> lib/kunit/string-stream-test.c: actual_bytes_used += ksize(frag_container);
> lib/kunit/string-stream-test.c: actual_bytes_used += ksize(frag_container->fragment);
> mm/nommu.c: return ksize(objp);
> mm/util.c: memcpy(n, kasan_reset_tag(p), ksize(p));
> security/tomoyo/gc.c: tomoyo_memory_used[TOMOYO_MEMORY_POLICY] -= ksize(ptr);
> security/tomoyo/memory.c: const size_t s = ksize(ptr);
> drivers/md/dm-vdo/memory-alloc.c: add_kmalloc_block(ksize(p));
> drivers/md/dm-vdo/memory-alloc.c: add_kmalloc_block(ksize(p));
> drivers/md/dm-vdo/memory-alloc.c: remove_kmalloc_block(ksize(ptr));
>
> * One usage may need to be handled
>
> sound/soc/codecs/cs-amp-lib-test.c: KUNIT_ASSERT_GE_MSG(test, ksize(buf), priv->cal_blob->size, "Buffer to small");
>
> * bigger problem is the kfree_sensitive(), which will use ksize() to
> get the total size and then zero all of them.
>
> One solution for this could be get the kmem_cache first, and
> do the skip_orig_size_check()
Maybe add a parameter for __ksize() that controls if we do
skip_orig_size_check(), current ksize() will pass "false" to it (once
remaining wrong users are handled), then another ksize_internal() variant
will pass "true" and be used from kfree_sensitive()?
> Thanks,
> Feng
Powered by blists - more mailing lists