From: Peter Zijlstra Sent: November 11, 2018 at 11:52:20 PM GMT > To: Nadav Amit <na...@vmware.com> > Cc: Ingo Molnar <mi...@redhat.com>, LKML <linux-kernel@vger.kernel.org>, X86 > ML <x...@kernel.org>, H. Peter Anvin <h...@zytor.com>, Thomas Gleixner > <t...@linutronix.de>, Borislav Petkov <b...@alien8.de>, Dave Hansen > <dave.han...@linux.intel.com>, Andy Lutomirski <l...@kernel.org>, Kees Cook > <keesc...@chromium.org>, Dave Hansen <dave.han...@intel.com>, Masami > Hiramatsu <mhira...@kernel.org> > Subject: Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text > poking > > > On Sun, Nov 11, 2018 at 08:53:07PM +0000, Nadav Amit wrote: > >>>> + /* >>>> + * The lock is not really needed, but this allows to avoid open-coding. >>>> + */ >>>> + ptep = get_locked_pte(poking_mm, poking_addr, &ptl); >>>> + >>>> + /* >>>> + * If we failed to allocate a PTE, fail. This should *never* happen, >>>> + * since we preallocate the PTE. >>>> + */ >>>> + if (WARN_ON_ONCE(!ptep)) >>>> + goto out; >>> >>> Since we hard rely on init getting that right; can't we simply get rid >>> of this? >> >> This is a repeated complaint of yours, which I do not feel comfortable with. >> One day someone will run some static analysis tool and start finding that >> all these checks are missing. >> >> The question is why do you care about them. > > Mostly because they should not be happening, ever. And if they happen, > there really isn't anything sensible we can do about it. > >> If it is because they affect the >> generated code and make it less efficient, I can fully understand and perhaps >> we should have something like PARANOID_WARN_ON_ONCE() which compiles into >> nothing >> unless a certain debug option is set. >> >> If it is about the way the source code looks - I guess it doesn’t sore my >> eyes as hard as some other stuff, and I cannot do much about it (other than >> removing it as you asked). > > And yes on the above two points. It adds both runtime overhead (albeit > trivially small) and code complexity.
I understand. So the question is - what would you prefer: something like PARANOID_WARN_ON_ONCE() or should I just remove the assertion? >>>> +out: >>>> + if (memcmp(addr, opcode, len)) >>>> + r = -EFAULT; >>> >>> How could this ever fail? And how can we reliably recover from that? >> >> This code has been there before (with slightly uglier code). Before this >> patch, a BUG_ON() was used here. However, I noticed that kgdb actually >> checks that text_poke() succeeded after calling it and gracefully fail. >> However, this was useless, since text_poke() would panic before kgdb gets >> the chance to do anything (see patch 7). > > Yes, I know it was there before, and I did see kgdb do it too. But aside > from that out-label case, which we also should never hit, how can we > realistically ever fail that memcmp()? > > If we fail here, something is _seriously_ buggered. I agree. But as it may be useful at least to warn in such a case, as debugging of SMC/CMC is hard. For example, if there is some sort of a race between module (un)loading and static-keys - such a check might be beneficial to indicate so. Having said that, changing it into VM_BUG_ON() or something similar may make more sense. Personally, I don’t care much - I’m just worried that I made some intrusive changes *and* you want me to remove the assertion that checks that I didn’t screw up.
