* [PATCHSET 0/2] Fix SO_INQ for af_unix @ 2025-12-18 14:59 Jens Axboe 2025-12-18 14:59 ` [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for Jens Axboe 2025-12-18 14:59 ` [PATCH 2/2] af_unix: only post SO_INQ cmsg for non-error case Jens Axboe 0 siblings, 2 replies; 7+ messages in thread From: Jens Axboe @ 2025-12-18 14:59 UTC (permalink / raw) To: netdev; +Cc: io-uring, kuba, kuniyu, willemb Hi, We ran into an issue with the recently added SO_INQ support for unix/stream sockets. First patch fixes the unconditional posting of cmsg for io_uring cases, which it should not do, and the second patch fixes the condition for when to post an SO_INQ cmsg in general (which is only for the non-error case). Please review and apply if you're happy with them, these patches fix a regression introduced in 6.17 and newer kernels and hence are marked for stable as well. net/unix/af_unix.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for 2025-12-18 14:59 [PATCHSET 0/2] Fix SO_INQ for af_unix Jens Axboe @ 2025-12-18 14:59 ` Jens Axboe 2025-12-18 20:35 ` Willem de Bruijn 2025-12-18 14:59 ` [PATCH 2/2] af_unix: only post SO_INQ cmsg for non-error case Jens Axboe 1 sibling, 1 reply; 7+ messages in thread From: Jens Axboe @ 2025-12-18 14:59 UTC (permalink / raw) To: netdev; +Cc: io-uring, kuba, kuniyu, willemb, Jens Axboe, stable, Julian Orth 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); -- 2.51.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for 2025-12-18 14:59 ` [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for Jens Axboe @ 2025-12-18 20:35 ` Willem de Bruijn 2025-12-18 20:44 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Willem de Bruijn @ 2025-12-18 20:35 UTC (permalink / raw) To: Jens Axboe, netdev Cc: io-uring, kuba, kuniyu, willemb, Jens Axboe, stable, Julian Orth 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for 2025-12-18 20:35 ` Willem de Bruijn @ 2025-12-18 20:44 ` Jens Axboe 2025-12-18 21:15 ` Willem de Bruijn 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2025-12-18 20:44 UTC (permalink / raw) To: Willem de Bruijn, netdev Cc: io-uring, kuba, kuniyu, willemb, stable, Julian Orth 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for 2025-12-18 20:44 ` Jens Axboe @ 2025-12-18 21:15 ` Willem de Bruijn 2025-12-18 21:18 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Willem de Bruijn @ 2025-12-18 21:15 UTC (permalink / raw) To: Jens Axboe, Willem de Bruijn, netdev Cc: io-uring, kuba, kuniyu, willemb, stable, Julian Orth 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for 2025-12-18 21:15 ` Willem de Bruijn @ 2025-12-18 21:18 ` Jens Axboe 0 siblings, 0 replies; 7+ messages in thread From: Jens Axboe @ 2025-12-18 21:18 UTC (permalink / raw) To: Willem de Bruijn, netdev Cc: io-uring, kuba, kuniyu, willemb, stable, Julian Orth 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. -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] af_unix: only post SO_INQ cmsg for non-error case 2025-12-18 14:59 [PATCHSET 0/2] Fix SO_INQ for af_unix Jens Axboe 2025-12-18 14:59 ` [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for Jens Axboe @ 2025-12-18 14:59 ` Jens Axboe 1 sibling, 0 replies; 7+ messages in thread From: Jens Axboe @ 2025-12-18 14:59 UTC (permalink / raw) To: netdev; +Cc: io-uring, kuba, kuniyu, willemb, Jens Axboe, stable, Julian Orth As is done for TCP sockets, don't post an SCM_INQ cmsg for an error case. Only post them for the non-error case, which is when unix_stream_read_generic will return >= 0. 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 110d716087b5..72dc5d5bcac8 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3091,7 +3091,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, scm_recv_unix(sock, msg, &scm, flags); do_cmsg = READ_ONCE(u->recvmsg_inq); - if (do_cmsg || msg->msg_get_inq) { + if ((do_cmsg || msg->msg_get_inq) && (copied ?: err) >= 0) { msg->msg_inq = READ_ONCE(u->inq_len); if (do_cmsg) put_cmsg(msg, SOL_SOCKET, SCM_INQ, -- 2.51.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-12-18 21:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-18 14:59 [PATCHSET 0/2] Fix SO_INQ for af_unix Jens Axboe 2025-12-18 14:59 ` [PATCH 1/2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for Jens Axboe 2025-12-18 20:35 ` Willem de Bruijn 2025-12-18 20:44 ` Jens Axboe 2025-12-18 21:15 ` Willem de Bruijn 2025-12-18 21:18 ` Jens Axboe 2025-12-18 14:59 ` [PATCH 2/2] af_unix: only post SO_INQ cmsg for non-error case Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.