Re: [PATCH 0.5/3] livepatch: Convert stack entries array to percpu

[Petr Mladek <pmladek@suse.com>]

On Mon 2023-03-13 16:33:46, Josh Poimboeuf wrote:
> On Fri, Mar 03, 2023 at 03:00:13PM +0100, Petr Mladek wrote:
> > > MAX_STACK_ENTRIES is 100, which seems excessive.  If we halved that, the
> > > array would be "only" 400 bytes, which is *almost* reasonable to
> > > allocate on the stack?
> > 
> > It is just for the stack in the process context. Right?
> > 
> > I think that I have never seen a stack with over 50 entries. And in
> > the worst case, a bigger amount of entries would "just" result in
> > a non-reliable stack which might be acceptable.
> > 
> > It looks acceptable to me.
> > 
> > > Alternatively we could have a percpu entries array... :-/
> > 
> > That said, percpu entries would be fine as well. It sounds like
> > a good price for the livepatching feature. I think that livepatching
> > is used on big systems anyway.
> > 
> > I slightly prefer the per-cpu solution.
> 
> Booting a kernel with PREEMPT+LOCKDEP gave me a high-water mark of 60+
> stack entries, seen when probing a device.  I decided not to mess with
> MAX_STACK_ENTRIES, and instead just convert the entries to percpu.  This
> patch could be inserted at the beginning of the set.

Good to know.

> 
> ---8<---
> 
> Subject: [PATCH 0.5/3] livepatch: Convert stack entries array to percpu
> 
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -240,12 +242,15 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
>   */
>  static int klp_check_stack(struct task_struct *task, const char **oldname)
>  {
> -	static unsigned long entries[MAX_STACK_ENTRIES];
> +	unsigned long *entries = this_cpu_ptr(klp_stack_entries);
>  	struct klp_object *obj;
>  	struct klp_func *func;
>  	int ret, nr_entries;
>  
> -	ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries));
> +	/* Protect 'klp_stack_entries' */
> +	lockdep_assert_preemption_disabled();

I think about adding:

	/*
	 * Stay on the safe side even when cond_resched() is called from
	 * an IRQ context by mistake.
	 */
	if (!in_task())
		return -EINVAL;

Or is this prevented another way, please?

> +
> +	ret = stack_trace_save_tsk_reliable(task, entries, MAX_STACK_ENTRIES);
>  	if (ret < 0)
>  		return -EINVAL;
>  	nr_entries = ret;

Otherwise, it looks good to me.

Best Regards,
Petr