[net-next] ip, ip6: Fix splice to raw and ping sockets

Commit Message

David Howells June 14, 2023, 8:04 a.m. UTC

Splicing to SOCK_RAW sockets may set MSG_SPLICE_PAGES, but in such a case,
__ip_append_data() will call skb_splice_from_iter() to access the 'from'
data, assuming it to point to a msghdr struct with an iter, instead of
using the provided getfrag function to access it.

In the case of raw_sendmsg(), however, this is not the case and 'from' will
point to a raw_frag_vec struct and raw_getfrag() will be the frag-getting
function.  A similar issue may occur with rawv6_sendmsg().

Fix this by ignoring MSG_SPLICE_PAGES if getfrag != ip_generic_getfrag as
ip_generic_getfrag() expects "from" to be a msghdr*, but the other getfrags
don't.  Note that this will prevent MSG_SPLICE_PAGES from being effective
for udplite.

This likely affects ping sockets too.  udplite looks like it should be okay
as it expects "from" to be a msghdr.

Signed-off-by: David Howells <[e-mail]>
Reported-by: [e-mail]
Link: https://lore.kernel.org/r/[e-mail]/
Fixes: 2dc334f1a63a ("splice, net: Use sendmsg(MSG_SPLICE_PAGES) rather than ->sendpage()")
Tested-by: [e-mail]
cc: Willem de Bruijn <[e-mail]>
cc: David Ahern <[e-mail]>
cc: "David S. Miller" <[e-mail]>
cc: Eric Dumazet <[e-mail]>
cc: Jakub Kicinski <[e-mail]>
cc: Paolo Abeni <[e-mail]>
cc: Jens Axboe <[e-mail]>
cc: Matthew Wilcox <[e-mail]>
cc: [e-mail]
---
 net/ipv4/ip_output.c  |    3 ++-
 net/ipv6/ip6_output.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski June 16, 2023, 5:23 a.m. UTC | #1

On Wed, 14 Jun 2023 09:04:16 +0100 David Howells wrote:
> Splicing to SOCK_RAW sockets may set MSG_SPLICE_PAGES, but in such a case,
> __ip_append_data() will call skb_splice_from_iter() to access the 'from'
> data, assuming it to point to a msghdr struct with an iter, instead of
> using the provided getfrag function to access it.
> 
> In the case of raw_sendmsg(), however, this is not the case and 'from' will
> point to a raw_frag_vec struct and raw_getfrag() will be the frag-getting
> function.  A similar issue may occur with rawv6_sendmsg().
> 
> Fix this by ignoring MSG_SPLICE_PAGES if getfrag != ip_generic_getfrag as
> ip_generic_getfrag() expects "from" to be a msghdr*, but the other getfrags
> don't.  Note that this will prevent MSG_SPLICE_PAGES from being effective
> for udplite.
> 
> This likely affects ping sockets too.  udplite looks like it should be okay
> as it expects "from" to be a msghdr.

Willem, looks good?

Willem de Bruijn June 16, 2023, 8:34 a.m. UTC | #2

On Fri, Jun 16, 2023 at 7:23 AM Jakub Kicinski <[e-mail]> wrote:
>
> On Wed, 14 Jun 2023 09:04:16 +0100 David Howells wrote:
> > Splicing to SOCK_RAW sockets may set MSG_SPLICE_PAGES, but in such a case,
> > __ip_append_data() will call skb_splice_from_iter() to access the 'from'
> > data, assuming it to point to a msghdr struct with an iter, instead of
> > using the provided getfrag function to access it.
> >
> > In the case of raw_sendmsg(), however, this is not the case and 'from' will
> > point to a raw_frag_vec struct and raw_getfrag() will be the frag-getting
> > function.  A similar issue may occur with rawv6_sendmsg().
> >
> > Fix this by ignoring MSG_SPLICE_PAGES if getfrag != ip_generic_getfrag as
> > ip_generic_getfrag() expects "from" to be a msghdr*, but the other getfrags
> > don't.  Note that this will prevent MSG_SPLICE_PAGES from being effective
> > for udplite.
> >
> > This likely affects ping sockets too.  udplite looks like it should be okay
> > as it expects "from" to be a msghdr.
>
> Willem, looks good?

Reviewed-by: Willem de Bruijn <[e-mail]>

Disabling splicing if not ip_generic_getfrag sounds great to me.

David Howells June 16, 2023, 9:59 a.m. UTC | #3

Willem de Bruijn <[e-mail]> wrote:

> Disabling splicing if not ip_generic_getfrag sounds great to me.

I have a patch in the works that passes msghdr through getfrag that allows
this to work if we definitely want to splice into raw and icmp sockets.

David

[e-mail] June 16, 2023, 7 p.m. UTC | #4

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <[e-mail]>:

On Wed, 14 Jun 2023 09:04:16 +0100 you wrote:
> Splicing to SOCK_RAW sockets may set MSG_SPLICE_PAGES, but in such a case,
> __ip_append_data() will call skb_splice_from_iter() to access the 'from'
> data, assuming it to point to a msghdr struct with an iter, instead of
> using the provided getfrag function to access it.
> 
> In the case of raw_sendmsg(), however, this is not the case and 'from' will
> point to a raw_frag_vec struct and raw_getfrag() will be the frag-getting
> function.  A similar issue may occur with rawv6_sendmsg().
> 
> [...]

Here is the summary with links:
  - [net-next] ip, ip6: Fix splice to raw and ping sockets
    https://git.kernel.org/netdev/net-next/c/5a6f6873606e

You are awesome, thank you!

Patch

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 244fb9365d87..4b39ea99f00b 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1040,7 +1040,8 @@  static int __ip_append_data(struct sock *sk,
 	} else if ((flags & MSG_SPLICE_PAGES) && length) {
 		if (inet->hdrincl)
 			return -EPERM;
-		if (rt->dst.dev->features & NETIF_F_SG)
+		if (rt->dst.dev->features & NETIF_F_SG &&
+		    getfrag == ip_generic_getfrag)
 			/* We need an empty buffer to attach stuff to */
 			paged = true;
 		else
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index c722cb881b2d..dd845139882c 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1592,7 +1592,8 @@  static int __ip6_append_data(struct sock *sk,
 	} else if ((flags & MSG_SPLICE_PAGES) && length) {
 		if (inet_sk(sk)->hdrincl)
 			return -EPERM;
-		if (rt->dst.dev->features & NETIF_F_SG)
+		if (rt->dst.dev->features & NETIF_F_SG &&
+		    getfrag == ip_generic_getfrag)
 			/* We need an empty buffer to attach stuff to */
 			paged = true;
 		else