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

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

Jens Axboe wrote:
> On 12/19/25 1:08 PM, Willem de Bruijn wrote:
> > [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?
> 
> > 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.
> 
> 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.

Agreed. That's more for net-next. I'll take a look.
 
> > Reviewed-by: Willem de Bruijn <willemb@google.com>
> 
> Thanks!
> 
> -- 
> Jens Axboe