On Mon, Jul 31 2023 at 20:03, Peter Zijlstra wrote:

On Mon, Jul 31, 2023 at 07:36:21PM +0200, Thomas Gleixner wrote:

quoted

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.

quoted

quoted

 	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.

quoted

quoted

+	if (flags & FLAGS_NUMA) {
+		void __user *naddr = uaddr + size/2;

size / 2;

quoted

+
+		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