Messages in this thread | | | Date | Mon, 2 Dec 2019 23:39:27 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH -tip 1/2] x86/alternative: Sync bp_patching update for avoiding NULL pointer exception |
| |
On Mon, 2 Dec 2019 14:43:54 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Dec 02, 2019 at 08:50:12PM +0900, Masami Hiramatsu wrote: > > On Mon, 2 Dec 2019 10:15:19 +0100 > > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Wed, Nov 27, 2019 at 02:56:52PM +0900, Masami Hiramatsu wrote: > > > > > --- a/arch/x86/kernel/alternative.c > > > > +++ b/arch/x86/kernel/alternative.c > > > > @@ -1134,8 +1134,14 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries > > > > * sync_core() implies an smp_mb() and orders this store against > > > > * the writing of the new instruction. > > > > */ > > > > - bp_patching.vec = NULL; > > > > bp_patching.nr_entries = 0; > > > > + /* > > > > + * This sync_core () 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; > > > > } > > > > > > Hurm.. is there no way we can merge that with the 'last' > > > text_poke_sync() ? It seems a little daft to do 2 back-to-back IPI > > > things like that. > > > > Maybe we can add a NULL check of bp_patchig.vec in poke_int3_handler() > > but it doesn't ensure the fundamental safeness, because the array > > pointed by bp_patching.vec itself can be released while > > poke_int3_handler() accesses it. > > No, what I mean is something like: > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 30e86730655c..347a234a7c52 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -1119,17 +1119,13 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries > * Third step: replace the first byte (int3) by the first byte of > * replacing opcode. > */ > - for (do_sync = 0, i = 0; i < nr_entries; i++) { > + for (i = 0; i < nr_entries; i++) { > if (tp[i].text[0] == INT3_INSN_OPCODE) > continue; > > text_poke(text_poke_addr(&tp[i]), tp[i].text, INT3_INSN_SIZE); > - do_sync++; > } > > - if (do_sync) > - text_poke_sync(); > - > /* > * sync_core() implies an smp_mb() and orders this store against > * the writing of the new instruction. > > > Or is that unsafe ?
OK, let's check it.
text_poke_bp_batch() { update vec update nr_entries smp_wmb() write int3 text_poke_sync() write rest_bytes text_poke_sync() if rest_bytes write first_byte text_poke_sync() if first_byte ... (*) update nr_entries text_poke_sync() ... (**) update vec }
Before (*), the first byte can be new opcode or int3, thus poke_int3_handler() can be called. But anyway, at that point nr_entries != 0, thus poke_int3_handler() correctly emulate the new instruction.
Before (**), all int3 should be removed, so nr_entries must not accessed, EXCEPT for writing int3 case.
If we just remove the (*) as you say, the poke_int3_handler() can see nr_entries = 0 before (**). So it is still unsafe.
I considered another way that skipping (**) if !first_byte, since (*) ensured the target address(text) doesn't hit int3 anymore. However, this will be also unsafe because there can be another int3 (by kprobes) has been hit while updating nr_entries and vec.
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |