Re: [RFC PATCH 1/2] livepatch: implement patch versioning

[Petr Mladek <pmladek@xxxxxxxx>]

On Tue 2019-02-12 09:52:04, Joe Lawrence wrote:
> On 2/12/19 5:15 AM, Petr Mladek wrote:
> > On Mon 2019-02-11 11:05:16, Joe Lawrence wrote:
> > > Add a simple livepatch versioning policy to the livepatch core.  For
> > > those klp_patches with non-zero version, only load those with higher
> > > versions than the livepatch core's latest-version.  Livepatches may opt
> > > out of the policy altogether (i.e. patches always load and do not update
> > > latest-version) by specifying klp_patch.version=0.
> 
> Hi Petr,
> 
> Thanks for having a look...
> 
> > 
> > I think that a more practical would be a per-feature/per-callback-set
> > versions.
> > 
> > We currently support many code streams. By definition, there are more
> > released livepatches for older kernels that for recent ones. It might
> > be error prone to decide about a particular feature by a global
> > version that would be different in different code streams.
> 
> The RFC livepatch versioning should only pertain to the same kernel version,
> and is more accurate to think of it as only "version N+1 must be safe to
> load if version N is loaded".  Making a strong association with a particular
> feature may lead to errors as you pointed out.

I remember that we also want to support downgrade. It is safe as long
as the cumulative patches are compatible.

It is doable event with the global version. We could bump it only
with incompatible changes. But then it won't be a real version.

In each case, I do not want to block the global version. It is
better than nothing. But I want to point out that we might want
to replace it with something more safe-to-use in the future.


> If we wanted to reuse versioning data across kernel versions, I would
> suggest using 'feature' enumerations.  So a vendor could define
> KLP_FEATURE_L1TF and then reuse that in a common header file for all
> livepatches for all kernels.  (Jumping ahead, I'm guessing the idea would be
> that patches would supply a list of features and then subsequent ones must
> include features of previous ones?)

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.

Best Regards,
Petr