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 cmsg if u->recvmsg_inq is set.
Cc: stable@vger.kernel.org Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.") Reported-by: Julian Orth ju.orth@gmail.com Link: https://github.com/axboe/liburing/issues/1509 Signed-off-by: Jens Axboe axboe@kernel.dk --- net/unix/af_unix.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 55cdebfa0da0..110d716087b5 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3086,12 +3086,16 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
mutex_unlock(&u->iolock); if (msg) { + bool do_cmsg; + scm_recv_unix(sock, msg, &scm, flags);
- if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) { + do_cmsg = READ_ONCE(u->recvmsg_inq); + if (do_cmsg || msg->msg_get_inq) { 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);
Jens Axboe wrote:
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 cmsg if u->recvmsg_inq is set.
Cc: stable@vger.kernel.org Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.") Reported-by: Julian Orth ju.orth@gmail.com Link: https://github.com/axboe/liburing/issues/1509 Signed-off-by: Jens Axboe axboe@kernel.dk
net/unix/af_unix.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 55cdebfa0da0..110d716087b5 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3086,12 +3086,16 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, mutex_unlock(&u->iolock); if (msg) {
bool do_cmsg;- scm_recv_unix(sock, msg, &scm, flags);
if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
do_cmsg = READ_ONCE(u->recvmsg_inq);if (do_cmsg || msg->msg_get_inq) { 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);
Is it intentional that msg_inq is set also if msg_get_inq is not set, but do_cmsg is?
It just seems a bit surprising behavior.
That is an entangling of two separate things. - msg_get_inq sets msg_inq, and - cmsg_flags & TCP_CMSG_INQ inserts TCP_CM_INQ cmsg
The original TCP patch also entangles them, but in another way. The cmsg is written only if msg_get_inq is requested.
- if (cmsg_flags && ret >= 0) { + if ((cmsg_flags || msg->msg_get_inq) && ret >= 0) { if (cmsg_flags & TCP_CMSG_TS) tcp_recv_timestamp(msg, sk, &tss); - if (cmsg_flags & TCP_CMSG_INQ) { - inq = tcp_inq_hint(sk); - put_cmsg(msg, SOL_TCP, TCP_CM_INQ, sizeof(inq), &inq); + if (msg->msg_get_inq) { + msg->msg_inq = tcp_inq_hint(sk); + if (cmsg_flags & TCP_CMSG_INQ) + put_cmsg(msg, SOL_TCP, TCP_CM_INQ, + sizeof(msg->msg_inq), &msg->msg_inq);
With this patch the two are still not entirely consistent.
}} else { scm_destroy(&scm); -- 2.51.0
On 12/18/25 1:35 PM, Willem de Bruijn wrote:
Jens Axboe wrote:
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 cmsg if u->recvmsg_inq is set.
Cc: stable@vger.kernel.org Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.") Reported-by: Julian Orth ju.orth@gmail.com Link: https://github.com/axboe/liburing/issues/1509 Signed-off-by: Jens Axboe axboe@kernel.dk
net/unix/af_unix.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 55cdebfa0da0..110d716087b5 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3086,12 +3086,16 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, mutex_unlock(&u->iolock); if (msg) {
bool do_cmsg;- scm_recv_unix(sock, msg, &scm, flags);
if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
do_cmsg = READ_ONCE(u->recvmsg_inq);if (do_cmsg || msg->msg_get_inq) { 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);Is it intentional that msg_inq is set also if msg_get_inq is not set, but do_cmsg is?
It doesn't really matter, what matters is the actual cmsg posting be guarded. The msg_inq should only be used for a successful return anyway, I think we're better off reading it unconditionally than having multiple branches.
Not really important, if you prefer to keep them consistent, that's fine with me too.
It just seems a bit surprising behavior.
That is an entangling of two separate things.
- msg_get_inq sets msg_inq, and
- cmsg_flags & TCP_CMSG_INQ inserts TCP_CM_INQ cmsg
The original TCP patch also entangles them, but in another way. The cmsg is written only if msg_get_inq is requested.
The cmsg is written iff TCP_CMSG_INQ is set, not if ->msg_get_inq is the only thing set. That part is important.
But yes, both need the data left.
Jens Axboe wrote:
On 12/18/25 1:35 PM, Willem de Bruijn wrote:
Jens Axboe wrote:
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 cmsg if u->recvmsg_inq is set.
Cc: stable@vger.kernel.org Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.") Reported-by: Julian Orth ju.orth@gmail.com Link: https://github.com/axboe/liburing/issues/1509 Signed-off-by: Jens Axboe axboe@kernel.dk
net/unix/af_unix.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 55cdebfa0da0..110d716087b5 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3086,12 +3086,16 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, mutex_unlock(&u->iolock); if (msg) {
bool do_cmsg;- scm_recv_unix(sock, msg, &scm, flags);
if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
do_cmsg = READ_ONCE(u->recvmsg_inq);if (do_cmsg || msg->msg_get_inq) { 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);Is it intentional that msg_inq is set also if msg_get_inq is not set, but do_cmsg is?
It doesn't really matter, what matters is the actual cmsg posting be guarded. The msg_inq should only be used for a successful return anyway, I think we're better off reading it unconditionally than having multiple branches.
Not really important, if you prefer to keep them consistent, that's fine with me too.
It just seems a bit surprising behavior.
That is an entangling of two separate things.
- msg_get_inq sets msg_inq, and
- cmsg_flags & TCP_CMSG_INQ inserts TCP_CM_INQ cmsg
The original TCP patch also entangles them, but in another way. The cmsg is written only if msg_get_inq is requested.
The cmsg is written iff TCP_CMSG_INQ is set, not if ->msg_get_inq is the only thing set. That part is important.
But yes, both need the data left.
I see, writing msg_inq if not requested is benign indeed. The inverse is not true.
Ok. I do think it would be good to have the protocols consistent. Simpler to reason about the behavior and intent long term.
-- Jens Axboe
On 12/18/25 2:15 PM, Willem de Bruijn wrote:
Jens Axboe wrote:
On 12/18/25 1:35 PM, Willem de Bruijn wrote:
Jens Axboe wrote:
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 cmsg if u->recvmsg_inq is set.
Cc: stable@vger.kernel.org Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.") Reported-by: Julian Orth ju.orth@gmail.com Link: https://github.com/axboe/liburing/issues/1509 Signed-off-by: Jens Axboe axboe@kernel.dk
net/unix/af_unix.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 55cdebfa0da0..110d716087b5 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3086,12 +3086,16 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, mutex_unlock(&u->iolock); if (msg) {
bool do_cmsg;- scm_recv_unix(sock, msg, &scm, flags);
if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
do_cmsg = READ_ONCE(u->recvmsg_inq);if (do_cmsg || msg->msg_get_inq) { 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);Is it intentional that msg_inq is set also if msg_get_inq is not set, but do_cmsg is?
It doesn't really matter, what matters is the actual cmsg posting be guarded. The msg_inq should only be used for a successful return anyway, I think we're better off reading it unconditionally than having multiple branches.
Not really important, if you prefer to keep them consistent, that's fine with me too.
It just seems a bit surprising behavior.
That is an entangling of two separate things.
- msg_get_inq sets msg_inq, and
- cmsg_flags & TCP_CMSG_INQ inserts TCP_CM_INQ cmsg
The original TCP patch also entangles them, but in another way. The cmsg is written only if msg_get_inq is requested.
The cmsg is written iff TCP_CMSG_INQ is set, not if ->msg_get_inq is the only thing set. That part is important.
But yes, both need the data left.
I see, writing msg_inq if not requested is benign indeed. The inverse is not true.
Ok. I do think it would be good to have the protocols consistent. Simpler to reason about the behavior and intent long term.
Sure, I can do that. Would you prefer patch 1 and 2 folded as well, or keep them separate? If we're mirroring the logic, seems like 1 patch would be better.
linux-stable-mirror@lists.linaro.org
-
Jens Axboe -
Willem de Bruijn