| Message ID | 20160106210532.GE27290@indiana.gru.redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: > On Wed, Jan 06, 2016 at 11:11:41PM +0300, Konstantin Khlebnikov wrote: > > On Wed, Jan 6, 2016 at 10:59 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > On Wed, Jan 6, 2016 at 11:15 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > > >> Looks like this happens because ip_options_fragment() relies on > > >> correct ip options length in ip control block in skb. But in > > >> ip_finish_output_gso() control block in segments is reused by > > >> skb_gso_segment(). following ip_fragment() sees some garbage. > > >> > > >> In my case there was no ip options but length becomes non-zero and > > >> ip_options_fragment() picked some bytes from payload and decides to > > >> fill huge range with IPOPT_NOOP (1). One of that ones flipped nr_frags > > >> in skb_shared_info at the end of data =) > > >> > > > > > > Hmm, it looks like SKB_GSO_CB should be cleared after skb_gso_segment() > > > since all the gso information should be saved in shared_info after it finishes. > > > > > > Does a memset(0) on SKB_GSO_CB after skb_gso_segment() work as well? > > > > This will break present logic around ip_options_fragment() - it clears > > options from > > second and following fragments. With zeroed cb it will do nothing. > > > > ip_options_fragment() can get required information directly from ip header but > > it also resets fields in IPCB -- probably it should stay valid here > > and somebody else will use it later. [..] > I have hit this as well, this fixes it for me on an older kernel. Can you try it > on latest kernel? > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index d8a1745..f44bc91 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) > netdev_features_t features; > struct sk_buff *segs; > int ret = 0; > + struct inet_skb_parm ipcb; > > if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb)) > return ip_finish_output2(skb); > @@ -227,6 +228,10 @@ static int ip_finish_output_gso(struct sk_buff *skb) > * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly > * from host network stack. > */ > + /* We need to save IPCB here because skb_gso_segment will use > + * SKB_GSO_CB. > + */ > + ipcb = *IPCB(skb); > features = netif_skb_features(skb); > segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); > if (IS_ERR_OR_NULL(segs)) { > @@ -241,6 +246,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) > int err; > > segs->next = NULL; > + *IPCB(segs) = ipcb; > err = ip_fragment(segs, ip_finish_output2); > > if (err && ret == 0) I'm worried that this doesn't solve all cases. f.e. xfrm may also call skb_gso_segment(), and it will call into ipv4/ipv6 netfilter postrouting + ipv4 output functions... nfqnl_enqueue_packet() is also affected. -- 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
Florian Westphal <fw@strlen.de> wrote: > Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: > > On Wed, Jan 06, 2016 at 11:11:41PM +0300, Konstantin Khlebnikov wrote: [ skb_gso_segment uses skb->cb[], causes oops if ip_fragment is invoked on segmented skbs ] > > I have hit this as well, this fixes it for me on an older kernel. Can you try it > > on latest kernel? > > > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > > index d8a1745..f44bc91 100644 > > --- a/net/ipv4/ip_output.c > > +++ b/net/ipv4/ip_output.c > > @@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) > > netdev_features_t features; > > struct sk_buff *segs; > > int ret = 0; > > + struct inet_skb_parm ipcb; > > > > if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb)) > > return ip_finish_output2(skb); > > @@ -227,6 +228,10 @@ static int ip_finish_output_gso(struct sk_buff *skb) > > * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly > > * from host network stack. > > */ > > + /* We need to save IPCB here because skb_gso_segment will use > > + * SKB_GSO_CB. > > + */ > > + ipcb = *IPCB(skb); > > features = netif_skb_features(skb); > > segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); > > if (IS_ERR_OR_NULL(segs)) { > > @@ -241,6 +246,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) > > int err; > > > > segs->next = NULL; > > + *IPCB(segs) = ipcb; > > err = ip_fragment(segs, ip_finish_output2); > > > > if (err && ret == 0) > > I'm worried that this doesn't solve all cases. f.e. xfrm may also > call skb_gso_segment(), and it will call into ipv4/ipv6 netfilter > postrouting + ipv4 output functions... > > nfqnl_enqueue_packet() is also affected. ... but it seems that those three are the only affected callers of skb_gso_segment (tbf is ok since skb isn't owned by anyone, ovs does save/restore already). I think this patch is the right way, we just need similar save/restore in nfqnl_enqueue_packet and xfrm_output_gso(). The latter two can be used by either ipv4 or ipv6 so it might be preferable to just save/restore sizeof(struct skb_gso_cb); or a union of inet_skb_parm+inet6_skb_parm. Cascardo, can you cook a patch? Thanks! -- 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, Jan 7, 2016 at 2:49 AM, Florian Westphal <fw@strlen.de> wrote: > Florian Westphal <fw@strlen.de> wrote: >> Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: >> > On Wed, Jan 06, 2016 at 11:11:41PM +0300, Konstantin Khlebnikov wrote: > > [ skb_gso_segment uses skb->cb[], causes oops if ip_fragment is invoked > on segmented skbs ] > >> > I have hit this as well, this fixes it for me on an older kernel. Can you try it >> > on latest kernel? >> >> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >> > index d8a1745..f44bc91 100644 >> > --- a/net/ipv4/ip_output.c >> > +++ b/net/ipv4/ip_output.c >> > @@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) >> > netdev_features_t features; >> > struct sk_buff *segs; >> > int ret = 0; >> > + struct inet_skb_parm ipcb; >> > >> > if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb)) >> > return ip_finish_output2(skb); >> > @@ -227,6 +228,10 @@ static int ip_finish_output_gso(struct sk_buff *skb) >> > * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly >> > * from host network stack. >> > */ >> > + /* We need to save IPCB here because skb_gso_segment will use >> > + * SKB_GSO_CB. >> > + */ >> > + ipcb = *IPCB(skb); >> > features = netif_skb_features(skb); >> > segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); >> > if (IS_ERR_OR_NULL(segs)) { >> > @@ -241,6 +246,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) >> > int err; >> > >> > segs->next = NULL; >> > + *IPCB(segs) = ipcb; >> > err = ip_fragment(segs, ip_finish_output2); >> > >> > if (err && ret == 0) >> >> I'm worried that this doesn't solve all cases. f.e. xfrm may also >> call skb_gso_segment(), and it will call into ipv4/ipv6 netfilter >> postrouting + ipv4 output functions... >> >> nfqnl_enqueue_packet() is also affected. > > ... but it seems that those three are the only affected callers > of skb_gso_segment (tbf is ok since skb isn't owned by anyone, > ovs does save/restore already). > > I think this patch is the right way, we just need similar > save/restore in nfqnl_enqueue_packet and xfrm_output_gso(). Which CB could be here? at this point skb isn't owned by netlink yet. > > The latter two can be used by either ipv4 or ipv6 so it might > be preferable to just save/restore sizeof(struct skb_gso_cb); > or a union of inet_skb_parm+inet6_skb_parm. Or just shift GSO CB and add couple checks like BUILD_BUG_ON(sizeof(SKB_GSO_CB(skb)->room) < sizeof(*IPCB(skb))); -- 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, Jan 7, 2016 at 2:00 PM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Thu, Jan 7, 2016 at 2:49 AM, Florian Westphal <fw@strlen.de> wrote: >> Florian Westphal <fw@strlen.de> wrote: >>> Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: >>> > On Wed, Jan 06, 2016 at 11:11:41PM +0300, Konstantin Khlebnikov wrote: >> >> [ skb_gso_segment uses skb->cb[], causes oops if ip_fragment is invoked >> on segmented skbs ] >> >>> > I have hit this as well, this fixes it for me on an older kernel. Can you try it >>> > on latest kernel? >>> >>> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >>> > index d8a1745..f44bc91 100644 >>> > --- a/net/ipv4/ip_output.c >>> > +++ b/net/ipv4/ip_output.c >>> > @@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) >>> > netdev_features_t features; >>> > struct sk_buff *segs; >>> > int ret = 0; >>> > + struct inet_skb_parm ipcb; >>> > >>> > if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb)) >>> > return ip_finish_output2(skb); >>> > @@ -227,6 +228,10 @@ static int ip_finish_output_gso(struct sk_buff *skb) >>> > * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly >>> > * from host network stack. >>> > */ >>> > + /* We need to save IPCB here because skb_gso_segment will use >>> > + * SKB_GSO_CB. >>> > + */ >>> > + ipcb = *IPCB(skb); >>> > features = netif_skb_features(skb); >>> > segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); >>> > if (IS_ERR_OR_NULL(segs)) { >>> > @@ -241,6 +246,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) >>> > int err; >>> > >>> > segs->next = NULL; >>> > + *IPCB(segs) = ipcb; >>> > err = ip_fragment(segs, ip_finish_output2); >>> > >>> > if (err && ret == 0) >>> >>> I'm worried that this doesn't solve all cases. f.e. xfrm may also >>> call skb_gso_segment(), and it will call into ipv4/ipv6 netfilter >>> postrouting + ipv4 output functions... >>> >>> nfqnl_enqueue_packet() is also affected. >> >> ... but it seems that those three are the only affected callers >> of skb_gso_segment (tbf is ok since skb isn't owned by anyone, >> ovs does save/restore already). >> >> I think this patch is the right way, we just need similar >> save/restore in nfqnl_enqueue_packet and xfrm_output_gso(). > > Which CB could be here? at this point skb isn't owned by netlink yet. > >> >> The latter two can be used by either ipv4 or ipv6 so it might >> be preferable to just save/restore sizeof(struct skb_gso_cb); >> or a union of inet_skb_parm+inet6_skb_parm. > > Or just shift GSO CB and add couple checks like > BUILD_BUG_ON(sizeof(SKB_GSO_CB(skb)->room) < sizeof(*IPCB(skb))); Somethin like this (in attachment) Also I've found strange thing: reason of expanding skb->cb from 40 to 48 bypes in 2006 3e3850e989c5d2eb1aab6f0fd9257759f0f4cbc6 was that struct inet6_skb_parm does not fit. But it's is only 24 bytes. Does some arches add pad after each _u16 field?
On Thu, Jan 7, 2016 at 6:38 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > > Also I've found strange thing: reason of expanding skb->cb from 40 to > 48 bypes in 2006 > 3e3850e989c5d2eb1aab6f0fd9257759f0f4cbc6 was that struct inet6_skb_parm does > not fit. But it's is only 24 bytes. Does some arches add pad after > each _u16 field? "struct inet6_skb_parm" is part of struct tcp_skb_cb This is why Patrick had to increase skb->cb[] -- 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
Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Thu, Jan 7, 2016 at 2:49 AM, Florian Westphal <fw@strlen.de> wrote: > > ... but it seems that those three are the only affected callers > > of skb_gso_segment (tbf is ok since skb isn't owned by anyone, > > ovs does save/restore already). > > > > I think this patch is the right way, we just need similar > > save/restore in nfqnl_enqueue_packet and xfrm_output_gso(). > > Which CB could be here? at this point skb isn't owned by netlink yet. inet(6)_skb_parm, nfqnl_enqueue_packet is called via netfilter hooks, skb is owned by ipv4 or ipv6 stack. > > The latter two can be used by either ipv4 or ipv6 so it might > > be preferable to just save/restore sizeof(struct skb_gso_cb); > > or a union of inet_skb_parm+inet6_skb_parm. > > Or just shift GSO CB and add couple checks like > BUILD_BUG_ON(sizeof(SKB_GSO_CB(skb)->room) < sizeof(*IPCB(skb))); Right, that works too. -- 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, Jan 7, 2016 at 2:59 PM, Eric Dumazet <edumazet@google.com> wrote: > On Thu, Jan 7, 2016 at 6:38 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: >> >> Also I've found strange thing: reason of expanding skb->cb from 40 to >> 48 bypes in 2006 >> 3e3850e989c5d2eb1aab6f0fd9257759f0f4cbc6 was that struct inet6_skb_parm does >> not fit. But it's is only 24 bytes. Does some arches add pad after >> each _u16 field? > > "struct inet6_skb_parm" is part of struct tcp_skb_cb > > This is why Patrick had to increase skb->cb[] Whoa. Funny. TCP moves that chunk back and forward instead of just putting it at the first place in struct. -- 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, Jan 7, 2016 at 7:04 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Thu, Jan 7, 2016 at 2:59 PM, Eric Dumazet <edumazet@google.com> wrote: >> On Thu, Jan 7, 2016 at 6:38 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: >>> >>> Also I've found strange thing: reason of expanding skb->cb from 40 to >>> 48 bypes in 2006 >>> 3e3850e989c5d2eb1aab6f0fd9257759f0f4cbc6 was that struct inet6_skb_parm does >>> not fit. But it's is only 24 bytes. Does some arches add pad after >>> each _u16 field? >> >> "struct inet6_skb_parm" is part of struct tcp_skb_cb >> >> This is why Patrick had to increase skb->cb[] > > Whoa. Funny. TCP moves that chunk back and forward instead of just > putting it at the first place in struct. You probably want to look at git history to find out why it is done this way. TCP performance is critical for some of us, and doing such trick avoid one cache miss per skb in some critical list traversals. -- 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, Jan 7, 2016 at 3:54 PM, Eric Dumazet <edumazet@google.com> wrote: > On Thu, Jan 7, 2016 at 7:04 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: >> On Thu, Jan 7, 2016 at 2:59 PM, Eric Dumazet <edumazet@google.com> wrote: >>> On Thu, Jan 7, 2016 at 6:38 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: >>>> >>>> Also I've found strange thing: reason of expanding skb->cb from 40 to >>>> 48 bypes in 2006 >>>> 3e3850e989c5d2eb1aab6f0fd9257759f0f4cbc6 was that struct inet6_skb_parm does >>>> not fit. But it's is only 24 bytes. Does some arches add pad after >>>> each _u16 field? >>> >>> "struct inet6_skb_parm" is part of struct tcp_skb_cb >>> >>> This is why Patrick had to increase skb->cb[] >> >> Whoa. Funny. TCP moves that chunk back and forward instead of just >> putting it at the first place in struct. > > You probably want to look at git history to find out why it is done this way. > > TCP performance is critical for some of us, and doing such trick avoid > one cache miss per skb in some critical list traversals. Right. This way tcp stuff perfectly fits into leftovers of first cache line. Then probably it's better to put ipv4/ipv6 cb into second line from the beginning.
On Thu, Jan 7, 2016 at 2:35 PM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Thu, Jan 7, 2016 at 3:54 PM, Eric Dumazet <edumazet@google.com> wrote: want to look at git history to find out why it is done this way. >> >> TCP performance is critical for some of us, and doing such trick avoid >> one cache miss per skb in some critical list traversals. > > Right. This way tcp stuff perfectly fits into leftovers of first cache line. > Then probably it's better to put ipv4/ipv6 cb into second line from > the beginning. Then IP forwarding might be slower. Look, each layer (TCP , IP, ....) can organize its skb->cb[] as it wants. Nobody tries to 'make universal room' for IPCB, since only IP layer wants it. TCP could even find a way in the future to no longer hold a copy of IPCB in the input skb, if code is reorganized a bit. Note that skbs for output path in TCP do not need IPCB at all. Only when skb leaves TCP and enter IP, skb->cb[] content is scratched.
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index d8a1745..f44bc91 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) netdev_features_t features; struct sk_buff *segs; int ret = 0; + struct inet_skb_parm ipcb; if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb)) return ip_finish_output2(skb); @@ -227,6 +228,10 @@ static int ip_finish_output_gso(struct sk_buff *skb) * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly * from host network stack. */ + /* We need to save IPCB here because skb_gso_segment will use + * SKB_GSO_CB. + */ + ipcb = *IPCB(skb); features = netif_skb_features(skb); segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); if (IS_ERR_OR_NULL(segs)) { @@ -241,6 +246,7 @@ static int ip_finish_output_gso(struct sk_buff *skb) int err; segs->next = NULL; + *IPCB(segs) = ipcb; err = ip_fragment(segs, ip_finish_output2); if (err && ret == 0)