Re: [Xen-devel] [PATCH v2 03/12] x86/IRQ: avoid UB (or worse) in trace_irq_mask()
>>> On 13.05.19 at 12:42, 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 Thanks, Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 03/12] x86/IRQ: avoid UB (or worse) in trace_irq_mask()
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 > --- > 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, 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 03/12] x86/IRQ: avoid UB (or worse) in trace_irq_mask()
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 Reviewed-by: Roger Pau Monné Thanks. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 03/12] x86/IRQ: avoid UB (or worse) in trace_irq_mask()
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
---
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.
--- 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), );
}
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
