Re: [PATCH 1/2] mm: krealloc: consider spare memory for __GFP_ZERO

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Jul 30, 2024 at 11:14:16PM +0200, Vlastimil Babka wrote:
> On 7/30/24 9:42 PM, Danilo Krummrich wrote:
> > As long as krealloc() is called with __GFP_ZERO consistently, starting
> > with the initial memory allocation, __GFP_ZERO should be fully honored.
> > 
> > However, if for an existing allocation krealloc() is called with a
> > decreased size, it is not ensured that the spare portion the allocation
> > is zeroed. Thus, if krealloc() is subsequently called with a larger size
> > again, __GFP_ZERO can't be fully honored, since we don't know the
> > previous size, but only the bucket size.
> > 
> > Example:
> > 
> > 	buf = kzalloc(64, GFP_KERNEL);
> > 	memset(buf, 0xff, 64);
> > 
> > 	buf = krealloc(buf, 48, GFP_KERNEL | __GFP_ZERO);
> > 
> > 	/* After this call the last 16 bytes are still 0xff. */
> > 	buf = krealloc(buf, 64, GFP_KERNEL | __GFP_ZERO);
> > 
> > Fix this, by explicitly setting spare memory to zero, when shrinking an
> > allocation with __GFP_ZERO flag set or init_on_alloc enabled.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  mm/slab_common.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 40b582a014b8..cff602cedf8e 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -1273,6 +1273,13 @@ __do_krealloc(const void *p, size_t new_size, gfp_t flags)
> >  
> >  	/* If the object still fits, repoison it precisely. */
> >  	if (ks >= new_size) {
> > +		/* Zero out spare memory. */
> > +		if (want_init_on_alloc(flags)) {
> > +			kasan_disable_current();
> > +			memset((void *)p + new_size, 0, ks - new_size);
> > +			kasan_enable_current();
> 
> If we do kasan_krealloc() first, shouldn't the memset then be legal
> afterwards without the disable/enable dance?

No, we always write into the poisoned area. The following tables show what we do
in the particular case:

Shrink
------
          new        old
0         size       size        ks
|----------|----------|----------|
|   keep   |        poison       |  <- poison
|--------------------------------|
|   keep   |         zero        |  <- data


Poison and zero things between old size and ks is not necessary, but we don't
know old size, hence we have do it between new size and ks.

Grow
----
          old        new
0         size       size        ks
|----------|----------|----------|
|       unpoison      |   keep   | <- poison
|--------------------------------|
|         keep        |   zero   | <- data

Zeroing between new_size and ks in not necessary in this case, since it must be
zero already. But without knowing the old size we don't know whether we shrink
and actually need to do something, or if we grow and don't need to do anything.

Analogously, we also unpoison things between 0 and old size for the same reason.

> 
> > +		}
> > +
> >  		p = kasan_krealloc((void *)p, new_size, flags);
> >  		return (void *)p;
> >  	}
> > 
> > base-commit: 7c3dd6d99f2df6a9d7944ee8505b195ba51c9b68
>