| Message ID | 5CD2D489020000780022CD34@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: IRQ management adjustments | expand |
On Wed, May 08, 2019 at 07:07:21AM -0600, Jan Beulich wrote: > Dynamically allocated CPU mask objects may be smaller than cpumask_t, so > copying has to be restricted to the actual allocation size. This is > particulary important since the function doesn't bail early when tracing > is not active, so even production builds would be affected by potential > misbehavior here. > > Take the opportunity and also > - use initializers instead of assignment + memset(), > - constify the cpumask_t input pointer, > - u32 -> uint32_t. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
On 5/8/19 2:07 PM, Jan Beulich wrote: > Dynamically allocated CPU mask objects may be smaller than cpumask_t, so > copying has to be restricted to the actual allocation size. This is > particulary important since the function doesn't bail early when tracing > is not active, so even production builds would be affected by potential > misbehavior here. > > Take the opportunity and also > - use initializers instead of assignment + memset(), > - constify the cpumask_t input pointer, > - u32 -> uint32_t. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: New. > --- > TBD: I wonder whether the function shouldn't gain an early tb_init_done > check, like many other trace_*() have. Yeah, avoiding these memcopies when tracing is not enabled seems like a good thing. Either way: Acked-by: George Dunlap <george.dunlap@citrix.com> > > George, despite your general request to be copied on entire series > rather than individual patches, I thought it would be better to copy > you on just this one (for its tracing aspect), as the patch here is > independent of the rest of the series, but at least one later patch > depends on the parameter constification done here. Yes, I think in this case this was the easiest thing for me. Thanks. :-) -George
>>> On 13.05.19 at 12:42, <george.dunlap@citrix.com> wrote: > On 5/8/19 2:07 PM, Jan Beulich wrote: >> TBD: I wonder whether the function shouldn't gain an early tb_init_done >> check, like many other trace_*() have. > > Yeah, avoiding these memcopies when tracing is not enabled seems like a > good thing. I've taken note to submit a respective follow-on patch. > Either way: > > Acked-by: George Dunlap <george.dunlap@citrix.com> Thanks, Jan
--- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -104,16 +104,19 @@ static inline bool valid_irq_vector(unsi return vector >= FIRST_DYNAMIC_VECTOR && vector <= LAST_HIPRIORITY_VECTOR; } -static void trace_irq_mask(u32 event, int irq, int vector, cpumask_t *mask) +static void trace_irq_mask(uint32_t event, int irq, int vector, + const cpumask_t *mask) { struct { unsigned int irq:16, vec:16; unsigned int mask[6]; - } d; - d.irq = irq; - d.vec = vector; - memset(d.mask, 0, sizeof(d.mask)); - memcpy(d.mask, mask, min(sizeof(d.mask), sizeof(cpumask_t))); + } d = { + .irq = irq, + .vec = vector, + }; + + memcpy(d.mask, mask, + min(sizeof(d.mask), BITS_TO_LONGS(nr_cpu_ids) * sizeof(long))); trace_var(event, 1, sizeof(d), &d); }
Dynamically allocated CPU mask objects may be smaller than cpumask_t, so copying has to be restricted to the actual allocation size. This is particulary important since the function doesn't bail early when tracing is not active, so even production builds would be affected by potential misbehavior here. Take the opportunity and also - use initializers instead of assignment + memset(), - constify the cpumask_t input pointer, - u32 -> uint32_t. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New. --- TBD: I wonder whether the function shouldn't gain an early tb_init_done check, like many other trace_*() have. George, despite your general request to be copied on entire series rather than individual patches, I thought it would be better to copy you on just this one (for its tracing aspect), as the patch here is independent of the rest of the series, but at least one later patch depends on the parameter constification done here.