linux-kernel - Re: [PATCH v1 11/14] futex: Implement FUTEX2

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: <875y5zn56w.ffs@tglx>
Date:   Mon, 31 Jul 2023 23:26:15 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     axboe@...nel.dk, linux-kernel@...r.kernel.org, mingo@...hat.com,
        dvhart@...radead.org, dave@...olabs.net, andrealmeid@...lia.com,
        Andrew Morton <akpm@...ux-foundation.org>, urezki@...il.com,
        hch@...radead.org, lstoakes@...il.com,
        Arnd Bergmann <arnd@...db.de>, linux-api@...r.kernel.org,
        linux-mm@...ck.org, linux-arch@...r.kernel.org,
        malteskarupke@....de
Subject: Re: [PATCH v1 11/14] futex: Implement FUTEX2_NUMA

On Mon, Jul 31 2023 at 20:03, Peter Zijlstra wrote:
> On Mon, Jul 31, 2023 at 07:36:21PM +0200, Thomas Gleixner wrote:
>> Hmm. Shouldn't that have changed with the allowance of the 1 and 2 byte
>> futexes?
>
> That patches comes after this.. :-)

Futexes are really cursed :)

> But I do have an open question here; do we want FUTEX2_NUMA futexes
> aligned at futex_size or double that? That is, what do we want the
> alignment of:
>
> struct futex_numa_32 {
> 	u32 val;
> 	u32 node;
> };
>
> to be? Having that u64 aligned will guarantee these two values end up in
> the same page, having them u32 aligned (as per this patch) allows for
> them to be split.

Same page and same cacheline.

> The current paths don't care, we don't hold locks, but perhaps it makes
> sense to be conservative.

I think it makes sense.

>> >  	address -= key->both.offset;
>> >  
>> > -	if (unlikely(!access_ok(uaddr, sizeof(u32))))
>> > +	if (flags & FLAGS_NUMA)
>> > +		size *= 2;
>> > +
>> > +	if (unlikely(!access_ok(uaddr, size)))
>> >  		return -EFAULT;
>> >  
>> >  	if (unlikely(should_fail_futex(fshared)))
>> >  		return -EFAULT;
>> >  
>> > +	key->both.node = -1;
>> 
>> Please put this into an else path.
>
> Can do, but I figured the compiler could figure it out through dead
> store elimitation or somesuch pass.

Sure, but taste disagrees and it simply makes the code more obvious.

>> > +	if (flags & FLAGS_NUMA) {
>> > +		void __user *naddr = uaddr + size/2;
>> 
>> size / 2;
>> 
>> > +
>> > +		if (futex_get_value(&node, naddr, flags))
>> > +			return -EFAULT;
>> > +
>> > +		if (node == -1) {
>> > +			node = numa_node_id();
>> > +			if (futex_put_value(node, naddr, flags))
>> > +				return -EFAULT;
>> > +		}
>> > +
>> > +		if (node >= MAX_NUMNODES || !node_possible(node))
>> > +			return -EINVAL;
>> 
>> That's clearly an else path too. No point in checking whether
>> numa_node_id() is valid.
>
> No, this also checks if the value we read from userspace is valid.
>
> Only when the value we read from userspace is -1 do we set
> numa_node_id(), otherwise we take the value as read, which then must be
> a valid value.

Right, but:

	if (node == -1) {
		node = numa_node_id();
		if (futex_put_value(node, naddr, flags))
			return -EFAULT;
	} else if (node >= MAX_NUMNODES || !node_possible(node)) {
		return -EINVAL;
        }

makes it clear that the path where @node read from user space is != -1
needs to be validated, while your version checks the result of

      node = numa_node_id();

too, which does not make sense to me. Yes, it works, but ...

Thanks,

        tglx