Re: [PATCH v1] xen/domain: fix memory leak in domain_create()

[dmkhn@xxxxxxxxx]

On Mon, Jun 23, 2025 at 10:09:54AM +0200, Jan Beulich wrote:
> On 23.06.2025 03:16, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Fix potential memory leak in domain_create() in late hardware domain case.
> >
> > Fixes: b959f3b820f5 ("xen: introduce hardware domain create flag")
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

Just in case, I've seen the patch is committed as
  3491e85a1505  ("xen/domain: fix memory leak in domain_create()")
but there's no R-b tag.

> 
> It may be relevant to mention that we still can't very well use "goto fail"
> on this error path, as insufficient struct initialization was done just yet.
> 
> What we may want to consider is to move down the get_unique_id() invocation.
> It's not the end of the world to lose one, but that may better be avoided
> when we easily can.
> 
> > ---
> > I think that no memory allocation is required before performing late hwdom
> > checks (ID range and hwdom existance).
> >
> > Looks like sanitise_domain_config() could better fit for performing such
> > configuration checks.
> >
> > Alternatively, hardware_domid range could be checked via custom parser
> > instead of code in domain_create() and then hwdom existance can be moved
> > before alloc_domain_struct().
> >
> > Thoughts?
> 
> Keeping related things together is the other desire we commonly have.

Yeah, I see this is to avoid duplicated checks, but verifying hardware_domid
range definitely can be moved outside of domain_create() 

This superfluous memory allocation will need a test case in cert world since
this is arch-common code :-/
So, IMO, it is better to avoid it.

> 
> Jan