| Message ID | e6ce03a5-cfec-42ea-91c4-b51849e2f299@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] evtchn/fifo: don't enforce higher than necessary alignment | expand |
Hi, On 21/04/2021 15:36, Jan Beulich wrote: > Neither the code nor the original commit provide any justification for > the need to 8-byte align the struct in all cases. Enforce just as much > alignment as the structure actually needs - 4 bytes - by using alignof() > instead of a literal number. I had another fresh look today at this patch. The 32-bit padding is right after the field 'ready'. I can't for sure tell how the second half is going to ever be used and how. However, one possibility would be to extend the field 'ready' to 64-bit. With the current code, we could easily make a single 64-bit access without having to know whether the guest is able to interpret the top half. With your approach, we may need to have different path depending on the padding and ensure the new extension cannot be enabled if the padding is 4-byte. Otherwise, the atomicity would be broken. > While relaxation of the requirements is intended here, the primary goal > is to simply get rid of the hard coded number as well its lack of > connection to the structure that is is meant to apply to. Based on what I wrote above, I think the relaxation should not be done to give us more flexibility about possible extension to the structure. Although, I would be worth documenting the reasoning in the code. Cheers,
On 21.04.2021 21:52, Julien Grall wrote: > Hi, > > On 21/04/2021 15:36, Jan Beulich wrote: >> Neither the code nor the original commit provide any justification for >> the need to 8-byte align the struct in all cases. Enforce just as much >> alignment as the structure actually needs - 4 bytes - by using alignof() >> instead of a literal number. > > I had another fresh look today at this patch. The 32-bit padding is > right after the field 'ready'. > > I can't for sure tell how the second half is going to ever be used and how. > > However, one possibility would be to extend the field 'ready' to 64-bit. > With the current code, we could easily make a single 64-bit access > without having to know whether the guest is able to interpret the top half. I don't think extending field sizes is generally to be considered ABI- compatible. I also don't think we can re-use the field at all, as I couldn't find any checking of it being zero (input) or it getting set to zero (output). struct evtchn_init_control, which in principle could be a way to convey respective controlling flags, similarly has no room for extension, as its _pad[] also doesn't look to get checked anywhere. Jan > With your approach, we may need to have different path depending on the > padding and ensure the new extension cannot be enabled if the padding is > 4-byte. Otherwise, the atomicity would be broken. > >> While relaxation of the requirements is intended here, the primary goal >> is to simply get rid of the hard coded number as well its lack of >> connection to the structure that is is meant to apply to. > > Based on what I wrote above, I think the relaxation should not be done > to give us more flexibility about possible extension to the structure. > > Although, I would be worth documenting the reasoning in the code. > > Cheers, >
Hi Jan, On 22/04/2021 10:19, Jan Beulich wrote: > On 21.04.2021 21:52, Julien Grall wrote: >> Hi, >> >> On 21/04/2021 15:36, Jan Beulich wrote: >>> Neither the code nor the original commit provide any justification for >>> the need to 8-byte align the struct in all cases. Enforce just as much >>> alignment as the structure actually needs - 4 bytes - by using alignof() >>> instead of a literal number. >> >> I had another fresh look today at this patch. The 32-bit padding is >> right after the field 'ready'. >> >> I can't for sure tell how the second half is going to ever be used and how. >> >> However, one possibility would be to extend the field 'ready' to 64-bit. >> With the current code, we could easily make a single 64-bit access >> without having to know whether the guest is able to interpret the top half. > > I don't think extending field sizes is generally to be considered ABI- > compatible. I also don't think we can re-use the field at all, as I > couldn't find any checking of it being zero (input) or it getting set > to zero (output). That's would be fine so long we have a flag to control it. We can still write unconditionally because a guest can't rely on the pad... > struct evtchn_init_control, which in principle could > be a way to convey respective controlling flags, similarly has no room > for extension, as its _pad[] also doesn't look to get checked anywhere. Right, we would need to have a different way to convey. Yet, I am still unconvinced of the benefits change offer in this patch. I am not going to Nack. So another maintainer in "THE REST" can express support for your patch and ack it. Cheers,
--- a/xen/common/event_fifo.c +++ b/xen/common/event_fifo.c @@ -567,6 +567,16 @@ static void setup_ports(struct domain *d } } +#ifdef CONFIG_COMPAT + +#include <compat/event_channel.h> + +#define xen_evtchn_fifo_control_block evtchn_fifo_control_block +CHECK_evtchn_fifo_control_block; +#undef xen_evtchn_fifo_control_block + +#endif + int evtchn_fifo_init_control(struct evtchn_init_control *init_control) { struct domain *d = current->domain; @@ -586,19 +596,20 @@ int evtchn_fifo_init_control(struct evtc return -ENOENT; /* Must not cross page boundary. */ - if ( offset > (PAGE_SIZE - sizeof(evtchn_fifo_control_block_t)) ) - return -EINVAL; + if ( offset > (PAGE_SIZE - sizeof(*v->evtchn_fifo->control_block)) ) + return -ENXIO; /* * Make sure the guest controlled value offset is bounded even during * speculative execution. */ offset = array_index_nospec(offset, - PAGE_SIZE - sizeof(evtchn_fifo_control_block_t) + 1); + PAGE_SIZE - + sizeof(*v->evtchn_fifo->control_block) + 1); - /* Must be 8-bytes aligned. */ - if ( offset & (8 - 1) ) - return -EINVAL; + /* Must be suitably aligned. */ + if ( offset & (alignof(*v->evtchn_fifo->control_block) - 1) ) + return -ENXIO; spin_lock(&d->event_lock); --- a/xen/include/public/event_channel.h +++ b/xen/include/public/event_channel.h @@ -368,6 +368,11 @@ typedef uint32_t event_word_t; #define EVTCHN_FIFO_NR_CHANNELS (1 << EVTCHN_FIFO_LINK_BITS) +/* + * While this structure only requires 4-byte alignment, Xen versions 4.15 and + * earlier reject offset values (in struct evtchn_init_control) that aren't a + * multiple of 8. + */ struct evtchn_fifo_control_block { uint32_t ready; uint32_t _rsvd; --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -67,6 +67,7 @@ ? evtchn_bind_virq event_channel.h ? evtchn_close event_channel.h ? evtchn_expand_array event_channel.h +? evtchn_fifo_control_block event_channel.h ? evtchn_init_control event_channel.h ? evtchn_op event_channel.h ? evtchn_reset event_channel.h
Neither the code nor the original commit provide any justification for the need to 8-byte align the struct in all cases. Enforce just as much alignment as the structure actually needs - 4 bytes - by using alignof() instead of a literal number. While relaxation of the requirements is intended here, the primary goal is to simply get rid of the hard coded number as well its lack of connection to the structure that is is meant to apply to. Take the opportunity and also - add so far missing validation that native and compat mode layouts of the structures actually match, - tie sizeof() expressions to the types of the fields we're actually after, rather than specifying the type explicitly (which in the general case risks a disconnect, even if there's close to zero risk in this particular case), - use ENXIO instead of EINVAL for the two cases of the address not satisfying the requirements, which will make an issue here better stand out at the call site. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: Adjust version in public header comment. v2: Add comment to public header. Re-base. --- I question the need for the array_index_nospec() here. Or else I'd expect map_vcpu_info() would also need the same.