[PATCH net-next] net: Remove VRF change to udp_sendmsg

[PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Ahern]

Remove the VRF change in udp_sendmsg to set the source address. The VRF
driver already has access to the packet on the TX path via the dst. It
can be used to update the source address in the header.

Update function based on OVS.

Cc: Tom Herbert <tom@herbertland.com>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 drivers/net/vrf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 net/ipv4/udp.c    | 18 ----------------
 2 files changed, 58 insertions(+), 23 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index e7094fbd7568..169418254636 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -160,6 +160,58 @@ static netdev_tx_t vrf_process_v6_outbound(struct sk_buff *skb,
 	return NET_XMIT_DROP;
 }
 
+static void update_ipv4_saddr(struct sk_buff *skb, struct iphdr *iph,
+			      __be32 new_addr)
+{
+	int tlen = skb->len - skb_transport_offset(skb);
+
+	if (iph->protocol == IPPROTO_TCP) {
+		if (likely(tlen >= sizeof(struct tcphdr))) {
+			inet_proto_csum_replace4(&tcp_hdr(skb)->check, skb,
+						 iph->saddr, new_addr, 1);
+		}
+	} else if (iph->protocol == IPPROTO_UDP) {
+		if (likely(tlen >= sizeof(struct udphdr))) {
+			struct udphdr *uh = udp_hdr(skb);
+
+			if (uh->check || skb->ip_summed == CHECKSUM_PARTIAL) {
+				inet_proto_csum_replace4(&uh->check, skb,
+							 iph->saddr, new_addr,
+							 1);
+			if (!uh->check)
+				uh->check = CSUM_MANGLED_0;
+			}
+		}
+	}
+
+	csum_replace4(&iph->check, iph->saddr, new_addr);
+	skb_clear_hash(skb);
+	iph->saddr = new_addr;
+}
+
+static int vrf_set_ip_saddr(struct sk_buff *skb, struct net_device *dev)
+{
+	struct iphdr *ip4h = ip_hdr(skb);
+	struct flowi4 fl4 = {
+		.flowi4_oif = dev->ifindex,
+		.flowi4_iif = LOOPBACK_IFINDEX,
+		.flowi4_tos = RT_TOS(ip4h->tos),
+		.flowi4_flags = FLOWI_FLAG_ANYSRC | FLOWI_FLAG_VRFSRC,
+		.daddr = ip4h->daddr,
+	};
+	struct rtable *rt;
+
+	rt = __ip_route_output_key(dev_net(dev), &fl4);
+	if (IS_ERR(rt))
+		return 1;
+
+	ip_rt_put(rt);
+
+	update_ipv4_saddr(skb, ip4h, fl4.saddr);
+
+	return 0;
+}
+
 static int vrf_send_v4_prep(struct sk_buff *skb, struct flowi4 *fl4,
 			    struct net_device *vrf_dev)
 {
@@ -200,11 +252,6 @@ static netdev_tx_t vrf_process_v4_outbound(struct sk_buff *skb,
 	if (vrf_send_v4_prep(skb, &fl4, vrf_dev))
 		goto err;
 
-	if (!ip4h->saddr) {
-		ip4h->saddr = inet_select_addr(skb_dst(skb)->dev, 0,
-					       RT_SCOPE_LINK);
-	}
-
 	ret = ip_local_out(skb);
 	if (unlikely(net_xmit_eval(ret)))
 		vrf_dev->stats.tx_errors++;
@@ -298,12 +345,18 @@ static int vrf_finish_output(struct sock *sk, struct sk_buff *skb)
 static int vrf_output(struct sock *sk, struct sk_buff *skb)
 {
 	struct net_device *dev = skb_dst(skb)->dev;
+	struct iphdr *iph = ip_hdr(skb);
 
 	IP_UPD_PO_STATS(dev_net(dev), IPSTATS_MIB_OUT, skb->len);
 
 	skb->dev = dev;
 	skb->protocol = htons(ETH_P_IP);
 
+	if (!iph->saddr && vrf_set_ip_saddr(skb, dev)) {
+		vrf_tx_error(dev, skb);
+		return -EINVAL;
+	}
+
 	return NF_HOOK_COND(NFPROTO_IPV4, NF_INET_POST_ROUTING, sk, skb,
 			    NULL, dev,
 			    vrf_finish_output,
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index c0a15e7f359f..ee3ba30f1ca5 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1017,24 +1017,6 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 		fl4 = &fl4_stack;
 
-		/* unconnected socket. If output device is enslaved to a VRF
-		 * device lookup source address from VRF table. This mimics
-		 * behavior of ip_route_connect{_init}.
-		 */
-		if (netif_index_is_vrf(net, ipc.oif)) {
-			flowi4_init_output(fl4, ipc.oif, sk->sk_mark, tos,
-					   RT_SCOPE_UNIVERSE, sk->sk_protocol,
-					   (flow_flags | FLOWI_FLAG_VRFSRC),
-					   faddr, saddr, dport,
-					   inet->inet_sport);
-
-			rt = ip_route_output_flow(net, fl4, sk);
-			if (!IS_ERR(rt)) {
-				saddr = fl4->saddr;
-				ip_rt_put(rt);
-			}
-		}
-
 		flowi4_init_output(fl4, ipc.oif, sk->sk_mark, tos,
 				   RT_SCOPE_UNIVERSE, sk->sk_protocol,
 				   flow_flags,
-- 
1.9.1

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[Tom Herbert]

On Mon, Aug 31, 2015 at 9:29 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> Remove the VRF change in udp_sendmsg to set the source address. The VRF
> driver already has access to the packet on the TX path via the dst. It
> can be used to update the source address in the header.
>

I don't understand this. The previous code was about selecting a
source address for packets being sourced ed on a socket, but this new
patch seems to essentially be doing SNAT in the VRF transmit path
which  seems like a fundamentally different behavior. Is this really
your intention?

Thanks,
Tom

>
> Cc: Tom Herbert <tom@herbertland.com>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>  drivers/net/vrf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  net/ipv4/udp.c    | 18 ----------------
>  2 files changed, 58 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index e7094fbd7568..169418254636 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -160,6 +160,58 @@ static netdev_tx_t vrf_process_v6_outbound(struct sk_buff *skb,
>         return NET_XMIT_DROP;
>  }
>
> +static void update_ipv4_saddr(struct sk_buff *skb, struct iphdr *iph,
> +                             __be32 new_addr)
> +{
> +       int tlen = skb->len - skb_transport_offset(skb);
> +
> +       if (iph->protocol == IPPROTO_TCP) {
> +               if (likely(tlen >= sizeof(struct tcphdr))) {
> +                       inet_proto_csum_replace4(&tcp_hdr(skb)->check, skb,
> +                                                iph->saddr, new_addr, 1);
> +               }
> +       } else if (iph->protocol == IPPROTO_UDP) {
> +               if (likely(tlen >= sizeof(struct udphdr))) {
> +                       struct udphdr *uh = udp_hdr(skb);
> +
> +                       if (uh->check || skb->ip_summed == CHECKSUM_PARTIAL) {
> +                               inet_proto_csum_replace4(&uh->check, skb,
> +                                                        iph->saddr, new_addr,
> +                                                        1);
> +                       if (!uh->check)
> +                               uh->check = CSUM_MANGLED_0;
> +                       }
> +               }
> +       }
> +
> +       csum_replace4(&iph->check, iph->saddr, new_addr);
> +       skb_clear_hash(skb);
> +       iph->saddr = new_addr;
> +}
> +
> +static int vrf_set_ip_saddr(struct sk_buff *skb, struct net_device *dev)
> +{
> +       struct iphdr *ip4h = ip_hdr(skb);
> +       struct flowi4 fl4 = {
> +               .flowi4_oif = dev->ifindex,
> +               .flowi4_iif = LOOPBACK_IFINDEX,
> +               .flowi4_tos = RT_TOS(ip4h->tos),
> +               .flowi4_flags = FLOWI_FLAG_ANYSRC | FLOWI_FLAG_VRFSRC,
> +               .daddr = ip4h->daddr,
> +       };
> +       struct rtable *rt;
> +
> +       rt = __ip_route_output_key(dev_net(dev), &fl4);
> +       if (IS_ERR(rt))
> +               return 1;
> +
> +       ip_rt_put(rt);
> +
> +       update_ipv4_saddr(skb, ip4h, fl4.saddr);
> +
> +       return 0;
> +}
> +
>  static int vrf_send_v4_prep(struct sk_buff *skb, struct flowi4 *fl4,
>                             struct net_device *vrf_dev)
>  {
> @@ -200,11 +252,6 @@ static netdev_tx_t vrf_process_v4_outbound(struct sk_buff *skb,
>         if (vrf_send_v4_prep(skb, &fl4, vrf_dev))
>                 goto err;
>
> -       if (!ip4h->saddr) {
> -               ip4h->saddr = inet_select_addr(skb_dst(skb)->dev, 0,
> -                                              RT_SCOPE_LINK);
> -       }
> -
>         ret = ip_local_out(skb);
>         if (unlikely(net_xmit_eval(ret)))
>                 vrf_dev->stats.tx_errors++;
> @@ -298,12 +345,18 @@ static int vrf_finish_output(struct sock *sk, struct sk_buff *skb)
>  static int vrf_output(struct sock *sk, struct sk_buff *skb)
>  {
>         struct net_device *dev = skb_dst(skb)->dev;
> +       struct iphdr *iph = ip_hdr(skb);
>
>         IP_UPD_PO_STATS(dev_net(dev), IPSTATS_MIB_OUT, skb->len);
>
>         skb->dev = dev;
>         skb->protocol = htons(ETH_P_IP);
>
> +       if (!iph->saddr && vrf_set_ip_saddr(skb, dev)) {
> +               vrf_tx_error(dev, skb);
> +               return -EINVAL;
> +       }
> +
>         return NF_HOOK_COND(NFPROTO_IPV4, NF_INET_POST_ROUTING, sk, skb,
>                             NULL, dev,
>                             vrf_finish_output,
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index c0a15e7f359f..ee3ba30f1ca5 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1017,24 +1017,6 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>
>                 fl4 = &fl4_stack;
>
> -               /* unconnected socket. If output device is enslaved to a VRF
> -                * device lookup source address from VRF table. This mimics
> -                * behavior of ip_route_connect{_init}.
> -                */
> -               if (netif_index_is_vrf(net, ipc.oif)) {
> -                       flowi4_init_output(fl4, ipc.oif, sk->sk_mark, tos,
> -                                          RT_SCOPE_UNIVERSE, sk->sk_protocol,
> -                                          (flow_flags | FLOWI_FLAG_VRFSRC),
> -                                          faddr, saddr, dport,
> -                                          inet->inet_sport);
> -
> -                       rt = ip_route_output_flow(net, fl4, sk);
> -                       if (!IS_ERR(rt)) {
> -                               saddr = fl4->saddr;
> -                               ip_rt_put(rt);
> -                       }
> -               }
> -
>                 flowi4_init_output(fl4, ipc.oif, sk->sk_mark, tos,
>                                    RT_SCOPE_UNIVERSE, sk->sk_protocol,
>                                    flow_flags,
> --
> 1.9.1
>

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Ahern]

On 8/31/15 11:02 AM, Tom Herbert wrote:
> On Mon, Aug 31, 2015 at 9:29 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> Remove the VRF change in udp_sendmsg to set the source address. The VRF
>> driver already has access to the packet on the TX path via the dst. It
>> can be used to update the source address in the header.
>>
>
> I don't understand this. The previous code was about selecting a
> source address for packets being sourced ed on a socket, but this new
> patch seems to essentially be doing SNAT in the VRF transmit path
> which  seems like a fundamentally different behavior. Is this really
> your intention?
>

The original code and this new code are only controlling FIB lookups 
which in turn set the source address. Functionally both versions do the 
same thing.

David

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[Tom Herbert]

On Mon, Aug 31, 2015 at 10:05 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 8/31/15 11:02 AM, Tom Herbert wrote:
>>
>> On Mon, Aug 31, 2015 at 9:29 AM, David Ahern <dsa@cumulusnetworks.com>
>> wrote:
>>>
>>> Remove the VRF change in udp_sendmsg to set the source address. The VRF
>>> driver already has access to the packet on the TX path via the dst. It
>>> can be used to update the source address in the header.
>>>
>>
>> I don't understand this. The previous code was about selecting a
>> source address for packets being sourced ed on a socket, but this new
>> patch seems to essentially be doing SNAT in the VRF transmit path
>> which  seems like a fundamentally different behavior. Is this really
>> your intention?
>>
>
> The original code and this new code are only controlling FIB lookups which
> in turn set the source address. Functionally both versions do the same
> thing.
>
It's a major departure from current convention. The source address of
the packet should be set before doing ip_send_skb. In UDP unconnected
case ip_route_output_flow calls inet_select_addr. AKAIK there is no
provision for not setting the source address and relying on the output
device to do this in its transmit routine. I still think it would be
better to call into the VRF device from inet_select_addr using an ndo
function.

Tom

 > David
>

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Ahern]

Hi Tom:

On 8/31/15 11:22 AM, Tom Herbert wrote:
> It's a major departure from current convention. The source address of
> the packet should be set before doing ip_send_skb. In UDP unconnected
> case ip_route_output_flow calls inet_select_addr. AKAIK there is no
> provision for not setting the source address and relying on the output
> device to do this in its transmit routine. I still think it would be
> better to call into the VRF device from inet_select_addr using an ndo
> function.

I was not disregarding your suggestion and definitely appreciate the 
advice. I will look into it in time. In fact I was looking at how to 
better encapsulate FIB oif changes (e.g., with ndo) on the plane home 
yesterday, but those take time and seem more appropriate for 4.4.

My intention with this patch is to reign in the VRF footprint and by 
extension impact for 4.3 without losing functionality. During the merge 
window for 4.3 net-next will be closed and it provides a good time to 
investigate abstractions like ndo handlers.

David

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Ahern]

On 8/31/15 11:22 AM, Tom Herbert wrote:
> It's a major departure from current convention. The source address of
> the packet should be set before doing ip_send_skb. In UDP unconnected
> case ip_route_output_flow calls inet_select_addr. AKAIK there is no
> provision for not setting the source address and relying on the output
> device to do this in its transmit routine.

BTW, one other clarification the code setting the source address is NOT 
in the xmit function but the dst.output function. The VRF device uses a 
dst to direct packets to it. The output function (equivalent to 
ip_output) is what sets the source address if necessary.

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[Tom Herbert]

On Mon, Aug 31, 2015 at 10:40 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> Hi Tom:
>
> On 8/31/15 11:22 AM, Tom Herbert wrote:
>>
>> It's a major departure from current convention. The source address of
>> the packet should be set before doing ip_send_skb. In UDP unconnected
>> case ip_route_output_flow calls inet_select_addr. AKAIK there is no
>> provision for not setting the source address and relying on the output
>> device to do this in its transmit routine. I still think it would be
>> better to call into the VRF device from inet_select_addr using an ndo
>> function.
>
>
> I was not disregarding your suggestion and definitely appreciate the advice.
> I will look into it in time. In fact I was looking at how to better
> encapsulate FIB oif changes (e.g., with ndo) on the plane home yesterday,
> but those take time and seem more appropriate for 4.4.
>
> My intention with this patch is to reign in the VRF footprint and by
> extension impact for 4.3 without losing functionality. During the merge
> window for 4.3 net-next will be closed and it provides a good time to
> investigate abstractions like ndo handlers.
>
Personally, I would opt to keep the existing patches as they are until
the underlying abstraction issues can be properly addressed. Turning
this into some sort of NAT problem for expediency doesn't seem like
the right approach.

Tom

> David

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 31 Aug 2015 09:29:40 -0700

> Remove the VRF change in udp_sendmsg to set the source address. The VRF
> driver already has access to the packet on the TX path via the dst. It
> can be used to update the source address in the header.
> 
> Update function based on OVS.
> 
> Cc: Tom Herbert <tom@herbertland.com>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

This is worse.

You have the source address in the VRF driver's output routine in
fl4.saddr, just use it as-is.

You're adding even more route lookups, at least the existing code
just walks the device address less which is often cheaper than
a full-blown route lookup.

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Ahern]

On 8/31/15 1:44 PM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Mon, 31 Aug 2015 09:29:40 -0700
>
>> Remove the VRF change in udp_sendmsg to set the source address. The VRF
>> driver already has access to the packet on the TX path via the dst. It
>> can be used to update the source address in the header.
>>
>> Update function based on OVS.
>>
>> Cc: Tom Herbert <tom@herbertland.com>
>> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>
> This is worse.
>
> You have the source address in the VRF driver's output routine in
> fl4.saddr, just use it as-is.

The original saddr code in vrf_xmit was just wrong. It should not have 
been there and was a leftover from early development days.

The saddr needs to be set in the dst output function (vrf_output) as I 
did in this patch otherwise the packet hits the tcpdump tap with 
potentially no source address.

>
> You're adding even more route lookups, at least the existing code
> just walks the device address less which is often cheaper than
> a full-blown route lookup.
>

TCP (and connected sockets) do not hit this lookup in vrf_output.

If anything I should be going straight to fib_table_lookup in the VRF 
driver for this new lookup to get the source address. It knows the exact 
table that should be used and hence can avoid the rules walk + local 
table miss which happens using the ip_route_xxxxx functions as well as 
the rth lookup/create which is not needed here.

Opinions before I work on another version?

David

Re: [PATCH net-next] net: Remove VRF change to udp_sendmsg

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 31 Aug 2015 14:44:46 -0600

> If anything I should be going straight to fib_table_lookup in the VRF
> driver for this new lookup to get the source address. It knows the
> exact table that should be used and hence can avoid the rules walk +
> local table miss which happens using the ip_route_xxxxx functions as
> well as the rth lookup/create which is not needed here.

Definitely if this is the case you should do a direct fib_table_lookup().