On Wed, 11 Apr 2018, Josh Poimboeuf wrote: > On Wed, Apr 11, 2018 at 10:07:31AM +0200, Miroslav Benes wrote: > > > > I was confused by wording "in the middle". It suggested that there > > > > might had been enabled patches on the top and the bottom of the stack > > > > and some disabled patches in between at the same time (or vice versa). > > > > This was not true. > > > > > > That *was* what I meant. Consider the following sequence of events: > > > > > > - Register patch 1 > > > - Enable patch 1 > > > - Register patch 2 > > > - Enable patch 2 > > > - Disable patch 2 > > > - Register patch 3 > > > - Enable patch 3 > > > > > > Notice that patch 2 (in the middle) is disabled, whereas patch 1 (on the > > > bottom) and patch 3 (on the top) are enabled. > > > > This should not be possible at all. > > > > __klp_enable_patch: > > > > if (patch->list.prev != &klp_patches && > > !list_prev_entry(patch, list)->enabled) > > return -EBUSY; > > > > When patch 3 is enabled, list_prev_entry() returns patch 2 and its > > ->enabled is false. > > Hm, you're right. I'm not sure how I got that idea... > > I still agree with my original conclusion that enforcing stack order no > longer makes sense though. Frankly I cannot say. I have got no opinion on this, so if there is a patch to remove it, I won't oppose it (probably). I just think it is connected to the atomic replace patch set. > > > > Another possibility would be to get rid of the enable/disable states. > > > > I mean that the patch will be automatically enabled during > > > > registration and removed during unregistration. > > > > > > I don't see how disabling during unregistration would be possible, since > > > the unregister is called from the patch module exit function, which > > > can only be called *after* the patch is disabled. > > > > > > However, we could unregister immediately after disabling (i.e., in > > > enabled_store context). > > > > I think this is what Petr meant. So there would be nothing in the patch > > module exit function. Well, not exactly. We'd need to remove sysfs dir and > > maybe something more. > > Sounds good to me, though aren't the livepatch sysfs entries removed by > klp during unregister? Yes. I was thinking we may take something out of disable+unregister step and leave it in the exit function, but maybe there is nothing like that. > > > > The question is what is acceptable to others > > > > > > If there are any objections, this is their chance to speak up :-) > > > > > > > and if it needs to be done as part of this patch set. > > > > > > Maybe so, for at least a few reasons: > > > > > > - This patch set makes the 'stack' obsolete, so it makes sense to remove > > > the 'stack' with it. > > > > Not necessarily. I like Petr's rebase explanation here. > > I'm not sure what you mean. IIRC, his rebase explanation referred to > how we handle 'replace' patches, for which there is no stacking (as I > meant the term: enforcement of stack order for registration and > enablement). See above. I don't completely agree with the obsoleteness part. I don't see it that way. But it is moot. I'm not against the enforcement removal. Regards, Miroslav -- 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
