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 -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
