On Thu 2019-02-14 11:47:42, Nicolai Stange wrote:
> Joe Lawrence <joe.lawrence@xxxxxxxxxx> writes:
> > - Do features need to be tightly coupled to callbacks? They are a
> > convenient tool to implement a policy, but I wonder if we lose
> > anything by moving this up to the klp_patch level and letting the core
> > handle it. (Like the versioning scheme.)
>
> If I'm understanding you correctly, you wonder whether having a
> klp_object member in klp_feature would be necessary or not, right?
>
> I think it doesn't matter much in these "safe upgrade" scenarios
> being discussed here.
The motivation to store the object pointer was to get an easy access
to the callbacks from the old patch. Heh, I did this when I thought
about the API on the Plumbers conference and do not remember the use
case any longer.
Hmm, it might allow to obsolete a feature using pre_unpatch()
and post_unpatch() callbacks from the old patch in pre-patch()
and post_patch() callback of the new cumulative patch.
Now, I think that it would actually complicate to access the same
feature from different objects.
> I have to admit though that I don't see the need for having the
> klp_features dynamic or "shadow variable like": I think we could embed
> those statically into klp_patch as well.
I see the following advantages of the dynamic approach:
+ easy to transfer (it just stays in the memory, some list)
+ Localized code (alloc/get only where needed, no need to
define it in some global structures, lists).
+ One structure for a single feature all the time. No confusion
which structure you work with. Temporary states might always
be stored in local static variables and stored into
the structure once the feature is fully migrated.
I could imagine that some people might see this as
a disadvantage though.
The disadvantages are:
+ allocation might fail (must be done in pre_patch())
+ more complicated compatibility check (another callback
to check provided features)
> > Features could still be stored in a list and the callbacks could get
> > access to current vs. this patch's feature set. They wouldn't be
> > dynamic, but then the API supporting that could drop out.
> Right, so during a transition, access to klp_features from the previous
> live patch would be possible only up to and including the pre-patch
> callback, correct? That would probably be enough to "take over" in all
> situations. If the next live patch is sticky, ->data ownership wouldn't
> be a problem, because, as Petr said in reply to the stickiness patch,
> the transition should be non-reversable in this case. If we're dealing
> with non-sticky live patches, the post-unpatch callback would only get
> called if the live patch hasn't been superseeded by another one (thanks
> to the atomic replace patchset) -- so we could teardown any ->data from
> there.
Anyway, this is another motivation to remove object pointer
from the feature structure. By other words, the dynamic structure
should not point to any static code that might get later removed
(because it is error prone).
> > - For follow on livepatches, would their feature set need to match or
> > exceed the values for all of the current features, or only the
> > features that the new patch is advertising to support? For example,
> > if we have current features:
> >
> > vmlinux : { { L1TF, v1 }, { CVE2, v2 } }
> >
> > should the incoming cumulative patch need L1TF >= v1 and CVE2 >= v2 ?
> > Or does it only need L1TF >= v1 *or* CVE2 >= v2?
> >
> > Dunno if cumulative livepatches change that requirement or not.
>
> I have to revisit the semantics of non-cumulative live patches
> after atomic replace, but I think that if the one to get applied is
> cumulative, it's an "and" and for those which are non-cumulative it
> could perhaps be an "or".
Good question. Well, I think that non-cumulative patches will not
need to check anything. They are primary used to add unrelated fixes.
They might theoretically take over or extend an existing feature
but then the authors/users is on their own shifting sands.
> Which brings another question: similar to the "sticky" case preventing
> unpatching, we probably shouldn't allow the the transition to get
> reversed back to live patches not providing a particular feature at all
> or only in a lower version?
>
> Perhaps it would make sense to make stickiness a klp_feature property?
It might be unclear with patch is sticky with the dynamically
allocated structures. It might work with dynamically allocated
structures.
The question is if it is worth the complexity. It is a global
state. I wonder if any callback would ever want to disable
the sticky flag once set.
> Let me throw in a half-baked idea/dream of mine completely unrelated to
> changing global semantics, safe upgrade paths and so on.
>
> >From time to time, the upstream mitigation for a certain CVE involves
> new control knobs. For example, in the L1TF kvm case, there had been a
> new option (whose name I forgot) to disable hyperthreading. The live
> patch author currently has no standardized way of providing such a
> control knob to live patching users and thus, all he/she can do about it is
> to apply the somewhat coarse policy of "security over performance".
> There are other examples for similar cases, which I forgot about now.
>
> Long story short, it would be nice if we could, for certain subpatches
> or "features" or however we call it, ask the user to enable them
> explictly through some /sys/.../enable file.
Huh, this is another level of complexity. I wonder what are the
problems to create a /sys interface that might get trasfered
between livepatches. This would allow to create the same
interface that is used by the upstream implementation.
Best Regards,
Petr
