Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 12 Nov 2018 00:52:20 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Nadav Amit <namit@vmware.com>
Cc:     Ingo Molnar <mingo@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Masami Hiramatsu <mhiramat@kernel.org>
Subject: Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text
 poking
Message-ID: <20181111235220.GB3056@worktop>
References: <20181110231732.15060-1-namit@vmware.com>
 <20181110231732.15060-7-namit@vmware.com>
 <20181111145936.GA3021@worktop>
 <43C8C690-F079-4513-82CD-46A62DBA5A3B@vmware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <43C8C690-F079-4513-82CD-46A62DBA5A3B@vmware.com>
User-Agent: Mutt/1.5.22.1 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

> >> +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.