[PATCH 0/2] disable/enable_patch manners for interdependent patches

[PATCH 0/2] disable/enable_patch manners for interdependent patches

[Li Bin]

Revert commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17
and introduce disable/enable_patch manners for interdependent patches

for disable_patch:
The patch is unallowed to be disabled if one patch after has
dependencies with it and has been enabled.

for enable_patch:
The patch is unallowed to be enabled if one patch before has
dependencies with it and has been disabled.
Li Bin (2):
  livepatch: Revert "livepatch: enforce patch stacking semantics"
  livepatch: disable/enable_patch manners for interdependent patches

 kernel/livepatch/core.c |   66 +++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 58 insertions(+), 8 deletions(-)

[PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Li Bin]

This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.

The method that only allowing the topmost patch on the stack to be
enabled or disabled is unreasonable. Such as the following case:

	- do live patch1
	- disable patch1
	- do live patch2 //error

Now, we will never be able to do new live patch unless disabing the
patch1 although there is no dependencies.

Signed-off-by: Li Bin <huawei.libin@huawei.com>
---
 kernel/livepatch/core.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc05d39..7861ed2 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -468,11 +468,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
 	struct klp_object *obj;
 	int ret;
 
-	/* enforce stacking: only the last enabled patch can be disabled */
-	if (!list_is_last(&patch->list, &klp_patches) &&
-	    list_next_entry(patch, list)->state == KLP_ENABLED)
-		return -EBUSY;
-
 	pr_notice("disabling patch '%s'\n", patch->mod->name);
 
 	for (obj = patch->objs; obj->funcs; obj++) {
@@ -529,11 +524,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (WARN_ON(patch->state != KLP_DISABLED))
 		return -EINVAL;
 
-	/* enforce stacking: only the first disabled patch can be enabled */
-	if (patch->list.prev != &klp_patches &&
-	    list_prev_entry(patch, list)->state == KLP_DISABLED)
-		return -EBUSY;
-
 	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
 	add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
 
-- 
1.7.1

[PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Li Bin]

for disable_patch:
The patch is unallowed to be disabled if one patch after has
dependencies with it and has been enabled.

for enable_patch:
The patch is unallowed to be enabled if one patch before has
dependencies with it and has been disabled.

Signed-off-by: Li Bin <huawei.libin@huawei.com>
---
 kernel/livepatch/core.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 7861ed2..a12a31c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -114,6 +114,21 @@ static bool klp_is_patch_registered(struct klp_patch *patch)
 	return false;
 }
 
