Re: [PATCH net-2.6.22-rc7] xfrm beet interfamily support
From: Patrick McHardy <hidden>
Date: 2007-07-16 18:47:47
Joakim Koskela wrote:
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/xfrm4_input.c b/net/ipv4/xfrm4_input.c index fa1902d..7a39f4c 100644 --- a/net/ipv4/xfrm4_input.c +++ b/net/ipv4/xfrm4_input.c@@ -108,7 +108,8 @@ int xfrm4_rcv_encap(struct sk_buff *skb, __u16 encap_type) if (x->mode->input(x, skb)) goto drop; - if (x->props.mode == XFRM_MODE_TUNNEL) { + if (x->props.mode == XFRM_MODE_TUNNEL || + x->props.mode == XFRM_MODE_BEET) { decaps = 1; break; }
I was under the impression that one of the main points of BEET is that it offers tunnel semantics but does only transport mode processing. Its necessary for inter-family tunnels, but shouldn't this be avoided for normal use?
- ph->padlen = 4 - (optlen & 4); - ph->hdrlen = optlen / 8; - ph->nexthdr = top_iph->protocol; - if (ph->padlen) - memset(ph + 1, IPOPT_NOP, ph->padlen); -
+ ph = (struct ip_beet_phdr *)skb_transport_header(skb); + ph->padlen = 4 - (optlen & 4); + ph->hdrlen = (optlen + ph->padlen + sizeof(*ph)) / 8;
Reintroduces pseudo-header length bug.
+ ph->nexthdr = iphv4->protocol; + top_iphv4->protocol = IPPROTO_BEETPH; + top_iphv4->ihl = sizeof(struct iphdr) / 4;
Where did padding go?
static int xfrm4_beet_input(struct xfrm_state *x, struct sk_buff *skb)
{- if (unlikely(iph->protocol == IPPROTO_BEETPH)) {
- struct ip_beet_phdr *ph;
-
- if (!pskb_may_pull(skb, sizeof(*ph)))
- goto out;
- ph = (struct ip_beet_phdr *)(ipip_hdr(skb) + 1);
-
- phlen = sizeof(*ph) + ph->padlen;
- optlen = ph->hdrlen * 8 + (IPV4_BEET_PHMAXLEN - phlen);
- if (optlen < 0 || optlen & 3 || optlen > 250)
- goto out;+ if (unlikely(iph->protocol == IPPROTO_BEETPH)) {
+ struct ip_beet_phdr *ph =
+ (struct ip_beet_phdr*)(iph + 1);
+
+ if (!pskb_may_pull(skb, sizeof(*ph)))
+ goto out;
+
+ phlen = ph->hdrlen * 8;
+ optlen = phlen - ph->padlen - sizeof(*ph);Reintroduces header length bug.
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c index 44ef208..8db7910 100644 --- a/net/ipv4/xfrm4_output.c +++ b/net/ipv4/xfrm4_output.c@@ -53,7 +53,8 @@ static int xfrm4_output_one(struct sk_buff *skb) goto error_nolock; } - if (x->props.mode == XFRM_MODE_TUNNEL) { + if (x->props.mode == XFRM_MODE_TUNNEL || + x->props.mode == XFRM_MODE_BEET) { err = xfrm4_tunnel_check_size(skb);
Its not a real tunnel and all packets are generated locally, why does it need to send ICMPs?
quoted hunk ↗ jump to hunk
if (err)@@ -81,10 +82,15 @@ __xfrm4_bundle_create(struct xfrm_policy *policy, struct xfrm_state **xfrm, int } } }; + union { + struct in6_addr *in6; + struct in_addr *in; + } remote, local; int i; int err; int header_len = 0; int trailer_len = 0; + unsigned short encap_family = 0; dst = dst_prev = NULL; dst_hold(&rt->u.dst);@@ -113,12 +119,24 @@ __xfrm4_bundle_create(struct xfrm_policy *policy, struct xfrm_state **xfrm, int dst1->next = dst_prev; dst_prev = dst1; - + if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) { + encap_family = xfrm[i]->props.family; + if (encap_family == AF_INET) { + remote.in = (struct in_addr *) + &xfrm[i]->id.daddr.a4; + local.in = (struct in_addr *) + &xfrm[i]->props.saddr.a4; + } else if (encap_family == AF_INET6) { + remote.in6 = (struct in6_addr *) + xfrm[i]->id.daddr.a6; + local.in6 = (struct in6_addr *) + xfrm[i]->props.saddr.a6; + }
No ifdefs here?
quoted hunk ↗ jump to hunk
+ } header_len += xfrm[i]->props.header_len; trailer_len += xfrm[i]->props.trailer_len; - if (xfrm[i]->props.mode == XFRM_MODE_TUNNEL) { - unsigned short encap_family = xfrm[i]->props.family; + if (encap_family) { switch (encap_family) { case AF_INET: fl_tunnel.fl4_dst = xfrm[i]->id.daddr.a4;@@ -198,6 +216,12 @@ __xfrm4_bundle_create(struct xfrm_policy *policy, struct xfrm_state **xfrm, int } xfrm_init_pmtu(dst); + if (encap_family == AF_INET6) { + /* The worst case */ + int delta = sizeof(struct ipv6hdr) - sizeof(struct iphdr); + u32 mtu = dst_mtu(dst); + xfrm4_update_pmtu(dst, mtu - delta);
This should be known at configuration time, why do you need to call xfrm4_update_pmtu? Just set header_len correctly.
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/xfrm4_tunnel.c b/net/ipv4/xfrm4_tunnel.c index 5685103..57c2dba 100644 --- a/net/ipv4/xfrm4_tunnel.c +++ b/net/ipv4/xfrm4_tunnel.c@@ -27,7 +27,8 @@ static int ipip_xfrm_rcv(struct xfrm_state *x, struct sk_buff *skb) static int ipip_init_state(struct xfrm_state *x) { - if (x->props.mode != XFRM_MODE_TUNNEL) + if (x->props.mode != XFRM_MODE_TUNNEL || + x->props.mode != XFRM_MODE_BEET) return -EINVAL;
Looks like a bug fix that should be seperated.
quoted hunk ↗ jump to hunk
@@ -365,6 +366,8 @@ static int esp6_init_state(struct xfrm_state *x) x->props.header_len = sizeof(struct ipv6_esp_hdr) + esp->conf.ivlen; if (x->props.mode == XFRM_MODE_TUNNEL) x->props.header_len += sizeof(struct ipv6hdr); + else if (x->props.mode == XFRM_MODE_BEET) + x->props.header_len += IPV4_BEET_PHMAXLEN;
Seems wrong, you don't do option encapsulation for IPv6. All you do is subtract this length again. What you probably should do is account for the IPv4 header *if* it is an interfamiyl tunnel.
quoted hunk ↗ jump to hunk
diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index c858537..bfb3d7b 100644 --- a/net/ipv6/xfrm6_input.c +++ b/net/ipv6/xfrm6_input.c@@ -73,7 +73,8 @@ int xfrm6_rcv_spi(struct sk_buff *skb, __be32 spi) if (x->mode->input(x, skb)) goto drop; - if (x->props.mode == XFRM_MODE_TUNNEL) { /* XXX */ + if (x->props.mode == XFRM_MODE_TUNNEL || + x->props.mode == XFRM_MODE_BEET) { /* XXX */ decaps = 1;
Same as for IPv4. I lost interest here, but the reintroduced bugs make me think that some old version was simply rediffed without even checking the output and the state initialization also seems to need a bit more work.