netdev - Re: [PATCH v2] af_unix: don't post cmsg for SO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe9dbb70-c345-41b2-96d6-2788e2510886@kernel.dk>
Date: Fri, 19 Dec 2025 12:55:57 -0700
From: Jens Axboe <axboe@...nel.dk>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 netdev <netdev@...r.kernel.org>
Cc: io-uring <io-uring@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
 Willem de Bruijn <willemb@...gle.com>, Kuniyuki Iwashima
 <kuniyu@...gle.com>, Julian Orth <ju.orth@...il.com>
Subject: Re: [PATCH v2] af_unix: don't post cmsg for SO_INQ unless explicitly
 asked for

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@...r.kernel.org
>> Fixes: df30285b3670 ("af_unix: Introduce SO_INQ.")
>> Reported-by: Julian Orth <ju.orth@...il.com>
>> Link: https://github.com/axboe/liburing/issues/1509
>> Signed-off-by: Jens Axboe <axboe@...nel.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;
}

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