Skip to contentSkip to conversation

ndisc: Ensure to reserve header space for encapsulation.

PATCH In review YOSHIFUJI Hideaki 3 participants 4 messages Dec 25, 2012 – Dec 26, 2012

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

YOSHIFUJI Hideaki

author
#0 lore raw

PATCH 1/1 ndisc: Ensure to reserve header space for encapsulation.

We allocate sk_buff of MAX_HEADER + LL_RESERVED_SPACE(dev) + packet length + dev->needed_tailroom, but reserved LL_RESERVED_SPACE(dev) only. This means that space for encapsulation was placed at the end of buffer. This does not make sense.

Reserve the space correctly, like this:

head data tail end

+--------------------------------------------------------------+
+                |           |                      |          |
+--------------------------------------------------------------+
|<--MAX_HEADER-->|<-hlen---->|<---ipv6 packet------>|<--tlen-->|
                   =LL_
                    RESERVED_
                    SPACE(dev)
Signed-off-by YOSHIFUJI Hideaki <[email protected]>
---
 net/ipv6/ndisc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/ipv6/ndisc.c +2 -2
index 6574175..5f78ac2 100644
@@ -404,7 +404,7 @@ static struct sk_buff *ndisc_build_skb(struct net_device *dev,
 		return NULL;
 	}
 
-	skb_reserve(skb, hlen);
+	skb_reserve(skb, MAX_HEADER + hlen);
 	ip6_nd_hdr(sk, skb, dev, saddr, daddr, IPPROTO_ICMPV6, len);
 
 	skb->transport_header = skb->tail;

@@ -1449,7 +1449,7 @@ void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target)
 		goto release;
 	}
 
-	skb_reserve(buff, hlen);
+	skb_reserve(buff, MAX_HEADER + hlen);
 	ip6_nd_hdr(sk, buff, dev, &saddr_buf, &ipv6_hdr(skb)->saddr,
 		   IPPROTO_ICMPV6, len);
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 35 lines · YOSHIFUJI Hideaki

We allocate sk_buff of MAX_HEADER + LL_RESERVED_SPACE(dev) + packet length + dev->needed_tailroom, but reserved LL_RESERVED_SPACE(dev) only. This means that space for encapsulation was placed at the end of buffer. This does not make sense.

Reserve the space correctly, like this:

head data tail end

+--------------------------------------------------------------+
+                |           |                      |          |
+--------------------------------------------------------------+
|<--MAX_HEADER-->|<-hlen---->|<---ipv6 packet------>|<--tlen-->|
                   =LL_
                    RESERVED_
                    SPACE(dev)
Signed-off-by YOSHIFUJI Hideaki <[email protected]>
---
 net/ipv6/ndisc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/ipv6/ndisc.c +1 -1
index 6574175..5f78ac2 100644
@@ -404,7 +404,7 @@ static struct sk_buff *ndisc_build_skb(struct net_device *dev,
 		return NULL;
 	}
 
-	skb_reserve(skb, hlen);
+	skb_reserve(skb, MAX_HEADER + hlen);
 	ip6_nd_hdr(sk, skb, dev, saddr, daddr, IPPROTO_ICMPV6, len);
 
 	skb->transport_header = skb->tail;

net/ipv6/ndisc.c void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) · line 1455

Quoted 8 lines
net/ipv6/ndisc.c +1 -1
@@ -1449,7 +1449,7 @@ void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target)
 		goto release;
 	}
 
-	skb_reserve(buff, hlen);
+	skb_reserve(buff, MAX_HEADER + hlen);
 	ip6_nd_hdr(sk, buff, dev, &saddr_buf, &ipv6_hdr(skb)->saddr,
 		   IPPROTO_ICMPV6, len);

It looks like this MAX_HEADER reserve is not needed at all.

Space for encapsulation should already be in
int hlen = LL_RESERVED_SPACE(dev);

arp_create() for example doesnt add this MAX_HEADER

If extra encapsulation is needed (at head or at tail), it really should be documented.

author
#2 lore raw
Quoted 17 lines · Eric Dumazet
Quoted 16 lines · YOSHIFUJI Hideaki

We allocate sk_buff of MAX_HEADER + LL_RESERVED_SPACE(dev) + packet length + dev->needed_tailroom, but reserved LL_RESERVED_SPACE(dev) only. This means that space for encapsulation was placed at the end of buffer. This does not make sense.

Reserve the space correctly, like this:

head data tail end

+--------------------------------------------------------------+
+                |           |                      |          |
+--------------------------------------------------------------+
|<--MAX_HEADER-->|<-hlen---->|<---ipv6 packet------>|<--tlen-->|
                   =LL_
                    RESERVED_
                    SPACE(dev)
Quoted 2 lines
Quoted 2 lines
Signed-off-by YOSHIFUJI Hideaki <[email protected]>

---

:

Quoted 9 lines

It looks like this MAX_HEADER reserve is not needed at all.

Space for encapsulation should already be in
int hlen = LL_RESERVED_SPACE(dev);

arp_create() for example doesnt add this MAX_HEADER

If extra encapsulation is needed (at head or at tail), it really should be documented.

Current code does not make sense, at least.

Please refer to my previous posting: "[GIT PULL net-next 01/17]
ndisc: Fix size calculation for headers." around Dec/19.

My previous patch did remove MAX_HEADER from allocation.

--yoshfuji

#3 lore raw
Quoted 6 lines · YOSHIFUJI Hideaki

Current code does not make sense, at least.

Please refer to my previous posting: "[GIT PULL net-next 01/17]
ndisc: Fix size calculation for headers." around Dec/19.

My previous patch did remove MAX_HEADER from allocation.

Yes, but the changelog was misleading, and David misunderstood it.

If you really explain why its safe to remove MAX_HEADER, I think your patch would make sense.