Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-06 Thread Petr Mladek 
On Mon 2018-03-05 10:54:16, Miroslav Benes wrote:
> On Fri, 2 Mar 2018, Joe Lawrence wrote:
> 
> > On 03/01/2018 05:28 AM, Petr Mladek wrote:
> > > On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> > >> On Wed, 21 Feb 2018, Petr Mladek wrote:
> > >>> This patch allows the late initialization.
> > >>>
> > >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > >>> index ad508a86b2f9..da1438d47d83 100644
> > >>> --- a/kernel/livepatch/core.c
> > >>> +++ b/kernel/livepatch/core.c
> > >>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> > >>>  
> > >>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > >>>  {
> > >>> -   if (!func->old_name || !func->new_func)
> > >>> +   if (!func->old_name)
> > >>> +   return -EINVAL;
> > >>> +
> > >>> +   /* NOPs do not know the address until the patched module is 
> > >>> loaded. */
> > >>> +   if (!func->new_func &&
> > >>> +   (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> > >>> klp_is_object_loaded(obj)))
> > >>> return -EINVAL;
> > >>
> > >> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> > >> calls in klp_init_object(), the hunk would not be needed. Is that 
> > >> correct? 
> > > 
> > > Not really. klp_init_object_loaded() would set func->new_func only
> > > when the object was loaded. But we want to proceed here and create
> > > the kobject for NOPs even when it was not loaded.

Anyway, the above check have to be updated after we removed the
redundant func->new_func assignment in klp_alloc_func_nop().

func->new_func is always NULL for NOPs in klp_init_func().
It is set later in klp_init_object_loaded().

I am going to use the following check in v10:

/*
 * NOPs get the address later. The the patched module must be loaded,
 * see klp_init_object_loaded().
 */
if (!func->new_func && !klp_is_func_type(func, KLP_FUNC_NOP))
return -EINVAL;

> > > 
> > >>> INIT_LIST_HEAD(>stack_node);
> > >>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct 
> > >>> klp_patch *patch,
> > >>> return -ENOENT;
> > >>> }
> > >>>  
> > >>> +   if (klp_is_func_type(func, KLP_FUNC_NOP))
> > >>> +   func->new_func = (void *)func->old_addr;
> > >>
> > >> Is there a reason why you left the same assignment in 
> > >> klp_alloc_func_nop()? This one is enough, no?
> > > 
> > > Good point! I am going to replace the obsolete assignment
> > > with a comment in v8.
> > 
> > Hi Petr, Miroslav,
> > 
> > I don't think the assignment in klp_alloc_func_nop() was necessarily
> > redundant.  It was removed in v9 and that breaks my atomic replace
> > sample module when I try to load it.  (Perhaps the sample patch has
> > issues, but here are my debug notes):

Sigh, I hate "trivial" last minute clean ups ;-)


> Ok, so it was not redundant. Or... I think it should be redundant, but we 
> need to define the if condition in klp_init_func() properly.

Exactly, see above.


> > I think this problem is contained to only replacement patches that need
> > the nop-revert feature... if the replacement patch provides a new
> > function definition, then it shouldn't be affected.
> > 
> > Man, we need a regression test suite for all these cases :)

It would be great...

> Any volunteer?

?

