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

[Jens Axboe <axboe@kernel.dk>]

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