Re: Revisiting patch dependencies

[Minfei Huang <mnfhuang@gmail.com>]

On 07/23/15 at 01:07pm, Josh Poimboeuf wrote:
> 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.

Thanks for your explaination.

Yes, kernel may crash, if module M calls the unpatched and exported
function vmlinux_func.

I may prefer to choice B, since user can make their own rule to restrict
the patches enabled/disabled. Thus livepatch may be simplier in code
layer.

Thanks
Minfei