Re: [lockdep] b09be676e0 BUG: unable to handle kernel NULL pointer dereference at 000001f2

[Peter Zijlstra <peterz@infradead.org>]

> Fengguang, if you're still listening, could you please rerun the tests
> on top of ce07a9415f26, with the attached patches also applied?

Ping!? it would be very good to get feedback on this asap.

> From e7840ad76515f0b5061fcdd098b57b7c01b61482 Mon Sep 17 00:00:00 2001
> Message-Id: <e7840ad76515f0b5061fcdd098b57b7c01b61482.1507215196.git.jpoimboe@redhat.com>
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Date: Thu, 5 Oct 2017 09:43:59 -0500
> Subject: [PATCH 1/2] unwinder fixes
> 
> ---
>  arch/x86/kernel/unwind_frame.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index b9389d72b2f7..0ecc42e34cc4 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -33,7 +33,7 @@ static void unwind_dump(struct unwind_state *state)
>  	struct stack_info stack_info = {0};
>  	unsigned long visit_mask = 0;
>  
> -	if (dumped_before)
> +	if (IS_ENABLED(CONFIG_X86_32) || dumped_before)
>  		return;
>  
>  	dumped_before = true;
> @@ -42,7 +42,8 @@ static void unwind_dump(struct unwind_state *state)
>  			state->stack_info.type, state->stack_info.next_sp,
>  			state->stack_mask, state->graph_idx);
>  
> -	for (sp = state->orig_sp; sp; sp = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
> +	for (sp = PTR_ALIGN(state->orig_sp, sizeof(long)); sp;
> +	     sp = PTR_ALIGN(stack_info.next_sp, sizeof(long))) {
>  		if (get_stack_info(sp, state->task, &stack_info, &visit_mask))
>  			break;
>  
> @@ -84,6 +85,12 @@ static size_t regs_size(struct pt_regs *regs)
>  	return sizeof(*regs);
>  }
>  
> +#ifdef CONFIG_X86_32
> +#define KERNEL_REGS_SIZE (sizeof(struct pt_regs) - 2*sizeof(long))
> +#else
> +#define KERNEL_REGS_SIZE (sizeof(struct pt_regs))
> +#endif
> +
>  static bool in_entry_code(unsigned long ip)
>  {
>  	char *addr = (char *)ip;
> @@ -183,6 +190,7 @@ static bool is_last_task_frame(struct unwind_state *state)
>   * This determines if the frame pointer actually contains an encoded pointer to
>   * pt_regs on the stack.  See ENCODE_FRAME_POINTER.
>   */
> +#ifdef CONFIG_X86_64
>  static struct pt_regs *decode_frame_pointer(unsigned long *bp)
>  {
>  	unsigned long regs = (unsigned long)bp;
> @@ -192,6 +200,17 @@ static struct pt_regs *decode_frame_pointer(unsigned long *bp)
>  
>  	return (struct pt_regs *)(regs & ~0x1);
>  }
> +#else
> +static struct pt_regs *decode_frame_pointer(unsigned long *bp)
> +{
> +	unsigned long regs = (unsigned long)bp;
> +
> +	if (regs & 0x80000000)
> +		return NULL;
> +
> +	return (struct pt_regs *)(regs | 0x80000000);
> +}
> +#endif
>  
>  static bool update_stack_state(struct unwind_state *state,
>  			       unsigned long *next_bp)
> @@ -211,7 +230,7 @@ static bool update_stack_state(struct unwind_state *state,
>  	regs = decode_frame_pointer(next_bp);
>  	if (regs) {
>  		frame = (unsigned long *)regs;
> -		len = regs_size(regs);
> +		len = KERNEL_REGS_SIZE;
>  		state->got_irq = true;
>  	} else {
>  		frame = next_bp;
> @@ -235,6 +254,14 @@ static bool update_stack_state(struct unwind_state *state,
>  	    frame < prev_frame_end)
>  		return false;
>  
> +	/*
> +	 * On 32-bit with user mode regs, make sure the last two regs are safe
> +	 * to access:
> +	 */
> +	if (IS_ENABLED(CONFIG_X86_32) && regs && user_mode(regs) &&
> +	    !on_stack(info, frame, len + 2*sizeof(long)))
> +		return false;
> +
>  	/* Move state to the next frame: */
>  	if (regs) {
>  		state->regs = regs;
> -- 
> 2.13.6
> 

> From 62105550632bfbd2e5e2f3768a37958a6872ec1e Mon Sep 17 00:00:00 2001
> Message-Id: <62105550632bfbd2e5e2f3768a37958a6872ec1e.1507215196.git.jpoimboe@redhat.com>
> In-Reply-To: <e7840ad76515f0b5061fcdd098b57b7c01b61482.1507215196.git.jpoimboe@redhat.com>
> References: <e7840ad76515f0b5061fcdd098b57b7c01b61482.1507215196.git.jpoimboe@redhat.com>
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu, 5 Oct 2017 09:44:33 -0500
> Subject: [PATCH 2/2] lockdep fixes
> 
> ---
>  kernel/locking/lockdep.c | 48 ++++++++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 841828ba35b9..6d540bdb24b3 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1827,10 +1827,10 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
>  	       struct held_lock *next, int distance, struct stack_trace *trace,
>  	       int (*save)(struct stack_trace *trace))
>  {
> +	struct lock_list *uninitialized_var(target_entry);
>  	struct lock_list *entry;
> -	int ret;
>  	struct lock_list this;
> -	struct lock_list *uninitialized_var(target_entry);
> +	int ret;
>  
>  	/*
>  	 * Prove that the new <prev> -> <next> dependency would not
> @@ -1844,8 +1844,17 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
>  	this.class = hlock_class(next);
>  	this.parent = NULL;
>  	ret = check_noncircular(&this, hlock_class(prev), &target_entry);
> -	if (unlikely(!ret))
> +	if (unlikely(!ret)) {
> +		if (!trace->entries) {
> +			/*
> +			 * If @save fails here, the printing might trigger
> +			 * a WARN but because of the !nr_entries it should
> +			 * not do bad things.
> +			 */
> +			save(trace);
> +		}
>  		return print_circular_bug(&this, target_entry, next, prev);
> +	}
>  	else if (unlikely(ret < 0))
>  		return print_bfs_bug(ret);
>  
> @@ -1892,7 +1901,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
>  		return print_bfs_bug(ret);
>  
>  
> -	if (save && !save(trace))
> +	if (!trace->entries && !save(trace))
>  		return 0;
>  
>  	/*
> @@ -1912,20 +1921,6 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
>  	if (!ret)
>  		return 0;
>  
> -	/*
> -	 * Debugging printouts:
> -	 */
> -	if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
> -		graph_unlock();
> -		printk("\n new dependency: ");
> -		print_lock_name(hlock_class(prev));
> -		printk(KERN_CONT " => ");
> -		print_lock_name(hlock_class(next));
> -		printk(KERN_CONT "\n");
> -		dump_stack();
> -		if (!graph_lock())
> -			return 0;
> -	}
>  	return 2;
>  }
>  
> @@ -1940,8 +1935,12 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next)
>  {
>  	int depth = curr->lockdep_depth;
>  	struct held_lock *hlock;
> -	struct stack_trace trace;
> -	int (*save)(struct stack_trace *trace) = save_trace;
> +	struct stack_trace trace = {
> +		.nr_entries = 0,
> +		.max_entries = 0,
> +		.entries = NULL,
> +		.skip = 0,
> +	};
>  
>  	/*
>  	 * Debugging checks.
> @@ -1967,18 +1966,11 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next)
>  		 */
>  		if (hlock->read != 2 && hlock->check) {
>  			int ret = check_prev_add(curr, hlock, next,
> -						distance, &trace, save);
> +						distance, &trace, save_trace);
>  			if (!ret)
>  				return 0;
>  
>  			/*
> -			 * Stop saving stack_trace if save_trace() was
> -			 * called at least once:
> -			 */
> -			if (save && ret == 2)
> -				save = NULL;
> -
> -			/*
>  			 * Stop after the first non-trylock entry,
>  			 * as non-trylock entries have added their
>  			 * own direct dependencies already, so this
> -- 
> 2.13.6
>