Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1739951 > unrolled thread

Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Petr Mladek <pmladek@suse.com> - 2017-09-26 17:00 +0200
    Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-26 21:10 +0200
      Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks Petr Mladek <pmladek@suse.com> - 2017-09-27 10:10 +0200

#1739951 — Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..aca62c4b8616 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj)
>  	return obj->name;
>  }
>  
> -static bool klp_is_object_loaded(struct klp_object *obj)
> -{
> -	return !obj->name || obj->mod;
> -}
> -
>  /* sets obj->mod if object is not vmlinux and module is found */
>  static void klp_find_object_module(struct klp_object *obj)
>  {
> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
>  
>  static int __klp_disable_patch(struct klp_patch *patch)
>  {
> +	struct klp_object *obj;
> +
>  	if (klp_transition_patch)
>  		return -EBUSY;
>  
> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  
>  	klp_init_transition(patch, KLP_UNPATCHED);
>  
> +	klp_for_each_object(patch, obj)
> +		if (patch->enabled && obj->patched)
> +			klp_pre_unpatch_callback(obj);
> +
>  	/*
>  	 * Enforce the order of the func->transition writes in
>  	 * klp_init_transition() and the TIF_PATCH_PENDING writes in
> @@ -388,13 +389,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  		if (!klp_is_object_loaded(obj))
>  			continue;
>  
> -		ret = klp_patch_object(obj);
> +		ret = klp_pre_patch_callback(obj);
>  		if (ret) {
> -			pr_warn("failed to enable patch '%s'\n",
> -				patch->mod->name);
> +			pr_warn("pre-patch callback failed for object '%s'\n",
> +				klp_is_module(obj) ? obj->name : "vmlinux");
> +			goto err;
> +		}
>  
> -			klp_cancel_transition();
> -			return ret;
> +		ret = klp_patch_object(obj);
> +		if (ret) {
> +			pr_warn("failed to patch object '%s'\n",
> +				klp_is_module(obj) ? obj->name : "vmlinux");

We should call klp_post_unpatch_callback(obj) here to make it
synchronous.

Well, what about calling:

      klp_pre_patch_callback() inside klp_patch_object() and
      klp_post_unpatch_callback() inside klp_unpatch_object()

By other words, we would do the two operations. It would have
two advantages:

   + error handling for free
   + no need for the strange callbacks_enabled flag

It would require the more strict consistency model if there
is a dependency between the callbacks and patches from various
modules. But we would probably need the consistency model
in this case anyway.

> +			goto err;

>  		}
>  	}
>  

Otherwise I think that we are getting close.

Best Regards,
Petr

PS: I hope that the above problem and solution has not been mentioned
yet. I am sorry if it was. I am a bit lost in many mails after
vacation, sickness, and conference.

[toc] | [next] | [standalone]

#1740139

On 09/26/2017 10:49 AM, Petr Mladek wrote:
> On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index b9628e43c78f..aca62c4b8616 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj)
>>  	return obj->name;
>>  }
>>  
>> -static bool klp_is_object_loaded(struct klp_object *obj)
>> -{
>> -	return !obj->name || obj->mod;
>> -}
>> -
>>  /* sets obj->mod if object is not vmlinux and module is found */
>>  static void klp_find_object_module(struct klp_object *obj)
>>  {
>> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
>>  
>>  static int __klp_disable_patch(struct klp_patch *patch)
>>  {
>> +	struct klp_object *obj;
>> +
>>  	if (klp_transition_patch)
>>  		return -EBUSY;
>>  
>> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
>>  
>>  	klp_init_transition(patch, KLP_UNPATCHED);
>>  
>> +	klp_for_each_object(patch, obj)
>> +		if (patch->enabled && obj->patched)
>> +			klp_pre_unpatch_callback(obj);
>> +
>>  	/*
>>  	 * Enforce the order of the func->transition writes in
>>  	 * klp_init_transition() and the TIF_PATCH_PENDING writes in
>> @@ -388,13 +389,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
>>  		if (!klp_is_object_loaded(obj))
>>  			continue;
>>  
>> -		ret = klp_patch_object(obj);
>> +		ret = klp_pre_patch_callback(obj);
>>  		if (ret) {
>> -			pr_warn("failed to enable patch '%s'\n",
>> -				patch->mod->name);
>> +			pr_warn("pre-patch callback failed for object '%s'\n",
>> +				klp_is_module(obj) ? obj->name : "vmlinux");
>> +			goto err;
>> +		}
>>  
>> -			klp_cancel_transition();
>> -			return ret;
>> +		ret = klp_patch_object(obj);
>> +		if (ret) {
>> +			pr_warn("failed to patch object '%s'\n",
>> +				klp_is_module(obj) ? obj->name : "vmlinux");
> 
> We should call klp_post_unpatch_callback(obj) here to make it
> synchronous.

Are you talking about the error path?  As its coded here,
klp_cancel_transition() will call klp_complete_transition() with
klp_target_state = KLP_UNPATCHED and then klp_complete_transition()'s
done: code will call klp_post_unpatch_callback() on all the necessary
kobj's.  Is there something asynchronous about that?

> Well, what about calling:
> 
>       klp_pre_patch_callback() inside klp_patch_object() and
>       klp_post_unpatch_callback() inside klp_unpatch_object()

v1 started out that way, but we migrated to placing these around the
callers of klp_(un)patch_object() to try and better line up the
locations of the pre- hooks  with the post- hook locations.

I can take a second look at reversing this decision, but that may take a
little time while I page all the testing corner cases back into my brain :)

> By other words, we would do the two operations. It would have
> two advantages:
> 
>    + error handling for free
>    + no need for the strange callbacks_enabled flag

Indeed, it would be nice to ditch that callbacks_enabled wart.

> It would require the more strict consistency model if there
> is a dependency between the callbacks and patches from various
> modules. But we would probably need the consistency model
> in this case anyway.
> 
>> +			goto err;
> 
>>  		}
>>  	}
>>  
> 
> Otherwise I think that we are getting close.
> 
> Best Regards,
> Petr
> 
> PS: I hope that the above problem and solution has not been mentioned
> yet. I am sorry if it was. I am a bit lost in many mails after
> vacation, sickness, and conference.

I think the only other outstanding issue before rolling a v6 is the one
that Miroslav raised about the error path in klp_module_coming():

  https://marc.info/?l=linux-kernel&m=150590635602784&w=2
  https://marc.info/?l=linux-kernel&m=150592065007463&w=2

Thanks,

-- Joe

[toc] | [prev] | [next] | [standalone]

#1740480

On Tue 2017-09-26 15:01:52, Joe Lawrence wrote:
> On 09/26/2017 10:49 AM, Petr Mladek wrote:
> > On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
> >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >> index b9628e43c78f..aca62c4b8616 100644
> >> --- a/kernel/livepatch/core.c
> >> +++ b/kernel/livepatch/core.c
> >> @@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj)
> >>  	return obj->name;
> >>  }
> >>  
> >> -static bool klp_is_object_loaded(struct klp_object *obj)
> >> -{
> >> -	return !obj->name || obj->mod;
> >> -}
> >> -
> >>  /* sets obj->mod if object is not vmlinux and module is found */
> >>  static void klp_find_object_module(struct klp_object *obj)
> >>  {
> >> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
> >>  
> >>  static int __klp_disable_patch(struct klp_patch *patch)
> >>  {
> >> +	struct klp_object *obj;
> >> +
> >>  	if (klp_transition_patch)
> >>  		return -EBUSY;
> >>  
> >> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
> >>  
> >>  	klp_init_transition(patch, KLP_UNPATCHED);
> >>  
> >> +	klp_for_each_object(patch, obj)
> >> +		if (patch->enabled && obj->patched)
> >> +			klp_pre_unpatch_callback(obj);
> >> +
> >>  	/*
> >>  	 * Enforce the order of the func->transition writes in
> >>  	 * klp_init_transition() and the TIF_PATCH_PENDING writes in
> >> @@ -388,13 +389,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
> >>  		if (!klp_is_object_loaded(obj))
> >>  			continue;
> >>  
> >> -		ret = klp_patch_object(obj);
> >> +		ret = klp_pre_patch_callback(obj);
> >>  		if (ret) {
> >> -			pr_warn("failed to enable patch '%s'\n",
> >> -				patch->mod->name);
> >> +			pr_warn("pre-patch callback failed for object '%s'\n",
> >> +				klp_is_module(obj) ? obj->name : "vmlinux");
> >> +			goto err;
> >> +		}
> >>  
> >> -			klp_cancel_transition();
> >> -			return ret;
> >> +		ret = klp_patch_object(obj);
> >> +		if (ret) {
> >> +			pr_warn("failed to patch object '%s'\n",
> >> +				klp_is_module(obj) ? obj->name : "vmlinux");
> > 
> > We should call klp_post_unpatch_callback(obj) here to make it
> > synchronous.
> 
> Are you talking about the error path?  As its coded here,
> klp_cancel_transition() will call klp_complete_transition() with
> klp_target_state = KLP_UNPATCHED and then klp_complete_transition()'s
> done: code will call klp_post_unpatch_callback() on all the necessary
> kobj's.  Is there something asynchronous about that?

Ah, I have missed it. It is a bit tricky ;-)


> > Well, what about calling:
> > 
> >       klp_pre_patch_callback() inside klp_patch_object() and
> >       klp_post_unpatch_callback() inside klp_unpatch_object()
> 
> v1 started out that way, but we migrated to placing these around the
> callers of klp_(un)patch_object() to try and better line up the
> locations of the pre- hooks  with the post- hook locations.

I guess that the move was mainly motivated by introducing 4 callbacks
instead of only two of them.

On one hand, it is fine to see a symmetric code like, for example,
in klp_module_going():

	klp_pre_unpatch_callback(obj);
	klp_unpatch_object(obj);
	klp_post_unpatch_callback(obj);


On the other hand, it adds yet another asymmetry between
__klp_enable_patch()/__klp_disable_patch() and
klp_finish_transition(), see my confusion above.

I know that that the asymmetry was already there because of the
klp_patch_object() and klp_unpatch_object().

I mean that klp_patch_object() calls klp_unpatch_object() in case
of errors. But this handles only the current object. We still
rely on calling klp_cancel_transition()->klp_complete_transition()
to call klp_unpatch_object() for the other already proceed objects.


> I can take a second look at reversing this decision, but that may take a
> little time while I page all the testing corner cases back into my brain :)

I am sorry for the late reply. Heh, I needed to refresh a lot of
things as well. The advantage is that one could see things from
new perspective when the head was cleaned in between ;-)


> > By other words, we would do the two operations. It would have
> > two advantages:
> > 
> >    + error handling for free
> >    + no need for the strange callbacks_enabled flag
> 
> Indeed, it would be nice to ditch that callbacks_enabled wart.

Yup, I hope that in this case the less states would mean
the easier logic. And handling of klp_patch_object()/klp_unpatch()
object is already tricky enough. It would be lovely to just reuse
it if we can.


> I think the only other outstanding issue before rolling a v6 is the one
> that Miroslav raised about the error path in klp_module_coming():
> 
>   https://marc.info/?l=linux-kernel&m=150590635602784&w=2
>   https://marc.info/?l=linux-kernel&m=150592065007463&w=2

I am going to look at it.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web