Petr Mladek <pmladek@xxxxxxxx> writes:
> On Fri 2019-02-15 11:58:33, Nicolai Stange wrote:
>> Petr Mladek <pmladek@xxxxxxxx> writes:
>>
>> > On Thu 2019-02-14 11:47:42, Nicolai Stange wrote:
>> >> Joe Lawrence <joe.lawrence@xxxxxxxxxx> writes:
>> > 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)
>>
>>
>> Ok. The only thing which remains unclear to me is how a live patch
>> announces the set of supported features to the klp core?
>
> We could add a callback is_feature_supported(). Something like:
>
> bool is_feature_supported(int id, int version)
> {
> if (id == L1TF && version <= 1)
> return true;
>
> if (id == BLA_BLA && version <= 2)
> return true;
>
> return false;
> }
>
> The livepatch core need to have somewhere the list
> of existing features (to implement klp_get_feature()).
> It might check if all the existing features are supported
> by calling the above callback from the new patch.
>
> Any better idea is welcome.
>
I don't have any, except for the obvious "array of (feature id, version)
pairs embedded into struct klp_patch".
But note that this ->is_feature_supported() API you're proposing
probably won't be able to handle the non-cumulative patch case we
discussed below ...
>> >> > - 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.
>>
>> Hmm, I think we could require the versions to be >= what is currently
>> there, but only for the features the new non-cumulative live patch
>> supports?
>
> Good point.
... because the KLP core would have no way of getting the list of
features supported by the new non-cumulative live patch, right?
That said, I don't care much about non-cumulative live patches.
>
>> I.e, for cumulative live patches, it would be: "all features currently
>> registered must be provided by a new cumulative live patch in the same
>> or higher version".
>>
>> And for non-cumulative ones: "all features supported by a new
>> non-cumulative live patch must be provided in the same or higher version
>> than what's currently registered on the system".
>
> This makes perfect sense.
>
>
>> >> 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.
>> >
>>
>> I don't care too much whether or not the stickiness is managed per-patch
>> or or'ed together from individual features.
>>
>> But I think what is important is how the feature versioning affects
>> the reversability of transitions.
>>
>> That is, as we won't allow a transision to a sticky patch to be
>> reversed, we probably also don't want to allow a transition to a patch
>> with a feature of a higher version to be reversed -- just to be
>> consistent there?
>
> It depends. The new livepatch must be able to migrate the feature
> to a higher version. In many cases, the backward migration might
> be easy, especially when the real work is done in post_patch()
> callbacks.
>
> We could implement:
>
> void klp_disable_revert(void)
>
> that might get called when the revert is not supported.
Just stating the obvious: we probably don't want to call this
klp_disable_revert() from any callback if the feature in question can be
found at the same version in the previous live patch. (And likewise, we
probably also don't want to any "real work" from post_patch(), because
everything has been done already when the previous live patch got applied).
So the next live patch's callbacks must be able to examine the feature
version from the previous live patch. But I guess that won't be a
problem with your shadow-like klp_feature_get() API?
> I could imagine that some vendors would always call this just to make
> their life easier.
>
>> >> 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.
>>
>> Indeed :)
>>
>> Ignore it, I just wanted to remark that static klp_features could have
>> the potential to get reused/extended in the future. But that should not
>> concern us now -- aplogies for the confusion.
>>
>> <digression>
>>
>> > I wonder what are the problems to create a /sys interface that might
>> > get trasfered between livepatches.
>>
>> Ok, in the L1TF kvm example, there's none.
>>
>> But there are cases where I'd prefer to introduce my own, live-patch
>> specific control knob not having any equivalent in upstream. Take the
>> mincore() patches currently under discussion. They slightly change
>> behaviour and it would be nice imo if the users could opt-in for that.
>
> I see.
>
>> And it would be great, if this delayed enabling of the mincore()
>> live-subpatch could happen with full consistency-model guarantees:
>> writing '1' to
>> /sys/.../foo-patch/mincore-cve-feature/enable
>> would trigger a full transition for the sub-patch instead of only
>> flipping some boolean flag.
>
> Huh, as I said, this is another level of complexity. And I am not
> convinced that it is even reasonable:
>
> + If the full kernel update does not allow to disable a feature.
> Why a livepatch should be more clever? It might always get
> fixed by the next livepatch.
>
> + Most livepatch fixes "just copy" the updated upstream
> code. Any livepatch specific-code might add livepatch-specific
> bugs. Livepatch users are very conservative. So should be
> the livepatch authors.
I'll stop arguing for that now ;)
Thanks,
Nicolai
--
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)
