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:
> 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) {

>  redo:
>  	/* Lock the socket to prevent queue disordering
>  	 * while sleeps in memcpy_tomsg
> @@ -3088,10 +3092,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>  	if (msg) {
>  		scm_recv_unix(sock, msg, &scm, flags);
>  
> -		if (READ_ONCE(u->recvmsg_inq) || msg->msg_get_inq) {
> +		if (msg->msg_get_inq && (copied ?: err) >= 0) {
>  			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);
>  		}
>  	} else {
>  		scm_destroy(&scm);
> -- 
> Jens Axboe
> 
>