af_unix: don't post cmsg for SO_INQ unless explicitly asked for

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c +4 -3

 redo:
 	/* Lock the socket to prevent queue disordering
 	 * while sleeps in memcpy_tomsg

@@ -3088,10 +3092,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	if (msg) {
 		scm_recv_unix(sock, msg, &scm, flags);
 
-		if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
+		if (msg->msg_get_inq && (copied ?: err) >= 0) {
 			msg->msg_inq = READ_ONCE(u->inq_len);
-			put_cmsg(msg, SOL_SOCKET, SCM_INQ,
-				 sizeof(msg->msg_inq), &msg->msg_inq);
+			if (do_cmsg)
+				put_cmsg(msg, SOL_SOCKET, SCM_INQ,
+					 sizeof(msg->msg_inq), &msg->msg_inq);
 		}
 	} else {
 		scm_destroy(&scm);

Signature · 1 line

Jens Axboe

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

Quoted 55 lines · Willem de Bruijn

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Quoted 4 lines

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

I think you need to look closer, because this is actually what the tcp path does:

if (tp->recvmsg_inq) {
	[...]
	msg->msg_get_inq = 1;
}

to avoid needing to check two things at the bottom. Which is actually why I did this for streams too, as the whole point was to unify the two and make them look the same.

Like I said, I'm happy to give you guys what you want, but you can't both ask for consistency and then want it different too. I just want the bug fixed and out of my hair and into a stable release, as it's causing regressions.

Let me know, and I'll send out a v3 if needed. But then let's please have that be it and move on.

Signature · 1 line

Jens Axboe

[PATCH net v2] assuming this is intended to go through the net tree.

Quoted 64 lines · Jens Axboe

Quoted 55 lines · Willem de Bruijn

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Quoted 4 lines

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

I think you need to look closer, because this is actually what the tcp path does:

Quoted 4 lines

if (tp->recvmsg_inq) {
	[...]
	msg->msg_get_inq = 1;
}

I indeed missed that TCP does the same. Ack. Indeed consistency was what I asked for.

Reviewed-by Willem de Bruijn <[email protected]>

Quoted 1 line · Willem de Bruijn

[PATCH net v2] assuming this is intended to go through the net tree.

Assuming this will hit -rc3 then, as netdev PRs usually go out on thursdays?

Quoted 70 lines

Quoted 68 lines · Jens Axboe

Quoted 55 lines · Willem de Bruijn

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Quoted 4 lines

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

I think you need to look closer, because this is actually what the tcp path does:

if (tp->recvmsg_inq) {
	[...]
	msg->msg_get_inq = 1;
}

Quoted 2 lines

I indeed missed that TCP does the same. Ack. Indeed consistency was what I asked for.

FWIW, I don't disagree with you, but sorting that out should then be a followup patch that would then touch both tcp and streams.

Quoted 1 line

Reviewed-by Willem de Bruijn <[email protected]>

Thanks!

Signature · 1 line

Jens Axboe

[PATCH net v2] assuming this is intended to go through the net tree.

Quoted 64 lines · Jens Axboe

Quoted 55 lines · Willem de Bruijn

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Quoted 4 lines

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

I think you need to look closer, because this is actually what the tcp path does:

Quoted 4 lines

if (tp->recvmsg_inq) {
	[...]
	msg->msg_get_inq = 1;
}

I indeed missed that TCP does the same. Ack. Indeed consistency was what I asked for.

Reviewed-by Willem de Bruijn <[email protected]>

Quoted 72 lines · Willem de Bruijn

[PATCH net v2] assuming this is intended to go through the net tree.

Quoted 68 lines · Jens Axboe

Quoted 55 lines · Willem de Bruijn

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Quoted 4 lines

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

I think you need to look closer, because this is actually what the tcp path does:

if (tp->recvmsg_inq) {
	[...]
	msg->msg_get_inq = 1;
}

Quoted 3 lines

I indeed missed that TCP does the same. Ack. Indeed consistency was what I asked for.

Reviewed-by Willem de Bruijn <[email protected]>

Can someone get this applied, please?

Quoted 73 lines · Jens Axboe

Quoted 72 lines · Willem de Bruijn

[PATCH net v2] assuming this is intended to go through the net tree.

Quoted 68 lines · Jens Axboe

Quoted 55 lines · Willem de Bruijn

Quoted 40 lines · Jens Axboe

A previous commit added SO_INQ support for AF_UNIX (SOCK_STREAM), but it posts a SCM_INQ cmsg even if just msg->msg_get_inq is set. This is incorrect, as ->msg_get_inq is just the caller asking for the remainder to be passed back in msg->msg_inq, it has nothing to do with cmsg. The original commit states that this is done to make sockets io_uring-friendly", but it's actually incorrect as io_uring doesn't use cmsg headers internally at all, and it's actively wrong as this means that cmsg's are always posted if someone does recvmsg via io_uring.

Fix that up by only posting a cmsg if u->recvmsg_inq is set.

Additionally, mirror how TCP handles inquiry handling in that it should only be done for a successful return. This makes the logic for the two identical.

Cc [email protected]

Fixes df30285b3670 ("af_unix: Introduce SO_INQ.")

Reported-by Julian Orth <[email protected]>

Link https://github.com/axboe/liburing/issues/1509

Signed-off-by Jens Axboe <[email protected]>

---

V2:
- Unify logic with tcp
- Squash the two patches into one

net/unix/af_unix.c +1 -0

index 55cdebfa0da0..a7ca74653d94 100644
@@ -2904,6 +2904,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	unsigned int last_len;
 	struct unix_sock *u;
 	int copied = 0;
+	bool do_cmsg;
 	int err = 0;
 	long timeo;
 	int target;

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2935

Quoted 6 lines

net/unix/af_unix.c +3 -0

@@ -2929,6 +2930,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 
 	u = unix_sk(sk);
 
+	do_cmsg = READ_ONCE(u->recvmsg_inq);
+	if (do_cmsg)
+		msg->msg_get_inq = 1;

I would avoid overwriting user written fields if it's easy to do so.

In this case it probably is harmless. But we've learned the hard way that applications can even get confused by recvmsg setting msg_flags. I've seen multiple reports of applications failing to scrub that field inbetween calls.

Quoted 4 lines

Also just more similar to tcp:

       do_cmsg = READ_ONCE(u->recvmsg_inq);
       if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) {

I think you need to look closer, because this is actually what the tcp path does:

if (tp->recvmsg_inq) {
	[...]
	msg->msg_get_inq = 1;
}

Quoted 5 lines

Quoted 3 lines

I indeed missed that TCP does the same. Ack. Indeed consistency was what I asked for.

Reviewed-by Willem de Bruijn <[email protected]>

Can someone get this applied, please?

For a few more days it's just me. That means a significantly longer than usual latency, but I'm almost there.