Re: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.

From: Evgenii Shatokhin
Date: Tue Apr 10 2018 - 09:57:19 EST


On 10.04.2018 16:21, Miroslav Benes wrote:

I think you're missing my point. This isn't about your patch set, per
se. It's really about our existing code. Today, our patch stack
doesn't follow real stack semantics, because patches in the middle might
be disabled. I see that as a problem.

No, please read it again. I wasn't talking about replaced patches.

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.

Do I understand it correctly that you do not like that the patches
on the stack might be in two states (disabled/enabled). This might
be translated that you do not like the state when the patch is
registered and disabled.

I wonder if the problem is in the "stack" abstraction. Would it help
if we call it "sorted list" or whatever more suitable?

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. Or we could rename
the operation do add/remove or anything else. In fact, this is how
it worked in kGraft.

I've already wondered couple of times why we had separate enable/disable.
If there is someone who knows, remind me, please. I wouldn't be against a
simplification here.

On the other hand, it is kind of nice to keep the registration and
enablement separate. It is more flexible if someone needs it.

Anyway, we should solve it together with the stacking. It is tightly
connected.
AFAIK, the enable/disabled state made more sense for immediate
patches that could not be unloaded. We still need to keep the patches
when the transaction is forced. The question is if we need to keep
the sysfs entries for loaded but unused patches.


If 'replace' were the only mode, then we wouldn't even need a patch
stack because it wouldn't really matter much whether the previous patch
is enabled or disabled. I think this is in agreement with the point
you're making.

But we still support non-replace patches. My feeling is that we should
either do a true stack, or no stack at all. The in-between thing is
going to be confusing, not only for us, but for patch authors and end
users.

I see it like two different modes. We either have a stack of patches
that depend on each other.

But if they depend on each other, they can use 'replace' and a stack
isn't needed.

Yes but see below.


And If they *don't* depend on each other, then the stack is overly
restrictive, for no good reason.

Either way, why do we still need a stack?

Good question. I suggest to agree on some terms first:

+ Independent patches make unrelated changes. Any new function
must not rely on changes done by any other patch.

+ Dependent patches mean that a later patch depend on changes
done by an earlier patch. For example, a new function might
use function added by an earlier patch.

+ Each cumulative patch include all necessary changes. I would say
that it is self-containing and independent. Except that they should
be able to continue using changes made by earlier patches (shadow
variables, callbacks).


Then we could say:

+ The stack helps to enforce dependencies between dependent
patches. But there is needed also some external solution
that forces loading the patches in the right order.

+ The "replace" feature is useful for cumulative patches.
It allows to remove existing changes and remove unused stuff.
But cumulative patches might be done even _without_ the atomic
replace.

+ Cumulative patches _with_ "replace" flag might be in theory
installed in random order because they handle not-longer patched
functions. Well, some incompatibility might be caused by shadow
variables and callbacks. Therefore it still might make sense
to install them in the right order.

Cumulative patches _without_ "replace" flag must be installed
in the right order because they do not handle not-longer patched
functions.

Anyway, for cumulative patches is important the external
ordering (loading modules) and not the stack.


Back to your question:

The stack is needed for dependent non-cumulative patches.

The cumulative patches with "replace" flag seems the be
the most safe and most flexible solution. The question is
if we want to force all users to use this mode.


Or we have replace patches that are
standalone and we allow a smooth transfer from one to another one.

Anyway, for us, it is much more important the removal of replaced
patches. We could probably live without the possibility to replace
disabled patches.

I think replacing disabled patches is ok, *if* we get rid of the
illusion of a stack. The current stack isn't really a stack, it's just
awkward.

I personally do not have problems with it. As I said, I see this as
two different modes how the life patches are distributed. The stack
is needed for dependent patches. The cumulative patches with
"replace" flag are self-contained and independent. They might
replace anything.

I agree here. Practically we use only cumulative replacement patches at
SUSE. So with that in mind I don't care about the stacking much. But, it
may make sense for someone else. Evgenii mentioned they used it for
hotfixes. Therefore I'm reluctant to remove it completely.

Well, it was convenient in some cases to provide a hot fix for a given bug on top of our official cumulative patch. So far, such fixes were only used on a few of the customers' machines (where they were needed ASAP). It just made it easier to see where is the common set of fixes and where is the customer-specific addition.

I think, we can use cumulative patches in such cases too without much additional effort. For example, we can encode the distinction (base set of fixes + addition) in the module name or somewhere else.

So, I think, it is fine for us, if stacking support is removed. Especially if that makes the implementation of livepatch less complex and more reliable.

Well, it would make sense to reduce the amount of possible
situations and use cases. The question is what is acceptable
to others and if it needs to be done as part of this patch set.

Yes. Input from actual users would be tremendously useful here.

Miroslav
.