Re: [patch] x86: some lock annotations for user copy paths

[Peter Zijlstra]

On Wed, 2008-09-10 at 16:48 +0200, Nick Piggin wrote:

> @@ -3016,3 +3016,18 @@ void print_vma_addr(char *prefix, unsign
>  	}
>  	up_read(&current->mm->mmap_sem);
>  }
> +
> +void might_fault(void)
> +{
> +	/*
> +	 * it would be nicer only to annotatea paths which are not under
> +	 * pagefault_disable, however that requires a larger audit and
> +	 * providing helpers like get_user_atomic.
> +	 */
> +	if (!in_atomic()) {
> +		might_sleep();
> +		if (current->mm)
> +			might_lock_read(&current->mm->mmap_sem);
> +	}
> +}
> +EXPORT_SYMBOL(might_fault);

>From the nitpick squad :-), I prefer the form:

void might_fault(void)
{
	if (in_atomic())
		return;

	might_sleep();

	if (!current->mm)
		might_lock_read(&current->mm->mmap_sem);
}


Due to it being one nesting level less.

> Index: linux-2.6/include/linux/kernel.h
> ===================================================================
> --- linux-2.6.orig/include/linux/kernel.h
> +++ linux-2.6/include/linux/kernel.h
> @@ -140,6 +140,15 @@ extern int _cond_resched(void);
>  		(__x < 0) ? -__x : __x;		\
>  	})
>  
> +#ifdef CONFIG_LOCKDEP
> +void might_fault(void);
> +#else
> +static inline void might_fault(void)
> +{
> +	might_sleep();
> +}
> +#endif
> +
>  extern struct atomic_notifier_head panic_notifier_list;
>  extern long (*panic_blink)(long time);
>  NORET_TYPE void panic(const char * fmt, ...)

This forgets that in_atomic() again - possibly triggering might_sleep()
where not appropriate.

I'm not sure its worth it to out-of-line the thing though (its only big
on debug builds), and CONFIG_LOCKDEP is the wrong CONFIG_* variable, I
think CONFIG_PROVE_LOCKING would be the appropriate one.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/