| Message ID | 20250217102741.4122367-1-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] xen/memory: Make resource_max_frames() to return 0 on unknown type | expand |
On 17/02/2025 10:27 am, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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 Please could you add a step looking for an invalid resource id and check you get 0 back? > Also, add an ASSERT_UNREACHABLE() in the default case of _acquire_resource(), > normally we won't get to this point, as an unknown type will always be rejected > earlier in resource_max_frames(). Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics") > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> ~Andrew
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@epam.com> >> >> 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. From 872565da55b7e87e1664714bb1b3ee84245db4a5 Mon Sep 17 00:00:00 2001 From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> 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@epam.com> --- 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 + * of uint16_t type. + */ + rc = xenforeignmemory_resource_size( + fh, domid, 65535, + XENMEM_resource_grant_table_id_shared, &size); + + /* + * 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"); } static void test_domain_configurations(void)
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@epam.com> >>> >>> 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@invisiblethingslab.com wires these tests up in Gitlab CI. > > > From 872565da55b7e87e1664714bb1b3ee84245db4a5 Mon Sep 17 00:00:00 2001 > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > 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@epam.com> > --- > 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. ~Andrew
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@epam.com> >>>> >>>> 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@invisiblethingslab.com > > > 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@epam.com> >> 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@epam.com> >> --- >> 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
diff --git a/xen/common/memory.c b/xen/common/memory.c index a6f2f6d1b3..8ca4e1a842 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -1157,7 +1157,7 @@ static unsigned int resource_max_frames(const struct domain *d, return d->vmtrace_size >> PAGE_SHIFT; default: - return -EOPNOTSUPP; + return 0; } } @@ -1240,6 +1240,7 @@ static int _acquire_resource( return acquire_vmtrace_buf(d, id, frame, nr_frames, mfn_list); default: + ASSERT_UNREACHABLE(); return -EOPNOTSUPP; } }