Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 30 Mar 2023 10:28:11 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To:     David Howells <dhowells@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Cc:     David Howells <dhowells@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Christoph Hellwig <hch@infradead.org>,
        Jens Axboe <axboe@kernel.dk>, Jeff Layton <jlayton@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Message-ID: <64259c7b2b327_21883920818@willemb.c.googlers.com.notmuch>
In-Reply-To: <20230329141354.516864-5-dhowells@redhat.com>
References: <20230329141354.516864-1-dhowells@redhat.com>
 <20230329141354.516864-5-dhowells@redhat.com>
Subject: RE: [RFC PATCH v2 04/48] net: Declare MSG_SPLICE_PAGES internal
 sendmsg() flag
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

David Howells wrote:
> Declare MSG_SPLICE_PAGES, an internal sendmsg() flag, that hints to a
> network protocol that it should splice pages from the source iterator
> rather than copying the data if it can.  This flag is added to a list that
> is cleared by sendmsg and recvmsg syscalls on entry.
> 
> This is intended as a replacement for the ->sendpage() op, allowing a way
> to splice in several multipage folios in one go.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> cc: "David S. Miller" <davem@davemloft.net>
> cc: Eric Dumazet <edumazet@google.com>
> cc: Jakub Kicinski <kuba@kernel.org>
> cc: Paolo Abeni <pabeni@redhat.com>
> cc: Jens Axboe <axboe@kernel.dk>
> cc: Matthew Wilcox <willy@infradead.org>
> cc: netdev@vger.kernel.org
> ---
>  include/linux/socket.h | 3 +++
>  net/socket.c           | 7 +++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index 13c3a237b9c9..c2fa0f800999 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -327,6 +327,7 @@ struct ucred {
>  					  */
>  
>  #define MSG_ZEROCOPY	0x4000000	/* Use user data in kernel path */
> +#define MSG_SPLICE_PAGES 0x8000000	/* Splice the pages from the iterator in sendmsg() */
>  #define MSG_FASTOPEN	0x20000000	/* Send data in TCP SYN */
>  #define MSG_CMSG_CLOEXEC 0x40000000	/* Set close_on_exec for file
>  					   descriptor received through
> @@ -337,6 +338,8 @@ struct ucred {
>  #define MSG_CMSG_COMPAT	0		/* We never have 32 bit fixups */
>  #endif
>  
> +/* Flags to be cleared on entry by sendmsg, recvmsg, sendmmsg and recvmmsg syscalls */
> +#define MSG_INTERNAL_FLAGS (MSG_SPLICE_PAGES)

This is fine, but there is no real need to cover both send and receive.

The sendpage internal flags only ensure that those flags cannot enter sendpage
code from any unintentional path. Indeed those "internal" flags can end up in
sendmsg, at least for UDP.

Similarly, this flag set only has to protect sendto and sendmsg. That
can simplify the patch a bit.
 
>  /* Setsockoptions(2) level. Thanks to BSD these must match IPPROTO_xxx */
>  #define SOL_IP		0
> diff --git a/net/socket.c b/net/socket.c
> index 6bae8ce7059e..dfb912bbed62 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2139,6 +2139,7 @@ int __sys_sendto(int fd, void __user *buff, size_t len, unsigned int flags,
>  		msg.msg_name = (struct sockaddr *)&address;
>  		msg.msg_namelen = addr_len;
>  	}
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  	if (sock->file->f_flags & O_NONBLOCK)
>  		flags |= MSG_DONTWAIT;
>  	msg.msg_flags = flags;
> @@ -2192,6 +2193,7 @@ int __sys_recvfrom(int fd, void __user *ubuf, size_t size, unsigned int flags,
>  	if (!sock)
>  		goto out;
>  
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  	if (sock->file->f_flags & O_NONBLOCK)
>  		flags |= MSG_DONTWAIT;
>  	err = sock_recvmsg(sock, &msg, flags);
> @@ -2579,6 +2581,7 @@ long __sys_sendmsg(int fd, struct user_msghdr __user *msg, unsigned int flags,
>  
>  	if (forbid_cmsg_compat && (flags & MSG_CMSG_COMPAT))
>  		return -EINVAL;
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  
>  	sock = sockfd_lookup_light(fd, &err, &fput_needed);
>  	if (!sock)
> @@ -2627,6 +2630,7 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
>  	entry = mmsg;
>  	compat_entry = (struct compat_mmsghdr __user *)mmsg;
>  	err = 0;
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  	flags |= MSG_BATCH;
>

No need to modify __sys_sendmmsg explicitly, as it ends up calling
__sys_sendmsg?

Also, sendpage does this flags masking in the internal sock_FUNC
helpers rather than __sys_FUNC. Might be preferable.

>  	while (datagrams < vlen) {
> @@ -2775,6 +2779,7 @@ long __sys_recvmsg_sock(struct socket *sock, struct msghdr *msg,
>  			struct user_msghdr __user *umsg,
>  			struct sockaddr __user *uaddr, unsigned int flags)
>  {
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  	return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
>  }
>  
> @@ -2787,6 +2792,7 @@ long __sys_recvmsg(int fd, struct user_msghdr __user *msg, unsigned int flags,
>  
>  	if (forbid_cmsg_compat && (flags & MSG_CMSG_COMPAT))
>  		return -EINVAL;
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  
>  	sock = sockfd_lookup_light(fd, &err, &fput_needed);
>  	if (!sock)
> @@ -2839,6 +2845,7 @@ static int do_recvmmsg(int fd, struct mmsghdr __user *mmsg,
>  			goto out_put;
>  		}
>  	}
> +	flags &= ~MSG_INTERNAL_FLAGS;
>  
>  	entry = mmsg;
>  	compat_entry = (struct compat_mmsghdr __user *)mmsg;
>