Best Regards,
Petr

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-06 Thread Petr Mladek 
On Mon 2018-03-05 10:54:16, Miroslav Benes wrote:
> On Fri, 2 Mar 2018, Joe Lawrence wrote:
> 
> > On 03/01/2018 05:28 AM, Petr Mladek wrote:
> > > On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> > >> On Wed, 21 Feb 2018, Petr Mladek wrote:
> > >>> This patch allows the late initialization.
> > >>>
> > >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > >>> index ad508a86b2f9..da1438d47d83 100644
> > >>> --- a/kernel/livepatch/core.c
> > >>> +++ b/kernel/livepatch/core.c
> > >>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> > >>>  
> > >>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > >>>  {
> > >>> -   if (!func->old_name || !func->new_func)
> > >>> +   if (!func->old_name)
> > >>> +   return -EINVAL;
> > >>> +
> > >>> +   /* NOPs do not know the address until the patched module is 
> > >>> loaded. */
> > >>> +   if (!func->new_func &&
> > >>> +   (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> > >>> klp_is_object_loaded(obj)))
> > >>> return -EINVAL;
> > >>
> > >> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> > >> calls in klp_init_object(), the hunk would not be needed. Is that 
> > >> correct? 
> > > 
> > > Not really. klp_init_object_loaded() would set func->new_func only
> > > when the object was loaded. But we want to proceed here and create
> > > the kobject for NOPs even when it was not loaded.

Anyway, the above check have to be updated after we removed the
redundant func->new_func assignment in klp_alloc_func_nop().

func->new_func is always NULL for NOPs in klp_init_func().
It is set later in klp_init_object_loaded().

I am going to use the following check in v10:

/*
 * NOPs get the address later. The the patched module must be loaded,
 * see klp_init_object_loaded().
 */
if (!func->new_func && !klp_is_func_type(func, KLP_FUNC_NOP))
return -EINVAL;

> > > 
> > >>> INIT_LIST_HEAD(>stack_node);
> > >>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct 
> > >>> klp_patch *patch,
> > >>> return -ENOENT;
> > >>> }
> > >>>  
> > >>> +   if (klp_is_func_type(func, KLP_FUNC_NOP))
> > >>> +   func->new_func = (void *)func->old_addr;
> > >>
> > >> Is there a reason why you left the same assignment in 
> > >> klp_alloc_func_nop()? This one is enough, no?
> > > 
> > > Good point! I am going to replace the obsolete assignment
> > > with a comment in v8.
> > 
> > Hi Petr, Miroslav,
> > 
> > I don't think the assignment in klp_alloc_func_nop() was necessarily
> > redundant.  It was removed in v9 and that breaks my atomic replace
> > sample module when I try to load it.  (Perhaps the sample patch has
> > issues, but here are my debug notes):

Sigh, I hate "trivial" last minute clean ups ;-)


> Ok, so it was not redundant. Or... I think it should be redundant, but we 
> need to define the if condition in klp_init_func() properly.

Exactly, see above.


> > I think this problem is contained to only replacement patches that need
> > the nop-revert feature... if the replacement patch provides a new
> > function definition, then it shouldn't be affected.
> > 
> > Man, we need a regression test suite for all these cases :)

It would be great...

> Any volunteer?

?

Best Regards,
Petr

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-05 Thread Josh Poimboeuf 
On Mon, Mar 05, 2018 at 10:54:16AM +0100, Miroslav Benes wrote:
> > I think this problem is contained to only replacement patches that need
> > the nop-revert feature... if the replacement patch provides a new
> > function definition, then it shouldn't be affected.
> > 
> > Man, we need a regression test suite for all these cases :)
> 
> Any volunteer?

I'd strongly prefer that we get such selftests in place before merging
any more major features...

-- 
Josh

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-05 Thread Josh Poimboeuf 
On Mon, Mar 05, 2018 at 10:54:16AM +0100, Miroslav Benes wrote:
> > I think this problem is contained to only replacement patches that need
> > the nop-revert feature... if the replacement patch provides a new
> > function definition, then it shouldn't be affected.
> > 
> > Man, we need a regression test suite for all these cases :)
> 
> Any volunteer?

I'd strongly prefer that we get such selftests in place before merging
any more major features...

-- 
Josh

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-05 Thread Miroslav Benes 
On Fri, 2 Mar 2018, Joe Lawrence wrote:

