| Message ID | 20170919003904.5124-13-tom@quantonium.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | gtp: Additional feature support | expand |
From: Tom Herbert <tom@quantonium.net> Date: Mon, 18 Sep 2017 17:39:02 -0700 > Add configuration to control use of zero checksums on transmit for both > IPv4 and IPv6, and control over accepting zero IPv6 checksums on > receive. > > Signed-off-by: Tom Herbert <tom@quantonium.net> I thought we were trying to move away from this special case of allowing zero UDP checksums with tunnels, especially for ipv6.
On Mon, Sep 18, 2017 at 9:24 PM, David Miller <davem@davemloft.net> wrote: > From: Tom Herbert <tom@quantonium.net> > Date: Mon, 18 Sep 2017 17:39:02 -0700 > >> Add configuration to control use of zero checksums on transmit for both >> IPv4 and IPv6, and control over accepting zero IPv6 checksums on >> receive. >> >> Signed-off-by: Tom Herbert <tom@quantonium.net> > > I thought we were trying to move away from this special case of allowing > zero UDP checksums with tunnels, especially for ipv6. I don't have a strong preference either way. I like consistency with VXLAN and foo/UDP, but I guess it's not required. Interestingly, since GTP only carries IP, IPv6 zero checksums are actually safer here than VXLAN or GRE/UDP. Tom
Hi Tom, On Wed, Sep 20, 2017 at 11:09:29AM -0700, Tom Herbert wrote: > On Mon, Sep 18, 2017 at 9:24 PM, David Miller <davem@davemloft.net> wrote: > > From: Tom Herbert <tom@quantonium.net> > >> Add configuration to control use of zero checksums on transmit for both > >> IPv4 and IPv6, and control over accepting zero IPv6 checksums on > >> receive. > > > > I thought we were trying to move away from this special case of allowing > > zero UDP checksums with tunnels, especially for ipv6. > > I don't have a strong preference either way. I like consistency with > VXLAN and foo/UDP, but I guess it's not required. Interestingly, since > GTP only carries IP, IPv6 zero checksums are actually safer here than > VXLAN or GRE/UDP. Just for the record: I don't care either way and I defer to the kernel networking developers to decide if they want to have zero UDP checksum in GTP or not. The 3GPP specs don't say anything about UDP checksums. So there's no requirement to use them, and hence operation without UDP checksums should be compliant. Cisco GTP implementation has udp checksumming configurable, so other implementations also seem to provide both ways. In general, I would argue one wants UDP checksumming of GTP in all setups, as while the inner IP packet might be protected, the GTP header itself is not, and that's what contains important data suhc as the TEID (Tunnel Endpoint ID). But that's of course just my personal opinion, and I'm not saying we should prevent people from using lower protection if that's what they want.
On Wed, Sep 20, 2017 at 6:55 PM, Harald Welte <laforge@gnumonks.org> wrote: > Hi Tom, > > On Wed, Sep 20, 2017 at 11:09:29AM -0700, Tom Herbert wrote: >> On Mon, Sep 18, 2017 at 9:24 PM, David Miller <davem@davemloft.net> wrote: >> > From: Tom Herbert <tom@quantonium.net> >> >> Add configuration to control use of zero checksums on transmit for both >> >> IPv4 and IPv6, and control over accepting zero IPv6 checksums on >> >> receive. >> > >> > I thought we were trying to move away from this special case of allowing >> > zero UDP checksums with tunnels, especially for ipv6. >> >> I don't have a strong preference either way. I like consistency with >> VXLAN and foo/UDP, but I guess it's not required. Interestingly, since >> GTP only carries IP, IPv6 zero checksums are actually safer here than >> VXLAN or GRE/UDP. > > Just for the record: I don't care either way and I defer to the kernel > networking developers to decide if they want to have zero UDP checksum > in GTP or not. > > The 3GPP specs don't say anything about UDP checksums. So there's no > requirement to use them, and hence operation without UDP checksums > should be compliant. Cisco GTP implementation has udp checksumming > configurable, so other implementations also seem to provide both ways. > > In general, I would argue one wants UDP checksumming of GTP in all > setups, as while the inner IP packet might be protected, the GTP header > itself is not, and that's what contains important data suhc as the TEID > (Tunnel Endpoint ID). But that's of course just my personal opinion, > and I'm not saying we should prevent people from using lower protection > if that's what they want. > The tradeoffs and requirements of zero UDP6 checksums are discussed at length in RFC6935 and RFC6936. Given other implementations make it configurable it should also be here. Tom > -- > - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ > ============================================================================ > "Privacy in residential applications is a desirable marketing option." > (ETSI EN 300 175-7 Ch. A6)
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 393f63cb2576..b53946f8b10b 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -75,6 +75,13 @@ struct pdp_ctx { struct rcu_head rcu_head; struct dst_cache dst_cache; + + unsigned int cfg_flags; + +#define GTP_F_UDP_ZERO_CSUM_TX 0x1 +#define GTP_F_UDP_ZERO_CSUM6_TX 0x2 +#define GTP_F_UDP_ZERO_CSUM6_RX 0x4 + }; /* One instance of the GTP device. */ @@ -536,6 +543,7 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev, struct gtp_dev *gtp = netdev_priv(dev); bool xnet = !net_eq(gtp->net, dev_net(gtp->dev)); struct sock *sk = pctx->sk; + bool udp_csum; int err = 0; /* Ensure there is sufficient headroom. */ @@ -563,11 +571,12 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev, skb_dst_drop(skb); gtp_push_header(skb, pctx); + udp_csum = !(pctx->cfg_flags & GTP_F_UDP_ZERO_CSUM_TX); udp_tunnel_xmit_skb(rt, sk, skb, saddr, pctx->peer_addr_ip4.s_addr, 0, ip4_dst_hoplimit(&rt->dst), 0, pctx->gtp_port, pctx->gtp_port, - xnet, false); + xnet, !udp_csum); netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n", &saddr, &pctx->peer_addr_ip4.s_addr); @@ -591,11 +600,12 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev, skb_dst_drop(skb); gtp_push_header(skb, pctx); + udp_csum = !(pctx->cfg_flags & GTP_F_UDP_ZERO_CSUM6_TX); udp_tunnel6_xmit_skb(dst, sk, skb, dev, &saddr, &pctx->peer_addr_ip6, 0, ip6_dst_hoplimit(dst), 0, pctx->gtp_port, pctx->gtp_port, - true); + !udp_csum); netdev_dbg(dev, "gtp -> IP src: %pI6 dst: %pI6\n", &saddr, &pctx->peer_addr_ip6); @@ -728,6 +738,7 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, { unsigned int role = GTP_ROLE_GGSN; bool have_fd, have_ports; + unsigned int flags = 0; bool is_ipv6 = false; struct gtp_dev *gtp; struct gtp_net *gn; @@ -747,6 +758,21 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, return -EINVAL; } + if (data[IFLA_GTP_UDP_CSUM]) { + if (!nla_get_u8(data[IFLA_GTP_UDP_CSUM])) + flags |= GTP_F_UDP_ZERO_CSUM_TX; + } + + if (data[IFLA_GTP_UDP_ZERO_CSUM6_TX]) { + if (nla_get_u8(data[IFLA_GTP_UDP_ZERO_CSUM6_TX])) + flags |= GTP_F_UDP_ZERO_CSUM6_TX; + } + + if (data[IFLA_GTP_UDP_ZERO_CSUM6_RX]) { + if (nla_get_u8(data[IFLA_GTP_UDP_ZERO_CSUM6_RX])) + flags |= GTP_F_UDP_ZERO_CSUM6_RX; + } + if (data[IFLA_GTP_AF]) { u16 af = nla_get_u16(data[IFLA_GTP_AF]); @@ -819,6 +845,9 @@ static const struct nla_policy gtp_policy[IFLA_GTP_MAX + 1] = { [IFLA_GTP_ROLE] = { .type = NLA_U32 }, [IFLA_GTP_PORT0] = { .type = NLA_U16 }, [IFLA_GTP_PORT1] = { .type = NLA_U16 }, + [IFLA_GTP_UDP_CSUM] = { .type = NLA_U8 }, + [IFLA_GTP_UDP_ZERO_CSUM6_TX] = { .type = NLA_U8 }, + [IFLA_GTP_UDP_ZERO_CSUM6_RX] = { .type = NLA_U8 }, }; static int gtp_validate(struct nlattr *tb[], struct nlattr *data[], @@ -990,6 +1019,8 @@ static struct socket *gtp_create_sock(struct net *net, bool ipv6, if (ipv6) { udp_conf.family = AF_INET6; + udp_conf.use_udp6_rx_checksums = + !(flags & GTP_F_UDP_ZERO_CSUM6_RX); udp_conf.ipv6_v6only = 1; } else { udp_conf.family = AF_INET; diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 81c26864abeb..14a32d745e24 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -555,6 +555,10 @@ enum { IFLA_GTP_AF, IFLA_GTP_PORT0, IFLA_GTP_PORT1, + IFLA_GTP_UDP_CSUM, + IFLA_GTP_UDP_ZERO_CSUM6_TX, + IFLA_GTP_UDP_ZERO_CSUM6_RX, + __IFLA_GTP_MAX, }; #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
Add configuration to control use of zero checksums on transmit for both IPv4 and IPv6, and control over accepting zero IPv6 checksums on receive. Signed-off-by: Tom Herbert <tom@quantonium.net> --- drivers/net/gtp.c | 35 +++++++++++++++++++++++++++++++++-- include/uapi/linux/if_link.h | 4 ++++ 2 files changed, 37 insertions(+), 2 deletions(-)