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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1221058864.30429.291.camel@twins.programming.kicks-ass.net>
Date:	Wed, 10 Sep 2008 17:01:04 +0200
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	Nick Piggin <npiggin@...e.de>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch] x86: some lock annotations for user copy paths

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/