Re: [PATCH v7 05/14] xen: extend domctl interface for cache coloring

From: Carlo Nonato <carlo.nonato@minervasys.tech>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	 Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	 Marco Solieri <marco.solieri@minervasys.tech>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v7 05/14] xen: extend domctl interface for cache coloring
Date: Thu, 21 Mar 2024 16:11:27 +0100	[thread overview]
Message-ID: <CAG+AhRX5HYc608gYgSk=EdJrK2V30=ZMN=1oEbTPJ1-0UKyNNw@mail.gmail.com> (raw)
In-Reply-To: <f934db54-3233-4330-9573-f3df3af389a1@suse.com>

Hi Jan,

On Tue, Mar 19, 2024 at 4:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.03.2024 11:58, Carlo Nonato wrote:
> > @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
> >      xfree(__va(__pa(d->llc_colors)));
> >  }
> >
> > +int domain_set_llc_colors(struct domain *d,
> > +                          const struct xen_domctl_set_llc_colors *config)
> > +{
> > +    unsigned int *colors;
> > +
> > +    if ( d->num_llc_colors )
> > +        return -EEXIST;
> > +
> > +    if ( !config->num_llc_colors )
> > +        return domain_set_default_colors(d);
> > +
> > +    if ( config->num_llc_colors > max_nr_colors || config->pad )
>
> The check of "pad" wants carrying out in all cases; I expect it wants
> moving to the caller.

Ok.

> > +        return -EINVAL;
> > +
> > +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
> > +        return -EFAULT;
>
> You're leaking "colors" when taking this or ...
>
> > +    if ( !check_colors(colors, config->num_llc_colors) )
> > +    {
> > +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> > +        return -EINVAL;
>
> ... this error path.

You're right.

> Jan

Thanks.

On Tue, Mar 19, 2024 at 4:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.03.2024 11:58, Carlo Nonato wrote:
> > @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
> >      xfree(__va(__pa(d->llc_colors)));
> >  }
> >
> > +int domain_set_llc_colors(struct domain *d,
> > +                          const struct xen_domctl_set_llc_colors *config)
> > +{
> > +    unsigned int *colors;
> > +
> > +    if ( d->num_llc_colors )
> > +        return -EEXIST;
> > +
> > +    if ( !config->num_llc_colors )
> > +        return domain_set_default_colors(d);
> > +
> > +    if ( config->num_llc_colors > max_nr_colors || config->pad )
>
> The check of "pad" wants carrying out in all cases; I expect it wants
> moving to the caller.
>
> > +        return -EINVAL;
> > +
> > +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(colors, config->llc_colors, config->num_llc_colors) )
> > +        return -EFAULT;
>
> You're leaking "colors" when taking this or ...
>
> > +    if ( !check_colors(colors, config->num_llc_colors) )
> > +    {
> > +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> > +        return -EINVAL;
>
> ... this error path.
>
> Jan