| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 Dec 2019 17:09:19 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>, x86@...nel.org,
linux-kernel@...r.kernel.org, bristot@...hat.com,
jbaron@...mai.com, torvalds@...ux-foundation.org,
tglx@...utronix.de, namit@...are.com, hpa@...or.com,
luto@...nel.org, ard.biesheuvel@...aro.org, jpoimboe@...hat.com,
jeyu@...nel.org, alexei.starovoitov@...il.com
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@...radead.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@...nel.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@...nel.org>
Powered by blists - more mailing lists