linux-kernel - Re: [PATCH v5 01/29] asm-generic: add generic futex for !CONFIG

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: <alpine.DEB.2.11.1410252243110.5308@nanos>
Date:	Sat, 25 Oct 2014 22:50:28 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Ley Foon Tan <lftan@...era.com>
cc:	linux-arch@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	linux-doc@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	lftan.linux@...il.com, cltang@...esourcery.com,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v5 01/29] asm-generic: add generic futex for
 !CONFIG_SMP

B1;2802;0cOn Fri, 24 Oct 2014, Ley Foon Tan wrote:
> +#ifndef CONFIG_SMP
> +static inline int
> +futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)

If we add this to the generic code we want to have a proper docbook
comment describing the function.

> +{
> +	int op = (encoded_op >> 28) & 7;
> +	int cmp = (encoded_op >> 24) & 15;
> +	int oparg = (encoded_op << 8) >> 20;
> +	int cmparg = (encoded_op << 20) >> 20;
> +	int oldval, ret;
> +	u32 tmp;
> +
> +	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
> +		oparg = 1 << oparg;
> +
> +	pagefault_disable();	/* implies preempt_disable() */

Now this mindlessly copied comment does not have any real value.

1) That pagefault_disable() implies preempt_disable() is an
   implementation detail which is not guaranteed to be true forever.

2) It's not telling at all, that this code really relies on both
   pagefault AND preemption being disabled.

> +
> +static inline int
> +futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> +			      u32 oldval, u32 newval)
> +{

Docbook comment missing as well. But what's worse is, that it does not
explain what the calling convention for this function is.

As above it requires both pagefault AND preemption being disabled. The
fact that both are the same are again just an implementation detail of
todays code ....

> +	u32 val;
> +
> +	if (unlikely(get_user(val, uaddr) != 0))
> +		return -EFAULT;
> +
> +	if (val == oldval && unlikely(put_user(newval, uaddr) != 0))
> +		return -EFAULT;
> +
> +	*uval = val;
> +
> +	return 0;
> +}

Thanks,

	tglx
--
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/