Re: [PATCH v2 0/5] mm/slub: Improve data handling of krealloc() when orig_size is enabled

[Feng Tang <feng.tang@intel.com>]

On Mon, Nov 04, 2024 at 12:45:51PM +0100, Vlastimil Babka wrote:
> 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()?

Sounds good to me! And for future wrong usages of ksize(), we can fix
them case by case when they are deteced.

Thanks,
Feng