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

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

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
>