Re: [PATCH v2] af_unix: don't post cmsg for SO_INQ unless explicitly asked for

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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

Jens Axboe wrote:
> On 12/19/25 12:02 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 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: 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>
> >>
> >> ---
> >>
> >> V2:
> >> - Unify logic with tcp
> >> - Squash the two patches into one
> >>
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index 55cdebfa0da0..a7ca74653d94 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -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;
> >> @@ -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) {
> 
> 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;
> }

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

Reviewed-by: Willem de Bruijn <willemb@google.com>

> 
> 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.
> 
> -- 
> Jens Axboe