Message ID | 50D9BAA4.7000207@linux-ipv6.org |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2012-12-25 at 23:39 +0900, YOSHIFUJI Hideaki wrote: > 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 <yoshfuji@linux-ipv6.org> > --- > net/ipv6/ndisc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c > index 6574175..5f78ac2 100644 > --- a/net/ipv6/ndisc.c > +++ b/net/ipv6/ndisc.c > @@ -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); > 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. -- 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
Eric Dumazet wrote: > On Tue, 2012-12-25 at 23:39 +0900, YOSHIFUJI Hideaki wrote: >> 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 <yoshfuji@linux-ipv6.org> >> --- : > 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 -- 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
On Thu, 2012-12-27 at 04:08 +0900, YOSHIFUJI Hideaki wrote: > 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. -- 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
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c index 6574175..5f78ac2 100644 --- a/net/ipv6/ndisc.c +++ b/net/ipv6/ndisc.c @@ -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);
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 <yoshfuji@linux-ipv6.org> --- net/ipv6/ndisc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)