Date: Thu, 23 Jul 2015 13:07:02 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Minfei Huang <mhuang@redhat.com>
Cc: sjenning@redhat.com, jkosina@suse.cz, vojtech@suse.cz,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Minfei Huang <mnfhuang@gmail.com>
Subject: Revisiting patch dependencies
Message-ID: <20150723180702.GA3641@treble.redhat.com>
References: <1436950506-5252-1-git-send-email-mhuang@redhat.com>
 <20150722144004.GC23235@treble.redhat.com>
 <20150723040206.GA18136@dhcp-128-25.nay.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20150723040206.GA18136@dhcp-128-25.nay.redhat.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3047
Lines: 70

On Thu, Jul 23, 2015 at 12:02:06PM +0800, Minfei Huang wrote:
> On 07/22/15 at 09:40am, Josh Poimboeuf wrote:
> > Is it really safe to assume that there are no dependencies between
> > patches which patch different objects?
> > 
> 
> I think so.

What about the following scenario:

1. register and enable patch A, which patches vmlinux_func() and changes
   its call signature
2. register and enable patch B, which patches a (not yet loaded) module
   M so that it will call vmlinux_func() with its new call signature
3. load module M, which is immediately patched by patch B
4. disable patch A.  Now the patched module M calls the unpatched
   version of vmlinux_func() with the wrong call signature - BOOM

In this case B, a patch to a module, would have an implicit dependency
on A, a patch to vmlinux.

So I don't think the approach in the above patch would work.  But I *do*
think we may need to revisit how we handle dependencies...

Note that our current patch stacking protects against unloading out of
order, but it assumes that the user loaded them in the correct order in
the first place.  If M and B are loaded before A, then it would still go
boom even with today's code.

So IMO the way we handle dependencies today is incomplete.  Some options
for improvement are:

a) Don't allow dependencies between patches.  Instead all dependencies
   must be contained within the patch itself.  So patch A and patch B
   are combined into a single patch AB.  If, later, a new patch C is
   needed, which also depends on A, then create a new cumulative patch
   ABC which replaces AB.

   Note there's no way to enforce the fact there are no dependencies,
   because they can be hidden.  So it would just have to be a documented
   rule that the patch author must follow, as part of the (yet to be
   written) patch creation guidelines.  This actually isn't a big deal
   because there are several other (still undocumented) rules the patch
   author must already follow.

   This would mean that klp code can assume there are no dependencies,
   and so patch stacking would no longer be necessary.  We'd probably
   have to rework the ops->func_stack code a bit so that it's ordered by
   when the patches were registered instead of when they were enabled,
   so that disabling and re-enabling an older patch wouldn't override a
   newer cumulative one which replaces it.

b) Create a way for the patch author to specify explicit patch
   dependencies.

Note that both options a and b delegate responsibility to the patch
author to ensure that dependencies are handled appropriately.
Ultimately I don't think there's any way around that.

My vote would be option a for now, by removing patch stacking and
documenting the guidelines.  With the eventual possibility of adding b
if needed.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/