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:   Tue, 12 Feb 2019 12:11:28 +0530
From:   Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
To:     Josh Poimboeuf <jpoimboe@redhat.com>
Cc:     live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Miroslav Benes <mbenes@suse.cz>,
        Petr Mladek <pmladek@suse.com>, Jiri Kosina <jikos@kernel.org>
Subject: Re: [PATCH] livepatch: Enforce reliable stack trace as config
 dependency
References: <20190209091728.23046-1-kamalesh@linux.vnet.ibm.com>
 <20190211140813.z7kap634gz3gp6a4@treble>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190211140813.z7kap634gz3gp6a4@treble>
User-Agent: Mutt/1.10.1 (2018-07-13)
Message-Id: <20190212064128.GA20554@JAVRIS.in.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 11, 2019 at 08:08:13AM -0600, Josh Poimboeuf wrote:
> On Sat, Feb 09, 2019 at 02:47:28PM +0530, Kamalesh Babulal wrote:
> > After removal of the immediate flag by commit d0807da78e11
> > ("livepatch: Remove immediate feature"), reliable stack trace became
> > enforcing dependency for livepatch support on any architecture. In
> > the current code, we ensure that the dependency is met when
> > enabling the patch during the module load.
> > 
> > This dependency check can be improved by moving it to the Kconfig,
> > which disables the support for livepatching in the kernel for unmet
> > dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
> > STACKTRACE under config LIVEPATCH, it also removes the
> > klp_have_reliable_stack() function.
> > 
> > Loading a livepatching module on an architecture where reliable
> > stack trace is yet to be implemented, the user should see:
> > 
> > insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module format
> > 
> > ...
> > [  286.453463] livepatch_sample: module is marked as livepatch module, but livepatch support is disabled
> 
> Wouldn't the module fail to build in the first place, since
> CONFIG_LIVEPATCH is disabled?

Yes, with this patch applied, the livepatch modules will fail to build.
The intention was to paste the output of a module load, marked as the
livepatch but without the reliable stack trace support. I used the
previously compiled livepatch sample module for the illustration but
yeah we would not have a functioning module build in the first place.

> 
> Anyway, I'm not sure about this approach.  This patch makes the s390
> livepatch code no longer compilable, turning it into completely dead
> code.  So if something changes in the s390 code which causes it to stop
> compiling, nobody will notice.

Fair point, we can drop this patch in favor of getting compile time
testing for s390.

> 
> I think I'd rather go in the opposite direction: allow the patches to be
> loaded.  Then they can be forced, if needed.  That enables both compile
> and runtime testing.  That way we don't make any backward progress,
> until such arches get reliable stacktraces.

klp_have_reliable_stack() enforces implementation of reliable
stack trace and run time testing will not be complete with we returning
after the check.  We can re-think about enforcing the dependency after
we have the reliable stack trace for s390.

-- 
Kamalesh