| Message ID | 00bb4998-d0a7-43dc-8d3c-abb3f66661cc@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.19] gnttab: fix compat query-size handling | expand |
On 25.06.2024 09:30, Jan Beulich wrote: > The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar > constructs, should have caught my attention. Turns out it was needed for > the build to succeed merely because the corresponding #ifndef had a > typo. That typo in turn broke compat mode guests, by having query-size > requests of theirs wire into the domain_crash() at the bottom of the > switch(). > > Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size check") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Looks like set-version is similarly missing in the set of structures > checked, but I'm pretty sure that we will now want to defer taking care > of that until after 4.20 was branched. Actually it's get-version too, yet then for both only half of what's needed that's missing (and luckily only the just-in-case part, not the actual layout check). In any event I've queued a patch for 4.20. Jan
On Tue, 2024-06-25 at 09:30 +0200, Jan Beulich wrote: > The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other > similar > constructs, should have caught my attention. Turns out it was needed > for > the build to succeed merely because the corresponding #ifndef had a > typo. That typo in turn broke compat mode guests, by having query- > size > requests of theirs wire into the domain_crash() at the bottom of the > switch(). > > Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native > gnttab_query_size check") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Release-Acked-by: Oleksii Kurochko <Oleksii.kurochko@gmail.com> ~ Oleksii > --- > Looks like set-version is similarly missing in the set of structures > checked, but I'm pretty sure that we will now want to defer taking > care > of that until after 4.20 was branched. > > --- a/xen/common/compat/grant_table.c > +++ b/xen/common/compat/grant_table.c > @@ -33,7 +33,6 @@ CHECK_gnttab_unmap_and_replace; > #define xen_gnttab_query_size gnttab_query_size > CHECK_gnttab_query_size; > #undef xen_gnttab_query_size > -DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_compat_t); > > DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t); > DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_compat_t); > @@ -111,7 +110,7 @@ int compat_grant_table_op( > CASE(copy); > #endif > > -#ifndef CHECK_query_size > +#ifndef CHECK_gnttab_query_size > CASE(query_size); > #endif >
On Tue, Jun 25, 2024 at 09:30:07AM +0200, Jan Beulich wrote: > The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar > constructs, should have caught my attention. Turns out it was needed for > the build to succeed merely because the corresponding #ifndef had a > typo. That typo in turn broke compat mode guests, by having query-size > requests of theirs wire into the domain_crash() at the bottom of the > switch(). > > Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size check") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Looks like set-version is similarly missing in the set of structures > checked, but I'm pretty sure that we will now want to defer taking care > of that until after 4.20 was branched. If we have to backport the fix anyway, we might as well consider taking it now. Thanks, Roger.
On 25.06.2024 11:24, Roger Pau Monné wrote: > On Tue, Jun 25, 2024 at 09:30:07AM +0200, Jan Beulich wrote: >> The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar >> constructs, should have caught my attention. Turns out it was needed for >> the build to succeed merely because the corresponding #ifndef had a >> typo. That typo in turn broke compat mode guests, by having query-size >> requests of theirs wire into the domain_crash() at the bottom of the >> switch(). >> >> Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size check") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- >> Looks like set-version is similarly missing in the set of structures >> checked, but I'm pretty sure that we will now want to defer taking care >> of that until after 4.20 was branched. > > If we have to backport the fix anyway, we might as well consider > taking it now. While I put a Fixes: tag there, this is the kind of change where I don't think it needs backporting. The ABI of released versions is supposed to be yet less in flux than the "stable" ABI in general. Jan
On 25/06/2024 8:30 am, Jan Beulich wrote: > The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar > constructs, should have caught my attention. Turns out it was needed for > the build to succeed merely because the corresponding #ifndef had a > typo. That typo in turn broke compat mode guests, by having query-size > requests of theirs wire into the domain_crash() at the bottom of the > switch(). > > Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size check") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Looks like set-version is similarly missing in the set of structures > checked, but I'm pretty sure that we will now want to defer taking care > of that until after 4.20 was branched. > > --- a/xen/common/compat/grant_table.c > +++ b/xen/common/compat/grant_table.c > @@ -33,7 +33,6 @@ CHECK_gnttab_unmap_and_replace; > #define xen_gnttab_query_size gnttab_query_size > CHECK_gnttab_query_size; > #undef xen_gnttab_query_size > -DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_compat_t); > > DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t); > DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_compat_t); > @@ -111,7 +110,7 @@ int compat_grant_table_op( > CASE(copy); > #endif > > -#ifndef CHECK_query_size > +#ifndef CHECK_gnttab_query_size > CASE(query_size); > #endif > /sigh - I almost rejected your and Stefano's feedback on v1 on the basis that it didn't compile, but then I adjusted it to look like the surrounding logic. Much fool me. But, this change *cannot* be correct. The result is: $ git grep -C3 CHECK_gnttab_query_size compat/grant_table.c-31-#undef xen_gnttab_unmap_and_replace compat/grant_table.c-32- compat/grant_table.c-33-#define xen_gnttab_query_size gnttab_query_size compat/grant_table.c:34:CHECK_gnttab_query_size; compat/grant_table.c-35-#undef xen_gnttab_query_size compat/grant_table.c-36- compat/grant_table.c-37-DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t); -- compat/grant_table.c-110- CASE(copy); compat/grant_table.c-111-#endif compat/grant_table.c-112- compat/grant_table.c:113:#ifndef CHECK_gnttab_query_size compat/grant_table.c-114- CASE(query_size); compat/grant_table.c-115-#endif compat/grant_table.c-116- and the second is dead code because CHECK_gnttab_query_size is defined. It shouldn't be there. So - my v1 was correct, and your and Stefano's feedback on v1 was incorrect. The problem is, of course, that absolutely none of this is written down, and none of the logic one needs to read to figure out it is even checked into the tree. It's entirely automatic and not even legible in its intermediate form. We are going to start with writing down a very simple set of instructions for how the compat infrastructure works and how it should be used. If it's too complicated I will delete bits until it becomes manageable, because this is completely insane. ~Andrew
On 25.06.2024 12:43, Andrew Cooper wrote: > On 25/06/2024 8:30 am, Jan Beulich wrote: >> The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar >> constructs, should have caught my attention. Turns out it was needed for >> the build to succeed merely because the corresponding #ifndef had a >> typo. That typo in turn broke compat mode guests, by having query-size >> requests of theirs wire into the domain_crash() at the bottom of the >> switch(). >> >> Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size check") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Looks like set-version is similarly missing in the set of structures >> checked, but I'm pretty sure that we will now want to defer taking care >> of that until after 4.20 was branched. >> >> --- a/xen/common/compat/grant_table.c >> +++ b/xen/common/compat/grant_table.c >> @@ -33,7 +33,6 @@ CHECK_gnttab_unmap_and_replace; >> #define xen_gnttab_query_size gnttab_query_size >> CHECK_gnttab_query_size; >> #undef xen_gnttab_query_size >> -DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_compat_t); >> >> DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t); >> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_compat_t); >> @@ -111,7 +110,7 @@ int compat_grant_table_op( >> CASE(copy); >> #endif >> >> -#ifndef CHECK_query_size >> +#ifndef CHECK_gnttab_query_size >> CASE(query_size); >> #endif >> > > /sigh - I almost rejected your and Stefano's feedback on v1 on the basis > that it didn't compile, but then I adjusted it to look like the > surrounding logic. Much fool me. > > But, this change *cannot* be correct. The result is: > > $ git grep -C3 CHECK_gnttab_query_size > compat/grant_table.c-31-#undef xen_gnttab_unmap_and_replace > compat/grant_table.c-32- > compat/grant_table.c-33-#define xen_gnttab_query_size gnttab_query_size > compat/grant_table.c:34:CHECK_gnttab_query_size; > compat/grant_table.c-35-#undef xen_gnttab_query_size > compat/grant_table.c-36- > compat/grant_table.c-37-DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t); > -- > compat/grant_table.c-110- CASE(copy); > compat/grant_table.c-111-#endif > compat/grant_table.c-112- > compat/grant_table.c:113:#ifndef CHECK_gnttab_query_size > compat/grant_table.c-114- CASE(query_size); > compat/grant_table.c-115-#endif > compat/grant_table.c-116- > > and the second is dead code because CHECK_gnttab_query_size is defined. > It shouldn't be there. As said elsewhere, it's there just-in-case (and now consistent with sibling gnttab-ops). We can certainly evaluate deleting all of those just-in-case constructs. But we want to retain consistency. > So - my v1 was correct, and your and Stefano's feedback on v1 was incorrect. I'm sorry, but maybe more a misunderstanding. I notice I had "now" in my reply to you when referring to my reply to Stefano, when I think I really meant "not". And he never actually replied, afaics. Jan
--- a/xen/common/compat/grant_table.c +++ b/xen/common/compat/grant_table.c @@ -33,7 +33,6 @@ CHECK_gnttab_unmap_and_replace; #define xen_gnttab_query_size gnttab_query_size CHECK_gnttab_query_size; #undef xen_gnttab_query_size -DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_compat_t); DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_compat_t); DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_compat_t); @@ -111,7 +110,7 @@ int compat_grant_table_op( CASE(copy); #endif -#ifndef CHECK_query_size +#ifndef CHECK_gnttab_query_size CASE(query_size); #endif
The odd DEFINE_XEN_GUEST_HANDLE(), inconsistent with all other similar constructs, should have caught my attention. Turns out it was needed for the build to succeed merely because the corresponding #ifndef had a typo. That typo in turn broke compat mode guests, by having query-size requests of theirs wire into the domain_crash() at the bottom of the switch(). Fixes: 8c3bb4d8ce3f ("xen/gnttab: Perform compat/native gnttab_query_size check") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Looks like set-version is similarly missing in the set of structures checked, but I'm pretty sure that we will now want to defer taking care of that until after 4.20 was branched.