| Message ID | 20220909102413.2899-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/gnttab: fix gnttab_acquire_resource() | expand |
On 09.09.2022 12:24, Juergen Gross wrote: > Commit 9dc46386d89d ("gnttab: work around "may be used uninitialized" > warning") was wrong, as vaddrs can legitimately be NULL in case > XENMEM_resource_grant_table_id_status was specified for a grant table > v1. This would result in crashes in debug builds due to > ASSERT_UNREACHABLE() triggering. > > Check vaddrs only to be NULL in the rc == 0 case. > > Fixes: 9dc46386d89d ("gnttab: work around "may be used uninitialized" warning") > Signed-off-by: Juergen Gross <jgross@suse.com> > Release-acked-by: Henry Wang <Henry.Wang@arm.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > Might be considered for backporting Sure. Jan
On 09/09/2022 11:24, Juergen Gross wrote: > Commit 9dc46386d89d ("gnttab: work around "may be used uninitialized" > warning") was wrong, as vaddrs can legitimately be NULL in case > XENMEM_resource_grant_table_id_status was specified for a grant table > v1. This would result in crashes in debug builds due to > ASSERT_UNREACHABLE() triggering. > > Check vaddrs only to be NULL in the rc == 0 case. > > Fixes: 9dc46386d89d ("gnttab: work around "may be used uninitialized" warning") > Signed-off-by: Juergen Gross <jgross@suse.com> > Release-acked-by: Henry Wang <Henry.Wang@arm.com> > --- > Might be considered for backporting Definitely for backport. Given that we do actually have a unit(ish) test capable of triggering this, I would request that tools/tests/resource/test-resource.c gets extended in the same commit, so we keep our testing in sync with our bugfixes. I've just got all the tests working in XenRT, and my comment about OSSTest fixes for 4.17 on the community call was first and foremost about wiring up these tests too. They're trivial run (even before the work to give them a consistent interface - probably 4.18 now), so there's no excuse for them not to run. In this case, we create the VMs strictly with gnttab v1, so the size() and map() calls should fail with -ENODEV (or possibly 0 for size). and not trigger any assertions in Xen. ~Andrew
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index ad773a6996..fba329dcc2 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4140,7 +4140,7 @@ int gnttab_acquire_resource( * on non-error paths, and hence it needs setting to NULL at the top of the * function. Leave some runtime safety. */ - if ( !vaddrs ) + if ( !rc && !vaddrs ) { ASSERT_UNREACHABLE(); rc = -ENODATA;
Commit 9dc46386d89d ("gnttab: work around "may be used uninitialized" warning") was wrong, as vaddrs can legitimately be NULL in case XENMEM_resource_grant_table_id_status was specified for a grant table v1. This would result in crashes in debug builds due to ASSERT_UNREACHABLE() triggering. Check vaddrs only to be NULL in the rc == 0 case. Fixes: 9dc46386d89d ("gnttab: work around "may be used uninitialized" warning") Signed-off-by: Juergen Gross <jgross@suse.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> --- Might be considered for backporting --- xen/common/grant_table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)