+static bool klp_func_in_patch(struct klp_func *kfunc, struct klp_patch *patch)
+{
+	struct klp_object *obj;
+	struct klp_func *func;
+
+	for (obj = patch->objs; obj->funcs; obj++) {
+		for (func = obj->funcs; func->old_name; func++) {
+			if (kfunc->old_addr == func->old_addr) {
+				return true;
+			}
+		}
+	}
+	return false;
+}
+
 static bool klp_initialized(void)
 {
 	return klp_root_kobj;
@@ -466,8 +481,31 @@ unregister:
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
+	struct klp_patch *temp;
+	struct klp_func *func;
 	int ret;
 
+	/*
+	 * the patch is unallowed to be disabled if one patch
+	 * after has dependencies with it and has been enabled.
+	 */
+	for (temp = list_next_entry(patch, list);
+			&temp->list != &klp_patches;
+			temp = list_next_entry(temp, list)) {
+		if (temp->state != KLP_ENABLED)
+			continue;
+
+		for (obj = patch->objs; obj->funcs; obj++) {
+			for (func = obj->funcs; func->old_name; func++) {
+				if (klp_func_in_patch(func, temp)) {
+					pr_err("this patch depends on '%s', please disable it firstly\n",
+						   temp->mod->name);
+					return -EBUSY;
+				}
+			}
+		}
+	}
+
 	pr_notice("disabling patch '%s'\n", patch->mod->name);
 
 	for (obj = patch->objs; obj->funcs; obj++) {
@@ -519,11 +557,33 @@ EXPORT_SYMBOL_GPL(klp_disable_patch);
 static int __klp_enable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
+	struct klp_patch *temp;
+	struct klp_func *func;
 	int ret;
 
 	if (WARN_ON(patch->state != KLP_DISABLED))
 		return -EINVAL;
 
+	/*
+	 * the patch is unallowed to be enabled if one patch
+	 * before has dependencies with it and has been disabled.
+	 */
+	for (temp = list_first_entry(&klp_patches, struct klp_patch, list);
+			temp != patch; temp = list_next_entry(temp, list)) {
+		if (temp->state != KLP_DISABLED)
+			continue;
+
+		for (obj = patch->objs; obj->funcs; obj++) {
+			for (func = obj->funcs; func->old_name; func++) {
+				if (klp_func_in_patch(func, temp)) {
+					pr_err("this patch depends on '%s', please enable it firstly\n",
+						   temp->mod->name);
+					return -EBUSY;
+				}
+			}
+		}
+	}
+
 	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
 	add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
 
-- 
1.7.1

Re: [PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Jiri Kosina]

On Wed, 21 Jan 2015, Li Bin wrote:

> This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.
> 
> The method that only allowing the topmost patch on the stack to be
> enabled or disabled is unreasonable. Such as the following case:
> 
> 	- do live patch1
> 	- disable patch1
> 	- do live patch2 //error
>
> Now, we will never be able to do new live patch unless disabing the
> patch1 although there is no dependencies.

Unregistering disabled patch still works and removes it from the list no 
matter the position.

So what exactly is the problem?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Jiri Kosina]

On Wed, 21 Jan 2015, Li Bin wrote:

> for disable_patch:
> The patch is unallowed to be disabled if one patch after has
> dependencies with it and has been enabled.
> 
> for enable_patch:
> The patch is unallowed to be enabled if one patch before has
> dependencies with it and has been disabled.
> 
> Signed-off-by: Li Bin <huawei.libin@huawei.com>
> ---
>  kernel/livepatch/core.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 7861ed2..a12a31c 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -114,6 +114,21 @@ static bool klp_is_patch_registered(struct klp_patch *patch)
>  	return false;
>  }
>  
> +static bool klp_func_in_patch(struct klp_func *kfunc, struct klp_patch *patch)
> +{
> +	struct klp_object *obj;
> +	struct klp_func *func;
> +
> +	for (obj = patch->objs; obj->funcs; obj++) {
> +		for (func = obj->funcs; func->old_name; func++) {
> +			if (kfunc->old_addr == func->old_addr) {
> +				return true;
> +			}
> +		}
> +	}
> +	return false;
> +}
> +
>  static bool klp_initialized(void)
>  {
>  	return klp_root_kobj;
> @@ -466,8 +481,31 @@ unregister:
>  static int __klp_disable_patch(struct klp_patch *patch)
>  {
>  	struct klp_object *obj;
> +	struct klp_patch *temp;
> +	struct klp_func *func;
>  	int ret;
>  
> +	/*
> +	 * the patch is unallowed to be disabled if one patch
> +	 * after has dependencies with it and has been enabled.
> +	 */
> +	for (temp = list_next_entry(patch, list);
> +			&temp->list != &klp_patches;
> +			temp = list_next_entry(temp, list)) {
> +		if (temp->state != KLP_ENABLED)
> +			continue;
> +
> +		for (obj = patch->objs; obj->funcs; obj++) {
> +			for (func = obj->funcs; func->old_name; func++) {
> +				if (klp_func_in_patch(func, temp)) {
> +					pr_err("this patch depends on '%s', please disable it firstly\n",
> +						   temp->mod->name);
> +					return -EBUSY;
> +				}
> +			}
> +		}
> +	}
> +
>  	pr_notice("disabling patch '%s'\n", patch->mod->name);
>  
>  	for (obj = patch->objs; obj->funcs; obj++) {
> @@ -519,11 +557,33 @@ EXPORT_SYMBOL_GPL(klp_disable_patch);
>  static int __klp_enable_patch(struct klp_patch *patch)
>  {
>  	struct klp_object *obj;
> +	struct klp_patch *temp;
> +	struct klp_func *func;
>  	int ret;
>  
>  	if (WARN_ON(patch->state != KLP_DISABLED))
>  		return -EINVAL;
>  
> +	/*
> +	 * the patch is unallowed to be enabled if one patch
> +	 * before has dependencies with it and has been disabled.
> +	 */
> +	for (temp = list_first_entry(&klp_patches, struct klp_patch, list);
> +			temp != patch; temp = list_next_entry(temp, list)) {
> +		if (temp->state != KLP_DISABLED)
> +			continue;
> +
> +		for (obj = patch->objs; obj->funcs; obj++) {
> +			for (func = obj->funcs; func->old_name; func++) {
> +				if (klp_func_in_patch(func, temp)) {
> +					pr_err("this patch depends on '%s', please enable it firstly\n",
> +						   temp->mod->name);
> +					return -EBUSY;

By this you limit the definition of the patch inter-dependency to just 
symbols. But that's not the only way how patches can depend on it other -- 
the dependency can be semantical.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Seth Jennings]

On Wed, Jan 21, 2015 at 03:06:38PM +0100, Jiri Kosina wrote:
> On Wed, 21 Jan 2015, Li Bin wrote:
> 
> > This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.
> > 
> > The method that only allowing the topmost patch on the stack to be
> > enabled or disabled is unreasonable. Such as the following case:
> > 
> > 	- do live patch1
> > 	- disable patch1
> > 	- do live patch2 //error
> >
> > Now, we will never be able to do new live patch unless disabing the
> > patch1 although there is no dependencies.
> 
> Unregistering disabled patch still works and removes it from the list no 
> matter the position.
> 
> So what exactly is the problem?

>From a quick glance, it seems that what this set does is it only
enforces the stacking requirements if two patches patch the same
function.

I'm not sure if that is correct logically or correctly implemented by
these patches yet.

Seth

> 
> -- 
> Jiri Kosina
> SUSE Labs

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Li Bin]

On 2015/1/21 22:08, Jiri Kosina wrote:
> On Wed, 21 Jan 2015, Li Bin wrote:
> 
>> for disable_patch:
>> The patch is unallowed to be disabled if one patch after has
>> dependencies with it and has been enabled.
>>
>> for enable_patch:
>> The patch is unallowed to be enabled if one patch before has
>> dependencies with it and has been disabled.
>>
>> Signed-off-by: Li Bin <huawei.libin@huawei.com>
>> ---
>>  kernel/livepatch/core.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 60 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 7861ed2..a12a31c 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -114,6 +114,21 @@ static bool klp_is_patch_registered(struct klp_patch *patch)
>>  	return false;
>>  }
>>  
>> +static bool klp_func_in_patch(struct klp_func *kfunc, struct klp_patch *patch)
>> +{
>> +	struct klp_object *obj;
>> +	struct klp_func *func;
>> +
>> +	for (obj = patch->objs; obj->funcs; obj++) {
>> +		for (func = obj->funcs; func->old_name; func++) {
>> +			if (kfunc->old_addr == func->old_addr) {
>> +				return true;
>> +			}
>> +		}
>> +	}
>> +	return false;
>> +}
>> +
>>  static bool klp_initialized(void)
>>  {
>>  	return klp_root_kobj;
>> @@ -466,8 +481,31 @@ unregister:
>>  static int __klp_disable_patch(struct klp_patch *patch)
>>  {
>>  	struct klp_object *obj;
>> +	struct klp_patch *temp;
>> +	struct klp_func *func;
>>  	int ret;
>>  
>> +	/*
>> +	 * the patch is unallowed to be disabled if one patch
>> +	 * after has dependencies with it and has been enabled.
>> +	 */
>> +	for (temp = list_next_entry(patch, list);
>> +			&temp->list != &klp_patches;
>> +			temp = list_next_entry(temp, list)) {
>> +		if (temp->state != KLP_ENABLED)
>> +			continue;
>> +
>> +		for (obj = patch->objs; obj->funcs; obj++) {
>> +			for (func = obj->funcs; func->old_name; func++) {
>> +				if (klp_func_in_patch(func, temp)) {
>> +					pr_err("this patch depends on '%s', please disable it firstly\n",
>> +						   temp->mod->name);
>> +					return -EBUSY;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>>  	pr_notice("disabling patch '%s'\n", patch->mod->name);
>>  
>>  	for (obj = patch->objs; obj->funcs; obj++) {
>> @@ -519,11 +557,33 @@ EXPORT_SYMBOL_GPL(klp_disable_patch);
>>  static int __klp_enable_patch(struct klp_patch *patch)
>>  {
>>  	struct klp_object *obj;
>> +	struct klp_patch *temp;
>> +	struct klp_func *func;
>>  	int ret;
>>  
>>  	if (WARN_ON(patch->state != KLP_DISABLED))
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * the patch is unallowed to be enabled if one patch
>> +	 * before has dependencies with it and has been disabled.
>> +	 */
>> +	for (temp = list_first_entry(&klp_patches, struct klp_patch, list);
>> +			temp != patch; temp = list_next_entry(temp, list)) {
>> +		if (temp->state != KLP_DISABLED)
>> +			continue;
>> +
>> +		for (obj = patch->objs; obj->funcs; obj++) {
>> +			for (func = obj->funcs; func->old_name; func++) {
>> +				if (klp_func_in_patch(func, temp)) {
>> +					pr_err("this patch depends on '%s', please enable it firstly\n",
>> +						   temp->mod->name);
>> +					return -EBUSY;
> 
> By this you limit the definition of the patch inter-dependency to just 
> symbols. But that's not the only way how patches can depend on it other -- 
> the dependency can be semantical.

Yes, I agree with you. But I think the other dependencies such as semantical
dependency should be judged by the user, like reverting a patch from git repository.
Right?

Thanks,
	Li Bin

> 

Re: [PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Li Bin]

On 2015/1/21 22:36, Seth Jennings wrote:
> On Wed, Jan 21, 2015 at 03:06:38PM +0100, Jiri Kosina wrote:
>> On Wed, 21 Jan 2015, Li Bin wrote:
>>
>>> This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.
>>>
>>> The method that only allowing the topmost patch on the stack to be
>>> enabled or disabled is unreasonable. Such as the following case:
>>>
>>> 	- do live patch1
>>> 	- disable patch1
>>> 	- do live patch2 //error
>>>
>>> Now, we will never be able to do new live patch unless disabing the
>>> patch1 although there is no dependencies.
>>
>> Unregistering disabled patch still works and removes it from the list no 
>> matter the position.
>>
>> So what exactly is the problem?
> 
>>From a quick glance, it seems that what this set does is it only
> enforces the stacking requirements if two patches patch the same
> function.
> 

Yes, this patch is only concerning this case that 'multi patches patch
the same function' and solve the problem that mentioned previously:

foo_unpatched()
	foo_patch1()
		foo_patch2()
			foo_patch3()
		disable(foo_patch2)
		disable(foo_patch3)
	foo_patch1()

foo_patch2 is not allowed to be disabled before disable foo_patch3.

Thanks,
	Li Bin

> I'm not sure if that is correct logically or correctly implemented by
> these patches yet.
> 
> Seth
> 
>>
>> -- 
>> Jiri Kosina
>> SUSE Labs
> 
> .
> 

Re: [PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Li Bin]

On 2015/1/21 17:07, Li Bin wrote:
> This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.
> 
> The method that only allowing the topmost patch on the stack to be
> enabled or disabled is unreasonable. Such as the following case:
> 
> 	- do live patch1
> 	- disable patch1
> 	- do live patch2 //error
> 
> Now, we will never be able to do new live patch unless disabing the
> patch1 although there is no dependencies.
> 

Correct the log:
... unless disabling the patch1 although ... -->
... unless enabling the patch1 firstly although ...

> Signed-off-by: Li Bin <huawei.libin@huawei.com>
> ---
>  kernel/livepatch/core.c |   10 ----------
>  1 files changed, 0 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc05d39..7861ed2 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -468,11 +468,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  	struct klp_object *obj;
>  	int ret;
>  
> -	/* enforce stacking: only the last enabled patch can be disabled */
> -	if (!list_is_last(&patch->list, &klp_patches) &&
> -	    list_next_entry(patch, list)->state == KLP_ENABLED)
> -		return -EBUSY;
> -
>  	pr_notice("disabling patch '%s'\n", patch->mod->name);
>  
>  	for (obj = patch->objs; obj->funcs; obj++) {
> @@ -529,11 +524,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	if (WARN_ON(patch->state != KLP_DISABLED))
>  		return -EINVAL;
>  
> -	/* enforce stacking: only the first disabled patch can be enabled */
> -	if (patch->list.prev != &klp_patches &&
> -	    list_prev_entry(patch, list)->state == KLP_DISABLED)
> -		return -EBUSY;
> -
>  	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
>  	add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
>  
> 

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Josh Poimboeuf]

On Thu, Jan 22, 2015 at 08:42:29AM +0800, Li Bin wrote:
> On 2015/1/21 22:08, Jiri Kosina wrote:
> > On Wed, 21 Jan 2015, Li Bin wrote:
> > By this you limit the definition of the patch inter-dependency to just 
> > symbols. But that's not the only way how patches can depend on it other -- 
> > the dependency can be semantical.
> 
> Yes, I agree with you. But I think the other dependencies such as semantical
> dependency should be judged by the user, like reverting a patch from git repository.
> Right?

But with live patching, there are two users: the patch creator (who
creates the patch module) and the end user (who loads it on their
system).

We can assume the patch creator knows what he's doing, but the end user
doesn't always know or care about low level details like patch
dependencies.  The easiest and safest way to protect the end user is the
current approach, which assumes that each patch depends on all
previously applied patches.

-- 
Josh

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Li Bin]

On 2015/1/22 11:51, Josh Poimboeuf wrote:
> On Thu, Jan 22, 2015 at 08:42:29AM +0800, Li Bin wrote:
>> On 2015/1/21 22:08, Jiri Kosina wrote:
>>> On Wed, 21 Jan 2015, Li Bin wrote:
>>> By this you limit the definition of the patch inter-dependency to just 
>>> symbols. But that's not the only way how patches can depend on it other -- 
>>> the dependency can be semantical.
>>
>> Yes, I agree with you. But I think the other dependencies such as semantical
>> dependency should be judged by the user, like reverting a patch from git repository.
>> Right?
> 
> But with live patching, there are two users: the patch creator (who
> creates the patch module) and the end user (who loads it on their
> system).
> 
> We can assume the patch creator knows what he's doing, but the end user
> doesn't always know or care about low level details like patch
> dependencies.  The easiest and safest way to protect the end user is the
> current approach, which assumes that each patch depends on all
> previously applied patches.

But then, the feature that disable patch dynamically is useless.
For example, if user find a bug be introduced by the last patch and disable
it directly, the new patch is no longer allowed from now unless enable the
old patch firstly but there is a risk window by this way.

> 

Re: [PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Miroslav Benes]

On Thu, 22 Jan 2015, Li Bin wrote:

> On 2015/1/21 17:07, Li Bin wrote:
> > This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.
> > 
> > The method that only allowing the topmost patch on the stack to be
> > enabled or disabled is unreasonable. Such as the following case:
> > 
> > 	- do live patch1
> > 	- disable patch1
> > 	- do live patch2 //error
> > 
> > Now, we will never be able to do new live patch unless disabing the
> > patch1 although there is no dependencies.
> > 
> 
> Correct the log:
> ... unless disabling the patch1 although ... -->
> ... unless enabling the patch1 firstly although ...

Yes, but in such situation you can unregister patch1 and proceed with new 
live patch. No problem. As Jiri has already written. Or are we missing 
something?

Regards,
--
Miroslav Benes
SUSE Labs

Re: [PATCH 1/2] livepatch: Revert "livepatch: enforce patch stacking semantics"

[Li Bin]

On 2015/1/22 17:15, Miroslav Benes wrote:
> On Thu, 22 Jan 2015, Li Bin wrote:
> 
>> On 2015/1/21 17:07, Li Bin wrote:
>>> This reverts commit 83a90bb1345767f0cb96d242fd8b9db44b2b0e17.
>>>
>>> The method that only allowing the topmost patch on the stack to be
>>> enabled or disabled is unreasonable. Such as the following case:
>>>
>>> 	- do live patch1
>>> 	- disable patch1
>>> 	- do live patch2 //error
>>>
>>> Now, we will never be able to do new live patch unless disabing the
>>> patch1 although there is no dependencies.
>>>
>>
>> Correct the log:
>> ... unless disabling the patch1 although ... -->
>> ... unless enabling the patch1 firstly although ...
> 
> Yes, but in such situation you can unregister patch1 and proceed with new 
> live patch. No problem. As Jiri has already written. Or are we missing 
> something?
> 

Ok, that is before process with new live patch we must unregister the disabled
patch1 previously. Is there need some message to avoid confusing the user?

Thanks,
Li Bin

> Regards,
> --
> Miroslav Benes
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Li Bin]

On 2015/1/22 16:39, Li Bin wrote:
> On 2015/1/22 11:51, Josh Poimboeuf wrote:
>> On Thu, Jan 22, 2015 at 08:42:29AM +0800, Li Bin wrote:
>>> On 2015/1/21 22:08, Jiri Kosina wrote:
>>>> On Wed, 21 Jan 2015, Li Bin wrote:
>>>> By this you limit the definition of the patch inter-dependency to just 
>>>> symbols. But that's not the only way how patches can depend on it other -- 
>>>> the dependency can be semantical.
>>>
>>> Yes, I agree with you. But I think the other dependencies such as semantical
>>> dependency should be judged by the user, like reverting a patch from git repository.
>>> Right?
>>
>> But with live patching, there are two users: the patch creator (who
>> creates the patch module) and the end user (who loads it on their
>> system).
>>
>> We can assume the patch creator knows what he's doing, but the end user
>> doesn't always know or care about low level details like patch
>> dependencies.  The easiest and safest way to protect the end user is the
>> current approach, which assumes that each patch depends on all
>> previously applied patches.
> 
> But then, the feature that disable patch dynamically is useless.
> For example, if user find a bug be introduced by the last patch and disable
> it directly, the new patch is no longer allowed from now unless enable the
> old patch firstly but there is a risk window by this way.
> 

Ok, in this case we can unregister the old patch firstly.
But it seems that the feature that enable/disable patch dynamically indeed
useless. (Its value is only for the last patch to enable or disable.)

>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Josh Poimboeuf]

On Thu, Jan 22, 2015 at 05:54:23PM +0800, Li Bin wrote:
> On 2015/1/22 16:39, Li Bin wrote:
> > On 2015/1/22 11:51, Josh Poimboeuf wrote:
> >> On Thu, Jan 22, 2015 at 08:42:29AM +0800, Li Bin wrote:
> >>> On 2015/1/21 22:08, Jiri Kosina wrote:
> >>>> On Wed, 21 Jan 2015, Li Bin wrote:
> >>>> By this you limit the definition of the patch inter-dependency to just 
> >>>> symbols. But that's not the only way how patches can depend on it other -- 
> >>>> the dependency can be semantical.
> >>>
> >>> Yes, I agree with you. But I think the other dependencies such as semantical
> >>> dependency should be judged by the user, like reverting a patch from git repository.
> >>> Right?
> >>
> >> But with live patching, there are two users: the patch creator (who
> >> creates the patch module) and the end user (who loads it on their
> >> system).
> >>
> >> We can assume the patch creator knows what he's doing, but the end user
> >> doesn't always know or care about low level details like patch
> >> dependencies.  The easiest and safest way to protect the end user is the
> >> current approach, which assumes that each patch depends on all
> >> previously applied patches.
> > 
> > But then, the feature that disable patch dynamically is useless.
> > For example, if user find a bug be introduced by the last patch and disable
> > it directly, the new patch is no longer allowed from now unless enable the
> > old patch firstly but there is a risk window by this way.
> > 
> 
> Ok, in this case we can unregister the old patch firstly.
> But it seems that the feature that enable/disable patch dynamically indeed
> useless. (Its value is only for the last patch to enable or disable.)

I wouldn't say it's useless... It's just a patch stack.  If there's a
bug at the bottom of the stack, you can either:

1) push a new patch which does the opposite of the original patch
   (similar to how "git revert" adds a new commit);

2) or pop everything off the stack and create a new stack to your
   liking.

It doesn't actually prevent you from doing what you want, it just makes
it less convenient (and more safe IMO).

I suppose an alternative would be to allow the patch creator to specify
patch dependencies (in addition to something like your patch to catch
the obvious dependencies).  But dependencies are tricky and I'm not
really convinced that would be worth the added risk and code complexity.

-- 
Josh

Re: [PATCH 2/2] livepatch: disable/enable_patch manners for interdependent patches

[Li Bin]

On 2015/1/22 21:05, Josh Poimboeuf wrote:
> On Thu, Jan 22, 2015 at 05:54:23PM +0800, Li Bin wrote:
>> On 2015/1/22 16:39, Li Bin wrote:
>>> On 2015/1/22 11:51, Josh Poimboeuf wrote:
>>>> On Thu, Jan 22, 2015 at 08:42:29AM +0800, Li Bin wrote:
>>>>> On 2015/1/21 22:08, Jiri Kosina wrote:
>>>>>> On Wed, 21 Jan 2015, Li Bin wrote:
>>>>>> By this you limit the definition of the patch inter-dependency to just 
>>>>>> symbols. But that's not the only way how patches can depend on it other -- 
>>>>>> the dependency can be semantical.
>>>>>
>>>>> Yes, I agree with you. But I think the other dependencies such as semantical
>>>>> dependency should be judged by the user, like reverting a patch from git repository.
>>>>> Right?
>>>>
>>>> But with live patching, there are two users: the patch creator (who
>>>> creates the patch module) and the end user (who loads it on their
>>>> system).
>>>>
>>>> We can assume the patch creator knows what he's doing, but the end user
>>>> doesn't always know or care about low level details like patch
>>>> dependencies.  The easiest and safest way to protect the end user is the
>>>> current approach, which assumes that each patch depends on all
>>>> previously applied patches.
>>>
>>> But then, the feature that disable patch dynamically is useless.
>>> For example, if user find a bug be introduced by the last patch and disable
>>> it directly, the new patch is no longer allowed from now unless enable the
>>> old patch firstly but there is a risk window by this way.
>>>
>>
>> Ok, in this case we can unregister the old patch firstly.
>> But it seems that the feature that enable/disable patch dynamically indeed
>> useless. (Its value is only for the last patch to enable or disable.)
> 
> I wouldn't say it's useless... It's just a patch stack.  If there's a
> bug at the bottom of the stack, you can either:
> 
> 1) push a new patch which does the opposite of the original patch
>    (similar to how "git revert" adds a new commit);
> 
> 2) or pop everything off the stack and create a new stack to your
>    liking.
> 
> It doesn't actually prevent you from doing what you want, it just makes
> it less convenient (and more safe IMO).
> 

I am not arguing the value of the patch stack manner, but the sysfs attribute
'enabled'.

> I suppose an alternative would be to allow the patch creator to specify
> patch dependencies (in addition to something like your patch to catch
> the obvious dependencies).  But dependencies are tricky and I'm not
> really convinced that would be worth the added risk and code complexity.

Yes, since we can not assume that end users know the low level details, but they
have permission to unregister the stacktop patch, and this action will affect the
subsequent patch without providing dependencies information.

>