Re: [PATCH V2] xen/memory: Make resource_max_frames() to return 0 on unknown type

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 17.02.25 15:52, Andrew Cooper wrote:

Hello

> On 17/02/2025 1:11 pm, Oleksandr Tyshchenko wrote:
> > On 17.02.25 13:09, Andrew Cooper wrote:
> >
> >
> > Hello Andrew
> >
> >
> > > On 17/02/2025 10:27 am, Oleksandr Tyshchenko wrote:
> > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > >
> > > > This is actually what the caller acquire_resource() expects on any kind
> > > > of error (the comment on top of resource_max_frames() also suggests
> > > > that).
> > > :(
> > >
> > > So it broke somewhere between v3 and v8 of the original patch series,
> > > but I can't seem to find the intervening series on lore.
> > >
> > > Given the comment, I suspect I got inadvertently-reviewed into this bug.
> > >
> > > > Otherwise, the caller will treat -errno as a valid value and
> > > > propagate incorrect
> > > > nr_frames to the VM. As a possible consequence, a VM trying to query
> > > > a resource
> > > > size of an unknown type will get the success result from the
> > > > hypercall and obtain
> > > > nr_frames 4294967201.
> > > This is one of the few interfaces we have low level testing for.
> > >
> > > tools/tests/resource/test-resource.c
> > yes
> >
> > > Please could you add a step looking for an invalid resource id and check
> > > you get 0 back?
> >
> >
> > Sure. I was thinking where to add this step and propose the following
> > change. I will send a formal patch once I find a way how to easily
> > test this change.
> https://lore.kernel.org/xen-devel/cover.36ee649a8537af1a5fb5b3c5b7ffc0d8a1369969.1739496480.git-series.marmarek@xxxxxxxxxxxxxxxxxxxxxx
>
>
> wires these tests up in Gitlab CI.

thanks for the pointer.

I didn't manage to run as is. But I managed to craft something suitable 
for running just test-resource based on these patches.
> >  From 872565da55b7e87e1664714bb1b3ee84245db4a5 Mon Sep 17 00:00:00 2001
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > Date: Mon, 17 Feb 2025 14:16:50 +0200
> > Subject: [PATCH] tests/resource: Verify that Xen can deal with invalid
> >   resource type
> >
> > The sign of the presence of a corresponding bugfix is an error
> > returned on querying the size of an unknown for Xen resource type.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> >   tools/tests/resource/test-resource.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/tools/tests/resource/test-resource.c
> > b/tools/tests/resource/test-resource.c
> > index 1b10be16a6..197477c3f3 100644
> > --- a/tools/tests/resource/test-resource.c
> > +++ b/tools/tests/resource/test-resource.c
> > @@ -123,6 +123,22 @@ static void test_gnttab(uint32_t domid, unsigned
> > int nr_frames,
> >           fail("    Fail: Managed to map gnttab v2 status frames in v1
> > mode\n");
> >           xenforeignmemory_unmap_resource(fh, res);
> >       }
> > +
> > +    /*
> > +     * While at it, verify that an attempt to query the size of an
> > unknown
> > +     * for Xen resource type fails. Use the highest possible value
> > for variable
> s/for //, I think?
>
> > +     * of uint16_t type.
> > +     */
> > +    rc = xenforeignmemory_resource_size(
> > +        fh, domid, 65535,
> > +        XENMEM_resource_grant_table_id_shared, &size);
> XENMEM_resource_grant_table_id_shared should probably be 0 here.
>
> But, I'd suggest choosing 3 (literal 3, not some kind of constant from
> the headers) for the major resource number.  That has the side effect of
> forcing people to extend this test case when they add a new resource type.
>
> > +
> > +    /*
> > +     * Success here indicates that Xen is missing the bugfix to make
> > size
> > +     * requests return an error on an invalid resource type.
> > +     */
> > +    if ( !rc )
> > +        fail("    Fail: Expected error on an invalid resource type,
> > got success\n");
> I'd phrase this differently.  "Check that Xen rejected the resource type."
>
> "avoid this bug we already fixed" won't be useful to people reading this
> code in the future.  It's in the commit message.

Thanks for the preliminary review, I agree with the comments.

I will send a patch for the test-resource soon.

> ~Andrew