On Mon, Oct 14, 2024 at 03:12:09PM +0200, Vlastimil Babka wrote:
> On 10/14/24 14:52, Feng Tang wrote:
> > On Mon, Oct 14, 2024 at 10:53:32AM +0200, Vlastimil Babka wrote:
> >> On 10/14/24 09:52, Feng Tang wrote:
> >> > On Fri, Oct 04, 2024 at 05:52:10PM +0800, Vlastimil Babka wrote:
> >> > Thanks for the suggestion!
> >> >
> >> > As there were error report about the NULL slab for big kmalloc object, how
> >> > about the following code for
> >> >
> >> > __do_krealloc(const void *p, size_t new_size, gfp_t flags)
> >> > {
> >> > void *ret;
> >> > size_t ks = 0;
> >> > int orig_size = 0;
> >> > struct kmem_cache *s = NULL;
> >> >
> >> > /* Check for double-free. */
> >> > if (likely(!ZERO_OR_NULL_PTR(p))) {
> >> > if (!kasan_check_byte(p))
> >> > return NULL;
> >> >
> >> > ks = ksize(p);
> >>
> >> I think this will result in __ksize() doing
> >> skip_orig_size_check(folio_slab(folio)->slab_cache, object);
> >> and we don't want that?
> >
> > I think that's fine. As later code will re-set the orig_size anyway.
>
> But you also read it first.
>
> >> > /* Some objects have no orig_size, like big kmalloc case */
> >> > if (is_kfence_address(p)) {
> >> > orig_size = kfence_ksize(p);
> >> > } else if (virt_to_slab(p)) {
> >> > s = virt_to_cache(p);
> >> > orig_size = get_orig_size(s, (void *)p);
>
> here.
>
> >> > }
>
> >> Also the checks below repeat some of the checks of ksize().
> >
> > Yes, there is some redundancy, mostly the virt_to_slab()
> >
> >> 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?
>
> Still I think __do_krealloc() should rather do its own thing and not call
> ksize().
The goal was to avoid having users of the allocation APIs change the
sizes of allocations without calling into realloc. This is because
otherwise the "alloc_size" attribute used by compilers inform
__builtin_dynamic_object_size() can get confused:
ptr = alloc(less_than_bucket_size);
...
size = ksize(ptr); /* larger size! */
memcpy(ptr, src, size); /* compiler instrumentation doesn't see that ptr "grows" */
So the callers use kmalloc_size_roundup() to just allocate the rounded
up size immediately. Internally, the allocator can do what it wants.
--
Kees Cook
