On Wed 2019-02-13 17:27:40, Joe Lawrence wrote:
> On 2/13/19 5:12 AM, Petr Mladek wrote:
> > My idea is to create something similar to the shadow variables API.
> > Something like:
> >
> > struct klp_feature {
> > /* feature ID, e.g. #define L1TF 1 */
> > int id;
> > /* Version of the code */
> > int version;
> > /* object + callbacks assiciated with the feature */
> > struct klp_object *obj;
> > /* optional data */
> > void *data;
> > /* list of available features */
> > struct list_head node;
> > */
> >
> >
> > Note that it would be possible to add id, version, data to
> > struct klp_callbacks.
> >
> > But more practical would be to create it dynamically.
> > Then one callback might support more features. Also
> > It is less error-prone for porting because everyhing
> > is self-contained in the callback code. Finally,
> > it makes it easier to take over the state by another
> > cummulative livepatch.
> >
> >
> > Now the callbacks:
> >
> > struct klp_feature *klp_feature_alloc(id, version, obj, data);
> > void klp_feature_free(feature);
> >
> > struct klp_feature *klp_feature_get(id);
> > struct klp_feature *klp_feature_get(id);
> >
> >
> > Then the callback might do something like:
> >
> > #define L1TF 1
> > #define L1TF_VERSION 1
> >
> >
> > int klp_pre_patch_callback_l1tf(struct klp_object *obj)
> > {
> > struct klp_feature *feature;
> >
> > if (klp_feature_get(L1TF))
> > return 0;
> >
> > /* this is the first patch doing L1TF */
> > feature = klp_feature_alloc(L1TF, L1TF_VERSION, obj, NULL);
> > if (!feature)
> > return -ENOMEM;
> > }
> >
> > void klp_post_callback_l1tf(struct klp_object *obj)
> > {
> > struct klp_feature *feature;
> >
> > feature = klp_get_feature(L1TF);
> > if (!feature) {
> > WARN("This should never happen\n);
> >
> > if (feature->obj != obj && old_feature->version == L1TF_VERSION)
> > {
> > /* just take over */
> > old_feature->obj = obj;
> > return;
> > }
> >
> > # enable L1TF mitigation
> > l1tf_enable();
> > }
> >
> > void klp_pre_unpatch_callback(struct klp_object *obj)
> > {
> > /* Disable l1tf mitigating, revert all changes */
> > l1tf_disable();
> > }
> >
> > void klp_post_unpatch_callback(struct klp_object *obj)
> > {
> > struct klp_feature *feature;
> >
> > feature = klp_get_feature(L1TF);
> > if (!feature) {
> > WARN("This should never happen\n);
> >
> > if (feature->obj != obj && feature->version == L1TF_VERSION) {
> > /* This is revert, previous patch keeps using it */
> > return;
> > }
> >
> > klp_feature_free(feature);
> > }
> >
> >
> > Finally, the question is how to check the compatibility within update.
> > In this case, it might be easier if version, id is part of
> > struct klp_callbacks.
> >
> > Alternative solution would be to add another callback:
> >
> > int klp_callback_feature_version(id, version);
> >
> > Then we could do:
> >
> > /* all features supported */
> > bool compatible = true;
> >
> > for_each_allocated_feature(feature) {
> > bool found_compat = false;
> > for_each_object() {
> > int version;
> > version = obj->callbacks->feature_version(feature->id);
> > if (version >= feature->version) {
> > found_compat == true;
> > break;
> > }
> > }
> > }
> >
> > if (!found_compat) {
> > compatible = false;
> > break;
> > }
> >
> >
> > It is more complicated than the global version. But it is not
> > a rocket since either.
> >
> > Now, the question is how much you like it.
>
> I see where this is going and off the top of my head, I think it would
> address the L1TF upgrade/downgrade safety concerns.
>
> If you prefer, we can revist this when there is code to review, but here my
> initial questions and impressions:
Good question. It might save me a lot of time if we find some
big flaw in my proposal now. On the other hand, many things
will be much easier to discuss with the real code.
> - 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.) 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.
I think that the name "feature" causes some confusion.
Unfortunately I can't find a better one at this point.
Anyway, I see it as a change that cannot get reverted simply
by disabling the ftrace handlers. It might be done either by
module exit() function or by the livepatch callbacks.
The exit() function might never be called. It leaves
callbacks as the only reasonable solutions. But see below.
> - Perhaps the features-to-callback connection in my mind is too strong. It
> makes it sound like the callbacks themselves have provided a feature. It is
> possible that patched routines are the unsafe culprits while callbacks only
> a mechanism to enforce safety. I guess this is why I think to abstract
> features out to the patch level.
The connection between features and callbacks is really weak
in my proposal.
One callback could work with many features. This might be a common
pattern if more features are related to one module, e.g. vmlinux.
And one feature might be affected by several callbacks. For example,
when an action is needed for vmlinux and other module.
Note that one struct klp_callbacks might advertise supporting more
features and the same feature might be advertised by more modules.
> - One more point on the abstraction: pulling this out of the callbacks may
> allow one to understand livepatch features without the details of the
> callbacks. Not sure if that's a good or bad thing ...
To be honest, I think that the way how the supported features
are advertised is the weakest thing in my proposal. I will
be happy if we find a better solution. I'll think about it.
> - 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?
The first variant. The incoming cumulative patch must be able to
handle all features (changes) provided by the current patch.
> Dunno if cumulative livepatches change that requirement or not.
>
> - Wacky idea: can features be deprecated? A livepatch neuters the unsafe
> condition, so future patches can now safely ignore it.
It will work with the dynamically allocated features. The new
livepatch just need to be able to do the needed clean up and
free the related struct klp_feature. The next livepatches
won't need to support the feature when they make sure
that the one (cleaning the stuff) was applied.
>
> Sorry for all the thinking out loud, but I figured I would share my frame of
> mind after coming out of the version/sticky RFC code. tl;dr: I think this
> would technically work, the question is how elaborate a policy do we need
> and then where does it need to be implemented.
No problem. Thanks for thinking about it and feedback.
Best Regards,
Petr
