Messages in this thread | | | Date | Wed, 11 Dec 2019 17:09:19 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH -tip 1/2] x86/alternative: Sync bp_patching update for avoiding NULL pointer exception |
| |
Hi Peter,
On Wed, 11 Dec 2019 01:09:43 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Dec 10, 2019 at 06:32:09PM +0100, Peter Zijlstra wrote: > > > I feel that is actually more complicated... Let me try to see if I can > > simplify things. > > How is this then?
This looks perfectly good to me :)
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you!
> > --- > arch/x86/kernel/alternative.c | 84 +++++++++++++++++++++++++++---------------- > 1 file changed, 53 insertions(+), 31 deletions(-) > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 30e86730655c..34360ca301a2 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -948,10 +948,29 @@ struct text_poke_loc { > const u8 text[POKE_MAX_OPCODE_SIZE]; > }; > > -static struct bp_patching_desc { > +struct bp_patching_desc { > struct text_poke_loc *vec; > int nr_entries; > -} bp_patching; > + atomic_t refs; > +}; > + > +static struct bp_patching_desc *bp_desc; > + > +static inline struct bp_patching_desc *try_get_desc(struct bp_patching_desc **descp) > +{ > + struct bp_patching_desc *desc = READ_ONCE(*descp); /* rcu_dereference */ > + > + if (!desc || !atomic_inc_not_zero(&desc->refs)) > + return NULL; > + > + return desc; > +} > + > +static inline void put_desc(struct bp_patching_desc *desc) > +{ > + smp_mb__before_atomic(); > + atomic_dec(&desc->refs); > +} > > static inline void *text_poke_addr(struct text_poke_loc *tp) > { > @@ -972,26 +991,26 @@ NOKPROBE_SYMBOL(patch_cmp); > > int notrace poke_int3_handler(struct pt_regs *regs) > { > + struct bp_patching_desc *desc; > struct text_poke_loc *tp; > + int len, ret = 0; > void *ip; > - int len; > + > + if (user_mode(regs)) > + return 0; > > /* > * Having observed our INT3 instruction, we now must observe > - * bp_patching.nr_entries. > + * bp_desc: > * > - * nr_entries != 0 INT3 > + * bp_desc = desc INT3 > * WMB RMB > - * write INT3 if (nr_entries) > - * > - * Idem for other elements in bp_patching. > + * write INT3 if (desc) > */ > smp_rmb(); > > - if (likely(!bp_patching.nr_entries)) > - return 0; > - > - if (user_mode(regs)) > + desc = try_get_desc(&bp_desc); > + if (!desc) > return 0; > > /* > @@ -1002,16 +1021,16 @@ int notrace poke_int3_handler(struct pt_regs *regs) > /* > * Skip the binary search if there is a single member in the vector. > */ > - if (unlikely(bp_patching.nr_entries > 1)) { > - tp = bsearch(ip, bp_patching.vec, bp_patching.nr_entries, > + if (unlikely(desc->nr_entries > 1)) { > + tp = bsearch(ip, desc->vec, desc->nr_entries, > sizeof(struct text_poke_loc), > patch_cmp); > if (!tp) > - return 0; > + goto out_put; > } else { > - tp = bp_patching.vec; > + tp = desc->vec; > if (text_poke_addr(tp) != ip) > - return 0; > + goto out_put; > } > > len = text_opcode_size(tp->opcode); > @@ -1023,7 +1042,7 @@ int notrace poke_int3_handler(struct pt_regs *regs) > * Someone poked an explicit INT3, they'll want to handle it, > * do not consume. > */ > - return 0; > + goto out_put; > > case CALL_INSN_OPCODE: > int3_emulate_call(regs, (long)ip + tp->rel32); > @@ -1038,7 +1057,11 @@ int notrace poke_int3_handler(struct pt_regs *regs) > BUG(); > } > > - return 1; > + ret = 1; > + > +out_put: > + put_desc(desc); > + return ret; > } > NOKPROBE_SYMBOL(poke_int3_handler); > > @@ -1069,14 +1092,18 @@ static int tp_vec_nr; > */ > static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries) > { > + struct bp_patching_desc desc = { > + .vec = tp, > + .nr_entries = nr_entries, > + .refs = ATOMIC_INIT(1), > + }; > unsigned char int3 = INT3_INSN_OPCODE; > unsigned int i; > int do_sync; > > lockdep_assert_held(&text_mutex); > > - bp_patching.vec = tp; > - bp_patching.nr_entries = nr_entries; > + smp_store_release(&bp_desc, &desc); /* rcu_assign_pointer */ > > /* > * Corresponding read barrier in int3 notifier for making sure the > @@ -1131,17 +1158,12 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries > text_poke_sync(); > > /* > - * sync_core() implies an smp_mb() and orders this store against > - * the writing of the new instruction. > + * Remove and synchronize_rcu(), except we have a very primitive > + * refcount based completion. > */ > - bp_patching.nr_entries = 0; > - /* > - * This sync_core () call ensures that all INT3 handlers in progress > - * have finished. This allows poke_int3_handler() after this to > - * avoid touching bp_paching.vec by checking nr_entries == 0. > - */ > - text_poke_sync(); > - bp_patching.vec = NULL; > + WRITE_ONCE(bp_desc, NULL); /* RCU_INIT_POINTER */ > + if (!atomic_dec_and_test(&desc.refs)) > + atomic_cond_read_acquire(&desc.refs, !VAL); > } > > void text_poke_loc_init(struct text_poke_loc *tp, void *addr,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |