[PATCH 0/3] jump_label: Robustify jump label patching

[PATCH 0/3] jump_label: Robustify jump label patching

[Josh Poimboeuf]

Warn if a jump_label fails to patch, and fix up a couple of issues found
by the warning.

Josh Poimboeuf (3):
  jump_label: Warn on failed jump_label patch
  x86/xen: Remove inert tracepoint in __init code
  percpu: Remove inert tracepoint in __init code

 arch/x86/xen/enlighten_pv.c | 2 --
 kernel/jump_label.c         | 8 ++++++--
 mm/percpu.c                 | 1 -
 3 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.14.3

[PATCH 1/3] jump_label: Warn on failed jump_label patch

[Josh Poimboeuf]

When the jump label code encounters an address which isn't recognized by
kernel_text_address(), it just silently fails.

This can be dangerous because jump labels are used in a variety of
places, and are generally expected to work.  Convert the silent failure
to a warning.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/jump_label.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index b4517095db6a..c71fb7cdfc41 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -370,8 +370,12 @@ static void __jump_label_update(struct static_key *key,
 		 * kernel_text_address() verifies we are not in core kernel
 		 * init code, see jump_label_invalidate_module_init().
 		 */
-		if (entry->code && kernel_text_address(entry->code))
-			arch_jump_label_transform(entry, jump_label_type(entry));
+		if (entry->code) {
+			if (kernel_text_address(entry->code))
+				arch_jump_label_transform(entry, jump_label_type(entry));
+			else
+				WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
+		}
 	}
 }
 
-- 
2.14.3

[PATCH 2/3] x86/xen: Remove inert tracepoint in __init code

[Josh Poimboeuf]

The jump_label code doesn't patch init code, so this tracepoint can
never be enabled.  Remove it.

Cc: Juergen Gross <jgross@suse.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/xen/enlighten_pv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c047f42552e1..0dbfad92f442 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -803,8 +803,6 @@ static void xen_write_gdt_entry(struct desc_struct *dt, int entry,
 static void __init xen_write_gdt_entry_boot(struct desc_struct *dt, int entry,
 					    const void *desc, int type)
 {
-	trace_xen_cpu_write_gdt_entry(dt, entry, desc, type);
-
 	switch (type) {
 	case DESC_LDT:
 	case DESC_TSS:
-- 
2.14.3

[PATCH 3/3] percpu: Remove inert tracepoint in __init code

[Josh Poimboeuf]

The jump_label code doesn't patch init code, so this tracepoint can
never be enabled.  Remove it.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 mm/percpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index 50e7fdf84055..3216364ad91b 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2156,7 +2156,6 @@ int __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	pcpu_chunk_relocate(pcpu_first_chunk, -1);
 
 	pcpu_stats_chunk_alloc();
-	trace_percpu_create_chunk(base_addr);
 
 	/* we're done */
 	pcpu_base_addr = base_addr;
-- 
2.14.3

Re: [PATCH 2/3] x86/xen: Remove inert tracepoint in __init code

[Juergen Gross]

On 14/02/18 17:40, Josh Poimboeuf wrote:
> The jump_label code doesn't patch init code, so this tracepoint can
> never be enabled.  Remove it.
> 
> Cc: Juergen Gross <jgross@suse.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Steven Rostedt]

On Wed, 14 Feb 2018 10:40:41 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> When the jump label code encounters an address which isn't recognized by
> kernel_text_address(), it just silently fails.
> 
> This can be dangerous because jump labels are used in a variety of
> places, and are generally expected to work.  Convert the silent failure
> to a warning.

I made ftrace function tracing work on init code, can we do the same
with tracepoints (aka jump labels)?

But I have to say that this goes with my argument that there exists
tracepoints in the kernel that nobody cares about ;-)

-- Steve

> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  kernel/jump_label.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index b4517095db6a..c71fb7cdfc41 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -370,8 +370,12 @@ static void __jump_label_update(struct static_key *key,
>  		 * kernel_text_address() verifies we are not in core kernel
>  		 * init code, see jump_label_invalidate_module_init().
>  		 */
> -		if (entry->code && kernel_text_address(entry->code))
> -			arch_jump_label_transform(entry, jump_label_type(entry));
> +		if (entry->code) {
> +			if (kernel_text_address(entry->code))
> +				arch_jump_label_transform(entry, jump_label_type(entry));
> +			else
> +				WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
> +		}
>  	}
>  }
>  

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Borislav Petkov]

On Wed, Feb 14, 2018 at 10:40:41AM -0600, Josh Poimboeuf wrote:
> When the jump label code encounters an address which isn't recognized by
> kernel_text_address(), it just silently fails.
> 
> This can be dangerous because jump labels are used in a variety of
> places, and are generally expected to work.  Convert the silent failure
> to a warning.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  kernel/jump_label.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index b4517095db6a..c71fb7cdfc41 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -370,8 +370,12 @@ static void __jump_label_update(struct static_key *key,
>  		 * kernel_text_address() verifies we are not in core kernel
>  		 * init code, see jump_label_invalidate_module_init().
>  		 */
> -		if (entry->code && kernel_text_address(entry->code))
> -			arch_jump_label_transform(entry, jump_label_type(entry));
> +		if (entry->code) {
> +			if (kernel_text_address(entry->code))
> +				arch_jump_label_transform(entry, jump_label_type(entry));
> +			else
> +				WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
> +		}
>  	}
>  }
>  
> -- 
> 2.14.3

Good idea.

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Jason Baron]

On 02/14/2018 12:01 PM, Steven Rostedt wrote:
> On Wed, 14 Feb 2018 10:40:41 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
>> When the jump label code encounters an address which isn't recognized by
>> kernel_text_address(), it just silently fails.
>>
>> This can be dangerous because jump labels are used in a variety of
>> places, and are generally expected to work.  Convert the silent failure
>> to a warning.
> 
> I made ftrace function tracing work on init code, can we do the same
> with tracepoints (aka jump labels)?
> 

jump labels do work on init code, except they don't work on it after it
has been freed :)

It uses 'kernel_text_address()', which will return true for init code if
system_state < SYSTEM_RUNNING. See: core_kernel_text().

So I'm guessing that the warnings here are coming from init code that
has already been freed. Are we sure that these warnings are coming from
init code that hasn't already been freed?

Thanks,

-Jason

> But I have to say that this goes with my argument that there exists
> tracepoints in the kernel that nobody cares about ;-)
> 
> -- Steve
> 
>>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>>  kernel/jump_label.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
>> index b4517095db6a..c71fb7cdfc41 100644
>> --- a/kernel/jump_label.c
>> +++ b/kernel/jump_label.c
>> @@ -370,8 +370,12 @@ static void __jump_label_update(struct static_key *key,
>>  		 * kernel_text_address() verifies we are not in core kernel
>>  		 * init code, see jump_label_invalidate_module_init().
>>  		 */
>> -		if (entry->code && kernel_text_address(entry->code))
>> -			arch_jump_label_transform(entry, jump_label_type(entry));
>> +		if (entry->code) {
>> +			if (kernel_text_address(entry->code))
>> +				arch_jump_label_transform(entry, jump_label_type(entry));
>> +			else
>> +				WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code);
>> +		}
>>  	}
>>  }
>>  
> 

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Josh Poimboeuf]

On Wed, Feb 14, 2018 at 12:18:20PM -0500, Jason Baron wrote:
> 
> 
> On 02/14/2018 12:01 PM, Steven Rostedt wrote:
> > On Wed, 14 Feb 2018 10:40:41 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> >> When the jump label code encounters an address which isn't recognized by
> >> kernel_text_address(), it just silently fails.
> >>
> >> This can be dangerous because jump labels are used in a variety of
> >> places, and are generally expected to work.  Convert the silent failure
> >> to a warning.
> > 
> > I made ftrace function tracing work on init code, can we do the same
> > with tracepoints (aka jump labels)?
> > 
> 
> jump labels do work on init code, except they don't work on it after it
> has been freed :)
> 
> It uses 'kernel_text_address()', which will return true for init code if
> system_state < SYSTEM_RUNNING. See: core_kernel_text().
> 
> So I'm guessing that the warnings here are coming from init code that
> has already been freed. Are we sure that these warnings are coming from
> init code that hasn't already been freed?

Ah, I see.  I missed that 'system_state < SYSTEM_RUNNING' check.  It was
nicely hidden. :-)

So I guess patches 2 & 3 can be dropped, as those tracepoints can still
be used during boot.

And in patch 1 the warning conditions need to be tweaked a bit to
exclude the __init case.

-- 
Josh

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Steven Rostedt]

On Wed, 14 Feb 2018 11:34:07 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> And in patch 1 the warning conditions need to be tweaked a bit to
> exclude the __init case.
> 

Hmm, I wonder if we could simply remove the references from the jump
label table when init is freed.

-- Steve

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Josh Poimboeuf]

On Wed, Feb 14, 2018 at 12:40:34PM -0500, Steven Rostedt wrote:
> On Wed, 14 Feb 2018 11:34:07 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > And in patch 1 the warning conditions need to be tweaked a bit to
> > exclude the __init case.
> > 
> 
> Hmm, I wonder if we could simply remove the references from the jump
> label table when init is freed.

Interesting idea.  The static key's entries (i.e. tracepoint sites) are
in a static array.  Maybe we could rewrite the affected arrays when init
memory is freed.  I'm not sure whether that would be overkill though.  I
can tinker with it and see how it compares to the current solution.

-- 
Josh

Re: [PATCH 3/3] percpu: Remove inert tracepoint in __init code

[Tejun Heo]

On Wed, Feb 14, 2018 at 10:40:43AM -0600, Josh Poimboeuf wrote:
> The jump_label code doesn't patch init code, so this tracepoint can
> never be enabled.  Remove it.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Applied to percpu/for-4.17.

Thanks.

-- 
tejun

Re: [PATCH 3/3] percpu: Remove inert tracepoint in __init code

[Josh Poimboeuf]

On Thu, Feb 15, 2018 at 07:22:46AM -0800, Tejun Heo wrote:
> On Wed, Feb 14, 2018 at 10:40:43AM -0600, Josh Poimboeuf wrote:
> > The jump_label code doesn't patch init code, so this tracepoint can
> > never be enabled.  Remove it.
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Applied to percpu/for-4.17.

Actually, I think this patch should be dropped.  As Jason pointed out,
tracepoints *can* be enabled at boot time for __init code, so it's
possible for this tracepoint to be used.

-- 
Josh

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Josh Poimboeuf]

On Wed, Feb 14, 2018 at 03:17:07PM -0600, Josh Poimboeuf wrote:
> On Wed, Feb 14, 2018 at 12:40:34PM -0500, Steven Rostedt wrote:
> > On Wed, 14 Feb 2018 11:34:07 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > 
> > > And in patch 1 the warning conditions need to be tweaked a bit to
> > > exclude the __init case.
> > > 
> > 
> > Hmm, I wonder if we could simply remove the references from the jump
> > label table when init is freed.
> 
> Interesting idea.  The static key's entries (i.e. tracepoint sites) are
> in a static array.  Maybe we could rewrite the affected arrays when init
> memory is freed.  I'm not sure whether that would be overkill though.  I
> can tinker with it and see how it compares to the current solution.

It turns out that something like this is already done for module init
code.  It doesn't *remove* the entry, but it does disable it by setting
entry->code to zero.  I'll just do the same thing right before initmem
is freed.

-- 
Josh

Re: [PATCH 3/3] percpu: Remove inert tracepoint in __init code

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Thu, Feb 15, 2018 at 07:22:46AM -0800, Tejun Heo wrote:
> > On Wed, Feb 14, 2018 at 10:40:43AM -0600, Josh Poimboeuf wrote:
> > > The jump_label code doesn't patch init code, so this tracepoint can
> > > never be enabled.  Remove it.
> > > 
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > Applied to percpu/for-4.17.
> 
> Actually, I think this patch should be dropped.  As Jason pointed out,
> tracepoints *can* be enabled at boot time for __init code, so it's
> possible for this tracepoint to be used.

Would it still be useful to have the warning for failed patching?

Thanks,

	Ingo

Re: [PATCH 3/3] percpu: Remove inert tracepoint in __init code

[Josh Poimboeuf]

On Fri, Feb 16, 2018 at 10:02:33AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Thu, Feb 15, 2018 at 07:22:46AM -0800, Tejun Heo wrote:
> > > On Wed, Feb 14, 2018 at 10:40:43AM -0600, Josh Poimboeuf wrote:
> > > > The jump_label code doesn't patch init code, so this tracepoint can
> > > > never be enabled.  Remove it.
> > > > 
> > > > Cc: Tejun Heo <tj@kernel.org>
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > 
> > > Applied to percpu/for-4.17.
> > 
> > Actually, I think this patch should be dropped.  As Jason pointed out,
> > tracepoints *can* be enabled at boot time for __init code, so it's
> > possible for this tracepoint to be used.
> 
> Would it still be useful to have the warning for failed patching?

Yes, I'm working on a v2.

-- 
Josh

Re: [PATCH 1/3] jump_label: Warn on failed jump_label patch

[Steven Rostedt]

On Thu, 15 Feb 2018 16:35:58 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> > Interesting idea.  The static key's entries (i.e. tracepoint sites) are
> > in a static array.  Maybe we could rewrite the affected arrays when init
> > memory is freed.  I'm not sure whether that would be overkill though.  I
> > can tinker with it and see how it compares to the current solution.  
> 
> It turns out that something like this is already done for module init
> code.  It doesn't *remove* the entry, but it does disable it by setting
> entry->code to zero.  I'll just do the same thing right before initmem
> is freed.

Yes, I was actually going to mention to do it like the module code does.

Thanks!

-- Steve