[CRIU] [PATCH 1/2] rst-malloc: return aligned pointers to sizeof(void *) (v3)

[Andrew Vagin]

On Wed, Feb 03, 2016 at 02:07:30PM +0300, Pavel Emelyanov wrote:
> On 01/27/2016 08:29 PM, Andrew Vagin wrote:
> > On Wed, Jan 27, 2016 at 04:29:43PM +0300, Pavel Emelyanov wrote:
> >> On 01/26/2016 10:58 PM, Andrey Vagin wrote:
> >>> From: Andrew Vagin <avagin at virtuozzo.com>
> >>>
> >>> Stas found that if we don't align a pointer,
> >>> futex and atomic operations can fail.
> >>>
> >>> v2: don't hard-code the size of void *
> >>> v3: add a function to allocate memory without gaps with
> >>>     a privious slice. It's used to allocate arrays.
> >>>
> >>> Cc: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
> >>> Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
> >>> ---
> >>>  include/rst-malloc.h |  1 +
> >>>  rst-malloc.c         | 23 ++++++++++++++++++++++-
> >>>  2 files changed, 23 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/rst-malloc.h b/include/rst-malloc.h
> >>> index 8c48f27..f205622 100644
> >>> --- a/include/rst-malloc.h
> >>> +++ b/include/rst-malloc.h
> >>> @@ -58,6 +58,7 @@ extern void *rst_mem_remap_ptr(unsigned long pos, int type);
> >>>   * last object can be freed (pop-ed from buffer).
> >>>   */
> >>>  extern void *rst_mem_alloc(unsigned long size, int type);
> >>> +extern void *rst_mem_alloc_cont(unsigned long size, int type);
> >>>  extern void rst_mem_free_last(int type);
> >>>  /*
> >>>   * Routines to remap SHREMAP and PRIVATE into restorer address space
> >>> diff --git a/rst-malloc.c b/rst-malloc.c
> >>> index 14e0b41..7685430 100644
> >>> --- a/rst-malloc.c
> >>> +++ b/rst-malloc.c
> >>> @@ -128,6 +128,9 @@ unsigned long rst_mem_cpos(int type)
> >>>  {
> >>>  	struct rst_mem_type_s *t = &rst_mems[type];
> >>>  	BUG_ON(!t->remapable || !t->enabled);
> >>> +
> >>> +	t->free_mem = (void *) round_up((unsigned long)t->free_mem, sizeof(void *));
> >>
> >> Should this hunk be here? Why?
> > 
> > Yes, it should. We want to aligne a start address of arrays. It's about
> > a case when rst_mem_alloc_cont is used.
> 
> It's wrong place for such a thing. The rst_mem_cpos() should be "pure function",
> i.e. do not change anything inside.

Where is the right place?

> 
> >>
> >>> +
> >>>  	return t->free_mem - t->buf;
> >>>  }
> >>>  
> >>> @@ -138,7 +141,7 @@ void *rst_mem_remap_ptr(unsigned long pos, int type)
> >>>  	return t->buf + pos;
> >>>  }
> >>>  
> >>> -void *rst_mem_alloc(unsigned long size, int type)
> >>> +static void *__rst_mem_alloc(unsigned long size, int type)
> >>>  {
> >>>  	struct rst_mem_type_s *t = &rst_mems[type];
> >>>  	void *ret;
> >>> @@ -158,6 +161,24 @@ void *rst_mem_alloc(unsigned long size, int type)
> >>>  	return ret;
> >>>  }
> >>>  
> >>> +void *rst_mem_alloc(unsigned long size, int type)
> >>> +{
> >>> +	struct rst_mem_type_s *t = &rst_mems[type];
> >>> +
> >>> +	t->free_mem = (void *) round_up((unsigned long)t->free_mem, sizeof(void *));
> >>
> >> Any guarantee you don't overflow the free_mem beyond the allocated memory buffer?
> > 
> > __rst_mem_alloc will check and call grow() if it's required.
> 
> Hm... Then the respective BUG_ON() should be here.
> 
> >>
> >>> +
> >>> +	return __rst_mem_alloc(size, type);
> >>> +}
> >>> +
> >>> +/* Allocate memory without gaps with a previous slice */
> >>> +void *rst_mem_alloc_cont(unsigned long size, int type)
> >>
> >> I'd better make a call titles rst_mem_alloc_aligned().
> > 
> > rst_mem_alloc_NOT_align
> > 
> > I don't want to rename rst_mem_alloc, because ussualy this sort of
> > functions (e.g. malloc) returns alligned addresses.
> > 
> > I was thinking about this, but rst_mem_alloc_cont looks better for met.
> > 
> > Maybe we need to add rst_mem_realloc() instead of rst_mem_alloc_cont.
> 
> Or rst_mem_grow(). OK, let's do that.
> 
> >>
> >>> +{
> >>> +	struct rst_mem_type_s *t = &rst_mems[type];
> >>> +	BUG_ON(!t->remapable);
> >>> +
> >>> +	return __rst_mem_alloc(size, type);
> >>> +}
> >>> +
> >>>  void rst_mem_free_last(int type)
> >>>  {
> >>>  	struct rst_mem_type_s *t = &rst_mems[type];
> >>>
> >>
> > .
> > 
>