[PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

So this should do the rest of the work such that when we encapsulate
into a UDP tunnel, the output path works on the UDP tunnel's socket
rather than skb->sk.

Part of this work is based upon changes done by Jiri Pirko some time
ago.

Basically the first step is to pass the socket through the nf_hook
okfn(), and then next we do the same for the UDP tunnel xmit routines.

Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[Tom Herbert]

On Sun, Apr 5, 2015 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
>
> So this should do the rest of the work such that when we encapsulate
> into a UDP tunnel, the output path works on the UDP tunnel's socket
> rather than skb->sk.
>
Doesn't this contradict "udp: Do not require sock in
udp_tunnel_xmit_skb" patch?  How is the skbuf getting set with a
garbage socket?

Thanks,
Tom

login
register
mail settings
> Part of this work is based upon changes done by Jiri Pirko some time
> ago.
>
> Basically the first step is to pass the socket through the nf_hook
> okfn(), and then next we do the same for the UDP tunnel xmit routines.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Mon, 6 Apr 2015 06:59:21 -0700

> On Sun, Apr 5, 2015 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
>>
>> So this should do the rest of the work such that when we encapsulate
>> into a UDP tunnel, the output path works on the UDP tunnel's socket
>> rather than skb->sk.
>>
> Doesn't this contradict "udp: Do not require sock in
> udp_tunnel_xmit_skb" patch?  How is the skbuf getting set with a
> garbage socket?

It's not a garbage socket, but it could be a socket for a completely
different address family than ipv4/ipv6.  The canonical example is
AF_PACKET transmitting over an encapsulating device.

Tom if you are saying that skb->sk should be reset to the tunnel
socket, that doesn't work and is completely broken.

You must retain the original skb->sk of the application so that socket
memory accounting does not break.  You can't just rewrite skb->sk
during tunnel encapsulation.

That's the whole point these changes.

If an AF_PACKET socket transmits over a UDP encapsulating tunnel device
we want skb->sk to be the AF_PACKET socket for socket memory accounting
purposes, and pass down explicitly the UDP tunnel socket through the
output path starting at the point of UDP encapsulation.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 06 Apr 2015 12:41:14 -0400 (EDT)

> Tom if you are saying that skb->sk should be reset to the tunnel
> socket, that doesn't work and is completely broken.

Thinking some more, I think what you are missing is that deeper in the
ipv4/ipv6 transmit call chain we do things like sk_mc_loop() etc. on
the socket and we cannot just do it on skb->sk.

To make that work correctly we must pass the tunnel socket down
through the ipv4/ipv6 packet output paths, via netfilter hooks
if necessary.

I am also really disappointed with the call signature of the udp
tunnel send paths.  You have to be honest with yourself and agree
that something with 11 arguments is not a well designed interface.

Now that hopefully you can see that the socket is actually required,
can possibly use that to trim the function signature down for
udp_tunnel{,6}_xmit_skb()?

Worst case, make a "struct udp_tunnel_state" just like I made a "struct
nf_hook_state" for the netfilter hooks.

Thanks.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Mon, 6 Apr 2015 10:27:31 -0700

> Seems like this is creating inconsistencies with other tunneling. We don't
> have socket for GRE, IPIP, or and there's no concept of a udp socket for
> GUE.

In those cases, we'll pass a NULL socket down and routines like sk_mc_loop()
can accommodate that.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[Tom Herbert]

On Mon, Apr 6, 2015 at 10:17 AM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 06 Apr 2015 12:41:14 -0400 (EDT)
>
>> Tom if you are saying that skb->sk should be reset to the tunnel
>> socket, that doesn't work and is completely broken.
>
> Thinking some more, I think what you are missing is that deeper in the
> ipv4/ipv6 transmit call chain we do things like sk_mc_loop() etc. on
> the socket and we cannot just do it on skb->sk.
>
> To make that work correctly we must pass the tunnel socket down
> through the ipv4/ipv6 packet output paths, via netfilter hooks
> if necessary.
>
> I am also really disappointed with the call signature of the udp
> tunnel send paths.  You have to be honest with yourself and agree
> that something with 11 arguments is not a well designed interface.
>
> Now that hopefully you can see that the socket is actually required,
> can possibly use that to trim the function signature down for
> udp_tunnel{,6}_xmit_skb()?
>
To be honest, requiring an additional socket to transmit UDP
encapsulation seems really convoluted to me, especially considering
that this is just trying trying to solve AF_PACKET in nf which seems
like a narrow use case.  Is there no way to test for AF_PACKET sockets
and take action at a lower function?  Does every type encapsulation
need its own UDP socket, or can you just have one which set from the
udp_tunnel when family of skb->sk is AF_PACKET?

Thanks,
Tom

> Worst case, make a "struct udp_tunnel_state" just like I made a "struct
> nf_hook_state" for the netfilter hooks.
>
> Thanks.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Mon, 6 Apr 2015 19:43:11 -0700

> To be honest, requiring an additional socket to transmit UDP
> encapsulation seems really convoluted to me, especially considering
> that this is just trying trying to solve AF_PACKET in nf which seems
> like a narrow use case.  Is there no way to test for AF_PACKET sockets
> and take action at a lower function?  Does every type encapsulation
> need its own UDP socket, or can you just have one which set from the
> udp_tunnel when family of skb->sk is AF_PACKET?

This has nothing to do with netfilter.

This has everything to do with being able to pass a socket down
through the complete ipv4/ipv6 output path.  That's the only
reason netfilter needed to be touched.

The ipv4/ipv6 output call paths have the NF hooks in the middle, and
the NF hooks determine what the call signature is for the rest of the
output path.  That's why it needed to be adjusted.

For ipv6 fragmentation, in particular, having the right ipv4/ipv6
socket is going to be important.

AF_PACKET is not an isolated case, just the most likely example.  It's
just as easy to trigger this problem for other protocol families too.
You can send appletalk packets over VXLAN.

I don't see what is convoluted about using the correct socket for
sending L3 protocol frames.  That's in fact how it's _supposed_ to
work.  And consistently having a proper matching socket available
makes it so that, long-term, we'll never have to deal with this issue
ever again.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[Tom Herbert]

On Mon, Apr 6, 2015 at 8:51 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <tom@herbertland.com>
> Date: Mon, 6 Apr 2015 19:43:11 -0700
>
>> To be honest, requiring an additional socket to transmit UDP
>> encapsulation seems really convoluted to me, especially considering
>> that this is just trying trying to solve AF_PACKET in nf which seems
>> like a narrow use case.  Is there no way to test for AF_PACKET sockets
>> and take action at a lower function?  Does every type encapsulation
>> need its own UDP socket, or can you just have one which set from the
>> udp_tunnel when family of skb->sk is AF_PACKET?
>
> This has nothing to do with netfilter.
>
> This has everything to do with being able to pass a socket down
> through the complete ipv4/ipv6 output path.  That's the only
> reason netfilter needed to be touched.
>
> The ipv4/ipv6 output call paths have the NF hooks in the middle, and
> the NF hooks determine what the call signature is for the rest of the
> output path.  That's why it needed to be adjusted.
>
> For ipv6 fragmentation, in particular, having the right ipv4/ipv6
> socket is going to be important.
>
> AF_PACKET is not an isolated case, just the most likely example.  It's
> just as easy to trigger this problem for other protocol families too.
> You can send appletalk packets over VXLAN.
>
> I don't see what is convoluted about using the correct socket for
> sending L3 protocol frames.  That's in fact how it's _supposed_ to
> work.  And consistently having a proper matching socket available
> makes it so that, long-term, we'll never have to deal with this issue
> ever again.

I guess this is where I'm confused. We can send just about anything
over GRE also, but have never needed a transmit socket for that. Is
UDP encapsulation so different, or is GRE equally broken also? Also,
will we need to add the socket to FOU and GUE then?

Thanks,
Tom

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[Eric Dumazet]

On Mon, 2015-04-06 at 21:45 -0700, Tom Herbert wrote:
> On Mon, Apr 6, 2015 at 8:51 PM, David Miller <davem@davemloft.net> wrote:

> > I don't see what is convoluted about using the correct socket for
> > sending L3 protocol frames.  That's in fact how it's _supposed_ to
> > work.  And consistently having a proper matching socket available
> > makes it so that, long-term, we'll never have to deal with this issue
> > ever again.
> 
> I guess this is where I'm confused. We can send just about anything
> over GRE also, but have never needed a transmit socket for that. Is
> UDP encapsulation so different, or is GRE equally broken also? Also,
> will we need to add the socket to FOU and GUE then?

GRE encap is very low level (not L3), and no socket simply sends GRE
packets as is.

For example, when GSO support was extended, it was first extended to
GRE, and only later to other tunnels with more thinking about allowing
more sophisticated encap levels.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[Wilco Baan Hofman]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On 07/04/15 07:03, Eric Dumazet wrote:
> On Mon, 2015-04-06 at 21:45 -0700, Tom Herbert wrote:
>> On Mon, Apr 6, 2015 at 8:51 PM, David Miller <davem@davemloft.net> wrote:
>>> I don't see what is convoluted about using the correct socket for
>>> sending L3 protocol frames.  That's in fact how it's _supposed_ to
>>> work.  And consistently having a proper matching socket available
>>> makes it so that, long-term, we'll never have to deal with this issue
>>> ever again.
>> I guess this is where I'm confused. We can send just about anything
>> over GRE also, but have never needed a transmit socket for that. Is
>> UDP encapsulation so different, or is GRE equally broken also? Also,
>> will we need to add the socket to FOU and GUE then?
> GRE encap is very low level (not L3), and no socket simply sends GRE
> packets as is.
FWIW, GRE encap is also broken for IPv6, it's layer 2, but only 8 bytes
can be used of the IPv6 address, because it uses sll_addr.

-- Wilco


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Mon, 6 Apr 2015 21:45:45 -0700

> I guess this is where I'm confused. We can send just about anything
> over GRE also, but have never needed a transmit socket for that. Is
> UDP encapsulation so different, or is GRE equally broken also? Also,
> will we need to add the socket to FOU and GUE then?

The situation is that if we have a socket we should use it.  More
information and context is better than less.

And making the stack more aware of what context exists happens to fix
a crash too.

Pablo has told me also that extending the output path signature in
this way helps work he is doing too.

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[Hannes Frederic Sowa]

On Tue, Apr 7, 2015, at 06:45, Tom Herbert wrote:
> On Mon, Apr 6, 2015 at 8:51 PM, David Miller <davem@davemloft.net> wrote:
> > I don't see what is convoluted about using the correct socket for
> > sending L3 protocol frames.  That's in fact how it's _supposed_ to
> > work.  And consistently having a proper matching socket available
> > makes it so that, long-term, we'll never have to deal with this issue
> > ever again.
> 
> I guess this is where I'm confused. We can send just about anything
> over GRE also, but have never needed a transmit socket for that. Is
> UDP encapsulation so different, or is GRE equally broken also? Also,
> will we need to add the socket to FOU and GUE then?

GRE, FOU, GUE in case of sk_mc_loop and the destination is a multicast
address, we imply sk_mc_loop() == true, what is not what we want. Tunnel
sockets deliberately set mc_loop to false but we cannot adhere to them,
yet. David's patchset changes that.

Also, I saw one inconsistency with sk_bound_dev_if in netfilter which
doesn't get solved by this patchset (it uses skb->sk->sk_bound_dev_if
unconditionally).

I think that having a struct-sock carrying over meta-information is a
good thing.

Thanks,
Hannes

Re: [PATCH 0/4] Prevent UDP tunnels from operating on garbage socket

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Tue, 07 Apr 2015 13:27:33 +0200

> Also, I saw one inconsistency with sk_bound_dev_if in netfilter which
> doesn't get solved by this patchset (it uses skb->sk->sk_bound_dev_if
> unconditionally).

I looked at this quickly, and it'll take a bit of (mechanical) work to
resolve.  Basically you have to get the socket down to ip_route_me_harder()
and sometimes that code path is coming from that XT targets, which don't
take the nf_hook_state as an argument or anything like that (yet).