> On 03/01/2018 05:28 AM, Petr Mladek wrote:
> > On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> >> On Wed, 21 Feb 2018, Petr Mladek wrote:
> >>> This patch allows the late initialization.
> >>>
> >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >>> index ad508a86b2f9..da1438d47d83 100644
> >>> --- a/kernel/livepatch/core.c
> >>> +++ b/kernel/livepatch/core.c
> >>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >>>  
> >>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >>>  {
> >>> - if (!func->old_name || !func->new_func)
> >>> + if (!func->old_name)
> >>> + return -EINVAL;
> >>> +
> >>> + /* NOPs do not know the address until the patched module is loaded. */
> >>> + if (!func->new_func &&
> >>> + (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> >>> klp_is_object_loaded(obj)))
> >>>   return -EINVAL;
> >>
> >> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> >> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> > 
> > Not really. klp_init_object_loaded() would set func->new_func only
> > when the object was loaded. But we want to proceed here and create
> > the kobject for NOPs even when it was not loaded.
> > 
> > 
> >>>   INIT_LIST_HEAD(>stack_node);
> >>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
> >>> *patch,
> >>>   return -ENOENT;
> >>>   }
> >>>  
> >>> + if (klp_is_func_type(func, KLP_FUNC_NOP))
> >>> + func->new_func = (void *)func->old_addr;
> >>
> >> Is there a reason why you left the same assignment in 
> >> klp_alloc_func_nop()? This one is enough, no?
> > 
> > Good point! I am going to replace the obsolete assignment
> > with a comment in v8.
> 
> Hi Petr, Miroslav,
> 
> I don't think the assignment in klp_alloc_func_nop() was necessarily
> redundant.  It was removed in v9 and that breaks my atomic replace
> sample module when I try to load it.  (Perhaps the sample patch has
> issues, but here are my debug notes):
> 
> To recap:
> 
>   patch 1 - modifies cmdline_proc_show()
>   patch 2 - modifies only meminfo_proc_show()
> 
> when I load patch 2 with .replace=1, klp_add_nops() is called and this
> adds a nop function to patch 2 so it reverts cmdline_proc_show():
> 
> klp_init_patch()
>   if (patch->replace)
> klp_add_nops()
>   list_for_each_entry(old_patch, _patches, list) {
> klp_for_each_object(old_patch, old_obj) {
>   klp_add_object_nops()
> klp_add_func_nop()
>   klp_alloc_func_nop()
> 
> the patch continues initialization and I hit the second -EINVAL
> condition on that nop function in klp_init_func():
> 
>   klp_init_object
> klp_init_func
> 
> (from added debug):
> [   48.456980] livepatch: func->old_name=cmdline_proc_show
> [   48.457620] livepatch: func->new_func=  (null)
> [   48.458042] livepatch: klp_is_func_type(func, KLP_FUNC_NOP)=1
> [   48.458573] livepatch: klp_is_object_loaded(obj)=1
> 
> If I restore the assignment of func->new_func to klp_alloc_func_nop()
> then the replacement patch properly loads.  (Reordering the code may
> have similar effect?)

Ok, so it was not redundant. Or... I think it should be redundant, but we 
need to define the if condition in klp_init_func() properly.
 
> I think this problem is contained to only replacement patches that need
> the nop-revert feature... if the replacement patch provides a new
> function definition, then it shouldn't be affected.
> 
> Man, we need a regression test suite for all these cases :)

Any volunteer?

Miroslav

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-05 Thread Miroslav Benes 
On Fri, 2 Mar 2018, Joe Lawrence wrote:

> On 03/01/2018 05:28 AM, Petr Mladek wrote:
> > On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> >> On Wed, 21 Feb 2018, Petr Mladek wrote:
> >>> This patch allows the late initialization.
> >>>
> >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >>> index ad508a86b2f9..da1438d47d83 100644
> >>> --- a/kernel/livepatch/core.c
> >>> +++ b/kernel/livepatch/core.c
> >>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >>>  
> >>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >>>  {
> >>> - if (!func->old_name || !func->new_func)
> >>> + if (!func->old_name)
> >>> + return -EINVAL;
> >>> +
> >>> + /* NOPs do not know the address until the patched module is loaded. */
> >>> + if (!func->new_func &&
> >>> + (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> >>> klp_is_object_loaded(obj)))
> >>>   return -EINVAL;
> >>
> >> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> >> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> > 
> > Not really. klp_init_object_loaded() would set func->new_func only
> > when the object was loaded. But we want to proceed here and create
> > the kobject for NOPs even when it was not loaded.
> > 
> > 
> >>>   INIT_LIST_HEAD(>stack_node);
> >>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
> >>> *patch,
> >>>   return -ENOENT;
> >>>   }
> >>>  
> >>> + if (klp_is_func_type(func, KLP_FUNC_NOP))
> >>> + func->new_func = (void *)func->old_addr;
> >>
> >> Is there a reason why you left the same assignment in 
> >> klp_alloc_func_nop()? This one is enough, no?
> > 
> > Good point! I am going to replace the obsolete assignment
> > with a comment in v8.
> 
> Hi Petr, Miroslav,
> 
> I don't think the assignment in klp_alloc_func_nop() was necessarily
> redundant.  It was removed in v9 and that breaks my atomic replace
> sample module when I try to load it.  (Perhaps the sample patch has
> issues, but here are my debug notes):
> 
> To recap:
> 
>   patch 1 - modifies cmdline_proc_show()
>   patch 2 - modifies only meminfo_proc_show()
> 
> when I load patch 2 with .replace=1, klp_add_nops() is called and this
> adds a nop function to patch 2 so it reverts cmdline_proc_show():
> 
> klp_init_patch()
>   if (patch->replace)
> klp_add_nops()
>   list_for_each_entry(old_patch, _patches, list) {
> klp_for_each_object(old_patch, old_obj) {
>   klp_add_object_nops()
> klp_add_func_nop()
>   klp_alloc_func_nop()
> 
> the patch continues initialization and I hit the second -EINVAL
> condition on that nop function in klp_init_func():
> 
>   klp_init_object
> klp_init_func
> 
> (from added debug):
> [   48.456980] livepatch: func->old_name=cmdline_proc_show
> [   48.457620] livepatch: func->new_func=  (null)
> [   48.458042] livepatch: klp_is_func_type(func, KLP_FUNC_NOP)=1
> [   48.458573] livepatch: klp_is_object_loaded(obj)=1
> 
> If I restore the assignment of func->new_func to klp_alloc_func_nop()
> then the replacement patch properly loads.  (Reordering the code may
> have similar effect?)

Ok, so it was not redundant. Or... I think it should be redundant, but we 
need to define the if condition in klp_init_func() properly.
 
> I think this problem is contained to only replacement patches that need
> the nop-revert feature... if the replacement patch provides a new
> function definition, then it shouldn't be affected.
> 
> Man, we need a regression test suite for all these cases :)

Any volunteer?

Miroslav

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-02 Thread Joe Lawrence 
On 03/01/2018 05:28 AM, Petr Mladek wrote:
> On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
>> On Wed, 21 Feb 2018, Petr Mladek wrote:
>>> This patch allows the late initialization.
>>>
>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>> index ad508a86b2f9..da1438d47d83 100644
>>> --- a/kernel/livepatch/core.c
>>> +++ b/kernel/livepatch/core.c
>>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
>>>  
>>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>>>  {
>>> -   if (!func->old_name || !func->new_func)
>>> +   if (!func->old_name)
>>> +   return -EINVAL;
>>> +
>>> +   /* NOPs do not know the address until the patched module is loaded. */
>>> +   if (!func->new_func &&
>>> +   (!klp_is_func_type(func, KLP_FUNC_NOP) || 
>>> klp_is_object_loaded(obj)))
>>> return -EINVAL;
>>
>> If we changed the order of klp_init_func() and klp_init_object_loaded() 
>> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> 
> Not really. klp_init_object_loaded() would set func->new_func only
> when the object was loaded. But we want to proceed here and create
> the kobject for NOPs even when it was not loaded.
> 
> 
>>> INIT_LIST_HEAD(>stack_node);
>>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
>>> *patch,
>>> return -ENOENT;
>>> }
>>>  
>>> +   if (klp_is_func_type(func, KLP_FUNC_NOP))
>>> +   func->new_func = (void *)func->old_addr;
>>
>> Is there a reason why you left the same assignment in 
>> klp_alloc_func_nop()? This one is enough, no?
> 
> Good point! I am going to replace the obsolete assignment
> with a comment in v8.

Hi Petr, Miroslav,

I don't think the assignment in klp_alloc_func_nop() was necessarily
redundant.  It was removed in v9 and that breaks my atomic replace
sample module when I try to load it.  (Perhaps the sample patch has
issues, but here are my debug notes):

To recap:

  patch 1 - modifies cmdline_proc_show()
  patch 2 - modifies only meminfo_proc_show()

when I load patch 2 with .replace=1, klp_add_nops() is called and this
adds a nop function to patch 2 so it reverts cmdline_proc_show():

klp_init_patch()
  if (patch->replace)
klp_add_nops()
  list_for_each_entry(old_patch, _patches, list) {
klp_for_each_object(old_patch, old_obj) {
  klp_add_object_nops()
klp_add_func_nop()
  klp_alloc_func_nop()

the patch continues initialization and I hit the second -EINVAL
condition on that nop function in klp_init_func():

  klp_init_object
klp_init_func

(from added debug):
[   48.456980] livepatch: func->old_name=cmdline_proc_show
[   48.457620] livepatch: func->new_func=  (null)
[   48.458042] livepatch: klp_is_func_type(func, KLP_FUNC_NOP)=1
[   48.458573] livepatch: klp_is_object_loaded(obj)=1

If I restore the assignment of func->new_func to klp_alloc_func_nop()
then the replacement patch properly loads.  (Reordering the code may
have similar effect?)

I think this problem is contained to only replacement patches that need
the nop-revert feature... if the replacement patch provides a new
function definition, then it shouldn't be affected.

Man, we need a regression test suite for all these cases :)

-- Joe

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-02 Thread Joe Lawrence 
On 03/01/2018 05:28 AM, Petr Mladek wrote:
> On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
>> On Wed, 21 Feb 2018, Petr Mladek wrote:
>>> This patch allows the late initialization.
>>>
>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>> index ad508a86b2f9..da1438d47d83 100644
>>> --- a/kernel/livepatch/core.c
>>> +++ b/kernel/livepatch/core.c
>>> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
>>>  
>>>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>>>  {
>>> -   if (!func->old_name || !func->new_func)
>>> +   if (!func->old_name)
>>> +   return -EINVAL;
>>> +
>>> +   /* NOPs do not know the address until the patched module is loaded. */
>>> +   if (!func->new_func &&
>>> +   (!klp_is_func_type(func, KLP_FUNC_NOP) || 
>>> klp_is_object_loaded(obj)))
>>> return -EINVAL;
>>
>> If we changed the order of klp_init_func() and klp_init_object_loaded() 
>> calls in klp_init_object(), the hunk would not be needed. Is that correct? 
> 
> Not really. klp_init_object_loaded() would set func->new_func only
> when the object was loaded. But we want to proceed here and create
> the kobject for NOPs even when it was not loaded.
> 
> 
>>> INIT_LIST_HEAD(>stack_node);
>>> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
>>> *patch,
>>> return -ENOENT;
>>> }
>>>  
>>> +   if (klp_is_func_type(func, KLP_FUNC_NOP))
>>> +   func->new_func = (void *)func->old_addr;
>>
>> Is there a reason why you left the same assignment in 
>> klp_alloc_func_nop()? This one is enough, no?
> 
> Good point! I am going to replace the obsolete assignment
> with a comment in v8.

Hi Petr, Miroslav,

I don't think the assignment in klp_alloc_func_nop() was necessarily
redundant.  It was removed in v9 and that breaks my atomic replace
sample module when I try to load it.  (Perhaps the sample patch has
issues, but here are my debug notes):

To recap:

  patch 1 - modifies cmdline_proc_show()
  patch 2 - modifies only meminfo_proc_show()

when I load patch 2 with .replace=1, klp_add_nops() is called and this
adds a nop function to patch 2 so it reverts cmdline_proc_show():

klp_init_patch()
  if (patch->replace)
klp_add_nops()
  list_for_each_entry(old_patch, _patches, list) {
klp_for_each_object(old_patch, old_obj) {
  klp_add_object_nops()
klp_add_func_nop()
  klp_alloc_func_nop()

the patch continues initialization and I hit the second -EINVAL
condition on that nop function in klp_init_func():

  klp_init_object
klp_init_func

(from added debug):
[   48.456980] livepatch: func->old_name=cmdline_proc_show
[   48.457620] livepatch: func->new_func=  (null)
[   48.458042] livepatch: klp_is_func_type(func, KLP_FUNC_NOP)=1
[   48.458573] livepatch: klp_is_object_loaded(obj)=1

If I restore the assignment of func->new_func to klp_alloc_func_nop()
then the replacement patch properly loads.  (Reordering the code may
have similar effect?)

I think this problem is contained to only replacement patches that need
the nop-revert feature... if the replacement patch provides a new
function definition, then it shouldn't be affected.

Man, we need a regression test suite for all these cases :)

-- Joe

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-01 Thread Petr Mladek 
On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> On Wed, 21 Feb 2018, Petr Mladek wrote:
> > This patch allows the late initialization.
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index ad508a86b2f9..da1438d47d83 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >  
> >  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  {
> > -   if (!func->old_name || !func->new_func)
> > +   if (!func->old_name)
> > +   return -EINVAL;
> > +
> > +   /* NOPs do not know the address until the patched module is loaded. */
> > +   if (!func->new_func &&
> > +   (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> > klp_is_object_loaded(obj)))
> > return -EINVAL;
> 
> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> calls in klp_init_object(), the hunk would not be needed. Is that correct? 

Not really. klp_init_object_loaded() would set func->new_func only
when the object was loaded. But we want to proceed here and create
the kobject for NOPs even when it was not loaded.


> > INIT_LIST_HEAD(>stack_node);
> > @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
> > *patch,
> > return -ENOENT;
> > }
> >  
> > +   if (klp_is_func_type(func, KLP_FUNC_NOP))
> > +   func->new_func = (void *)func->old_addr;
> 
> Is there a reason why you left the same assignment in 
> klp_alloc_func_nop()? This one is enough, no?

Good point! I am going to replace the obsolete assignment
with a comment in v8.

Best Regards,
Petr

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-03-01 Thread Petr Mladek 
On Thu 2018-02-22 22:00:28, Miroslav Benes wrote:
> On Wed, 21 Feb 2018, Petr Mladek wrote:
> > This patch allows the late initialization.
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index ad508a86b2f9..da1438d47d83 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >  
> >  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  {
> > -   if (!func->old_name || !func->new_func)
> > +   if (!func->old_name)
> > +   return -EINVAL;
> > +
> > +   /* NOPs do not know the address until the patched module is loaded. */
> > +   if (!func->new_func &&
> > +   (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> > klp_is_object_loaded(obj)))
> > return -EINVAL;
> 
> If we changed the order of klp_init_func() and klp_init_object_loaded() 
> calls in klp_init_object(), the hunk would not be needed. Is that correct? 

Not really. klp_init_object_loaded() would set func->new_func only
when the object was loaded. But we want to proceed here and create
the kobject for NOPs even when it was not loaded.


> > INIT_LIST_HEAD(>stack_node);
> > @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
> > *patch,
> > return -ENOENT;
> > }
> >  
> > +   if (klp_is_func_type(func, KLP_FUNC_NOP))
> > +   func->new_func = (void *)func->old_addr;
> 
> Is there a reason why you left the same assignment in 
> klp_alloc_func_nop()? This one is enough, no?

Good point! I am going to replace the obsolete assignment
with a comment in v8.

Best Regards,
Petr

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-02-22 Thread Miroslav Benes 
On Wed, 21 Feb 2018, Petr Mladek wrote:

> The atomic replace feature uses dynamically allocated struct klp_func to
> handle functions that will not longer be patched. These structures are

s/not longer/no longer/, but "handle functions that will not be patched 
any longer" may be even better.

> of the type KLP_FUNC_NOP. They cause the ftrace handler to jump to
> the original code. But the address of the original code is not known
> until the patched module is loaded.
> 
> This patch allows the late initialization. Also it adds a sanity check
> into the ftrace handler.
> 
> Alternative solution would be to do not set the address at all. The ftrace

"Alternative solution would be not to set the address at all." ?

> handler could just return to the original code when NOP struct klp_func
> is used. But this would require another changes. For example, in the stack
> checking. Note that NOP structures might be available even when the patch
> is being disabled. This would happen when the patch enable transition is
> reverted.
> 
> Signed-off-by: Petr Mladek 
> ---
>  kernel/livepatch/core.c  | 16 ++--
>  kernel/livepatch/patch.c |  5 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ad508a86b2f9..da1438d47d83 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -945,8 +945,12 @@ static void klp_free_object_loaded(struct klp_object 
> *obj)
>  
>   obj->mod = NULL;
>  
> - klp_for_each_func(obj, func)
> + klp_for_each_func(obj, func) {
>   func->old_addr = 0;
> +
> + if (klp_is_func_type(func, KLP_FUNC_NOP))
> + func->new_func = NULL;
> + }
>  }
>  
>  /*
> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
>  
>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  {
> - if (!func->old_name || !func->new_func)
> + if (!func->old_name)
> + return -EINVAL;
> +
> + /* NOPs do not know the address until the patched module is loaded. */
> + if (!func->new_func &&
> + (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> klp_is_object_loaded(obj)))
>   return -EINVAL;

If we changed the order of klp_init_func() and klp_init_object_loaded() 
calls in klp_init_object(), the hunk would not be needed. Is that correct? 
But that would require more changes elsewhere, so it's not worth it, I 
think.

>   INIT_LIST_HEAD(>stack_node);
> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
> *patch,
>   return -ENOENT;
>   }
>  
> + if (klp_is_func_type(func, KLP_FUNC_NOP))
> + func->new_func = (void *)func->old_addr;

Is there a reason why you left the same assignment in 
klp_alloc_func_nop()? This one is enough, no?

Miroslav

Re: [PATCH v8 7/8] livepatch: Correctly handle atomic replace for not yet loaded modules

2018-02-22 Thread Miroslav Benes 
On Wed, 21 Feb 2018, Petr Mladek wrote:

> The atomic replace feature uses dynamically allocated struct klp_func to
> handle functions that will not longer be patched. These structures are

s/not longer/no longer/, but "handle functions that will not be patched 
any longer" may be even better.

> of the type KLP_FUNC_NOP. They cause the ftrace handler to jump to
> the original code. But the address of the original code is not known
> until the patched module is loaded.
> 
> This patch allows the late initialization. Also it adds a sanity check
> into the ftrace handler.
> 
> Alternative solution would be to do not set the address at all. The ftrace

"Alternative solution would be not to set the address at all." ?

> handler could just return to the original code when NOP struct klp_func
> is used. But this would require another changes. For example, in the stack
> checking. Note that NOP structures might be available even when the patch
> is being disabled. This would happen when the patch enable transition is
> reverted.
> 
> Signed-off-by: Petr Mladek 
> ---
>  kernel/livepatch/core.c  | 16 ++--
>  kernel/livepatch/patch.c |  5 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ad508a86b2f9..da1438d47d83 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -945,8 +945,12 @@ static void klp_free_object_loaded(struct klp_object 
> *obj)
>  
>   obj->mod = NULL;
>  
> - klp_for_each_func(obj, func)
> + klp_for_each_func(obj, func) {
>   func->old_addr = 0;
> +
> + if (klp_is_func_type(func, KLP_FUNC_NOP))
> + func->new_func = NULL;
> + }
>  }
>  
>  /*
> @@ -984,7 +988,12 @@ static void klp_free_patch(struct klp_patch *patch)
>  
>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  {
> - if (!func->old_name || !func->new_func)
> + if (!func->old_name)
> + return -EINVAL;
> +
> + /* NOPs do not know the address until the patched module is loaded. */
> + if (!func->new_func &&
> + (!klp_is_func_type(func, KLP_FUNC_NOP) || 
> klp_is_object_loaded(obj)))
>   return -EINVAL;

If we changed the order of klp_init_func() and klp_init_object_loaded() 
calls in klp_init_object(), the hunk would not be needed. Is that correct? 
But that would require more changes elsewhere, so it's not worth it, I 
think.

>   INIT_LIST_HEAD(>stack_node);
> @@ -1039,6 +1048,9 @@ static int klp_init_object_loaded(struct klp_patch 
> *patch,
>   return -ENOENT;
>   }
>  
> + if (klp_is_func_type(func, KLP_FUNC_NOP))
> + func->new_func = (void *)func->old_addr;

Is there a reason why you left the same assignment in 
klp_alloc_func_nop()? This one is enough, no?

Miroslav