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

[Li Bin <huawei.libin@huawei.com>]

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

>