On 17.09.2021 10:45, Andrew Cooper wrote: > --- a/xen/common/trace.c > +++ b/xen/common/trace.c > @@ -686,22 +686,21 @@ void __trace_var(u32 event, bool_t cycles, unsigned int > extra, > unsigned long flags; > u32 bytes_to_tail, bytes_to_wrap; > unsigned int rec_size, total_size; > - unsigned int extra_word; > bool_t started_below_highwater; > > if( !tb_init_done ) > return; > > - /* Convert byte count into word count, rounding up */ > - extra_word = (extra / sizeof(u32)); > - if ( (extra % sizeof(u32)) != 0 ) > - extra_word++; > - > - ASSERT(extra_word <= TRACE_EXTRA_MAX); > - extra_word = min_t(int, extra_word, TRACE_EXTRA_MAX); > - > - /* Round size up to nearest word */ > - extra = extra_word * sizeof(u32); > + /* > + * Trace records require extra data which is an exact multiple of > + * uint32_t. Reject out-of-spec records. Any failure here is an error > in > + * the caller. > + */
Hmm, is "require" accurate? They may very well come without extra data afaics. > + if ( extra % sizeof(uint32_t) || > + extra / sizeof(uint32_t) > TRACE_EXTRA_MAX ) > + return printk_once(XENLOG_WARNING > + "Trace event %#x bad size %u, discarding\n", > + event, extra); Any HVM guest looks to be able to trivially trigger this log message (via HVMOP_xentrace), thus pointing out an issue in a guest and hiding any other Xen related output. I'd like to suggest to adjust that call site in prereq patch (I'm not overly fussed which of the two relatively obvious ways). Further sched/rt.c:burn_budget() has a bool field last in a packed struct, yielding a sizeof() that's not a multiple of 4. All the uses of __packed there look at best suspicious anyway. Jan
