* [PATCH v2] net: preserve IP control block during GSO segmentation @ 2016-01-08 12:21 Konstantin Khlebnikov 2016-01-13 20:51 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Konstantin Khlebnikov @ 2016-01-08 12:21 UTC (permalink / raw) To: netdev, David S. Miller Cc: dev, Thadeu Lima de Souza Cascardo, Eric Dumazet, Florian Westphal, linux-kernel, Pravin Shelar, Cong Wang Skb_gso_segment() uses skb control block during segmentation. This patch adds 32-bytes room for previous control block which will be copied into all resulting segments. This patch fixes kernel crash during fragmenting forwarded packets. Fragmentation requires valid IP CB in skb for clearing ip options. Also patch removes custom save/restore in ovs code, now it's redundant. Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com> Link: http://lkml.kernel.org/r/CALYGNiP-0MZ-FExV2HutTvE9U-QQtkKSoE--KN=JQE5STYsjAA@mail.gmail.com --- include/linux/skbuff.h | 3 ++- net/core/dev.c | 5 +++++ net/ipv4/ip_output.c | 1 + net/openvswitch/datapath.c | 5 +---- net/xfrm/xfrm_output.c | 2 ++ 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 4355129fff91..9147f9f34cbe 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -3446,7 +3446,8 @@ struct skb_gso_cb { int encap_level; __u16 csum_start; }; -#define SKB_GSO_CB(skb) ((struct skb_gso_cb *)(skb)->cb) +#define SKB_SGO_CB_OFFSET 32 +#define SKB_GSO_CB(skb) ((struct skb_gso_cb *)((skb)->cb + SKB_SGO_CB_OFFSET)) static inline int skb_tnl_header_len(const struct sk_buff *inner_skb) { diff --git a/net/core/dev.c b/net/core/dev.c index ae00b894e675..7f00f2439770 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2542,6 +2542,8 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path) * * It may return NULL if the skb requires no segmentation. This is * only possible when GSO is used for verifying header integrity. + * + * Segmentation preserves SKB_SGO_CB_OFFSET bytes of previous skb cb. */ struct sk_buff *__skb_gso_segment(struct sk_buff *skb, netdev_features_t features, bool tx_path) @@ -2556,6 +2558,9 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb, return ERR_PTR(err); } + BUILD_BUG_ON(SKB_SGO_CB_OFFSET + + sizeof(*SKB_GSO_CB(skb)) > sizeof(skb->cb)); + SKB_GSO_CB(skb)->mac_offset = skb_headroom(skb); SKB_GSO_CB(skb)->encap_level = 0; diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 4233cbe47052..59ed4b89b67a 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -240,6 +240,7 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk, * from host network stack. */ features = netif_skb_features(skb); + BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET); segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); if (IS_ERR_OR_NULL(segs)) { kfree_skb(skb); diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 91a8b004dc51..deadfdab1bc3 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -336,12 +336,10 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb, unsigned short gso_type = skb_shinfo(skb)->gso_type; struct sw_flow_key later_key; struct sk_buff *segs, *nskb; - struct ovs_skb_cb ovs_cb; int err; - ovs_cb = *OVS_CB(skb); + BUILD_BUG_ON(sizeof(*OVS_CB(skb)) > SKB_SGO_CB_OFFSET); segs = __skb_gso_segment(skb, NETIF_F_SG, false); - *OVS_CB(skb) = ovs_cb; if (IS_ERR(segs)) return PTR_ERR(segs); if (segs == NULL) @@ -359,7 +357,6 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb, /* Queue all of the segments. */ skb = segs; do { - *OVS_CB(skb) = ovs_cb; if (gso_type & SKB_GSO_UDP && skb != segs) key = &later_key; diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c index cc3676eb6239..ff4a91fcab9f 100644 --- a/net/xfrm/xfrm_output.c +++ b/net/xfrm/xfrm_output.c @@ -167,6 +167,8 @@ static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb { struct sk_buff *segs; + BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET); + BUILD_BUG_ON(sizeof(*IP6CB(skb)) > SKB_SGO_CB_OFFSET); segs = skb_gso_segment(skb, 0); kfree_skb(skb); if (IS_ERR(segs)) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] net: preserve IP control block during GSO segmentation 2016-01-08 12:21 [PATCH v2] net: preserve IP control block during GSO segmentation Konstantin Khlebnikov @ 2016-01-13 20:51 ` David Miller [not found] ` <20160113.155112.119405358547782630.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: David Miller @ 2016-01-13 20:51 UTC (permalink / raw) To: koct9i-Re5JQEeQqe8AvxtiuMwx3w Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA, fw-HFFVJYpyMKqzQB+pC5nmwQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA, edumazet-hpIqsD4AKlfQT0dZR+AlfA, xiyou.wangcong-Re5JQEeQqe8AvxtiuMwx3w From: Konstantin Khlebnikov <koct9i@gmail.com> Date: Fri, 08 Jan 2016 15:21:46 +0300 > Skb_gso_segment() uses skb control block during segmentation. > This patch adds 32-bytes room for previous control block which > will be copied into all resulting segments. > > This patch fixes kernel crash during fragmenting forwarded packets. > Fragmentation requires valid IP CB in skb for clearing ip options. > Also patch removes custom save/restore in ovs code, now it's redundant. > > Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com> > Link: http://lkml.kernel.org/r/CALYGNiP-0MZ-FExV2HutTvE9U-QQtkKSoE--KN=JQE5STYsjAA@mail.gmail.com If this works I definitely prefer this approach to the other patch where the CB is copied back and forth. Any other reviewers? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20160113.155112.119405358547782630.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>]
* Re: [PATCH v2] net: preserve IP control block during GSO segmentation [not found] ` <20160113.155112.119405358547782630.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> @ 2016-01-13 23:36 ` Florian Westphal [not found] ` <20160113233628.GA10507-E0PNVn5OA6ohrxcnuTQ+TQ@public.gmane.org> 2016-01-15 19:35 ` David Miller 0 siblings, 2 replies; 5+ messages in thread From: Florian Westphal @ 2016-01-13 23:36 UTC (permalink / raw) To: David Miller Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA, fw-HFFVJYpyMKqzQB+pC5nmwQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA, edumazet-hpIqsD4AKlfQT0dZR+AlfA, xiyou.wangcong-Re5JQEeQqe8AvxtiuMwx3w, koct9i-Re5JQEeQqe8AvxtiuMwx3w David Miller <davem@davemloft.net> wrote: > From: Konstantin Khlebnikov <koct9i@gmail.com> > Date: Fri, 08 Jan 2016 15:21:46 +0300 > > > Skb_gso_segment() uses skb control block during segmentation. > > This patch adds 32-bytes room for previous control block which > > will be copied into all resulting segments. > > > > This patch fixes kernel crash during fragmenting forwarded packets. > > Fragmentation requires valid IP CB in skb for clearing ip options. > > Also patch removes custom save/restore in ovs code, now it's redundant. > > > > Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com> > > Link: http://lkml.kernel.org/r/CALYGNiP-0MZ-FExV2HutTvE9U-QQtkKSoE--KN=JQE5STYsjAA@mail.gmail.com > > If this works I definitely prefer this approach to the other patch > where the CB is copied back and forth. I quite frankly don't care and just like you to apply one or the other; use coin toss if needed :-} I would prefer to use a on-stack state since there is no need to use skb->cb (no queueing) but when I gave it a try it got out of hand rather quick :-/ Anyway Konstantins approach is safe since we only need this in ovs/ip forward + nfnetlink_queue cases and in all of these there is enough room at the cb end (for now at least). _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20160113233628.GA10507-E0PNVn5OA6ohrxcnuTQ+TQ@public.gmane.org>]
* Re: [PATCH v2] net: preserve IP control block during GSO segmentation [not found] ` <20160113233628.GA10507-E0PNVn5OA6ohrxcnuTQ+TQ@public.gmane.org> @ 2016-01-14 23:22 ` Hannes Frederic Sowa 0 siblings, 0 replies; 5+ messages in thread From: Hannes Frederic Sowa @ 2016-01-14 23:22 UTC (permalink / raw) To: Florian Westphal, David Miller Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, edumazet-hpIqsD4AKlfQT0dZR+AlfA, xiyou.wangcong-Re5JQEeQqe8AvxtiuMwx3w, koct9i-Re5JQEeQqe8AvxtiuMwx3w On 14.01.2016 00:36, Florian Westphal wrote: > David Miller <davem@davemloft.net> wrote: >> From: Konstantin Khlebnikov <koct9i@gmail.com> >> Date: Fri, 08 Jan 2016 15:21:46 +0300 >> >>> Skb_gso_segment() uses skb control block during segmentation. >>> This patch adds 32-bytes room for previous control block which >>> will be copied into all resulting segments. >>> >>> This patch fixes kernel crash during fragmenting forwarded packets. >>> Fragmentation requires valid IP CB in skb for clearing ip options. >>> Also patch removes custom save/restore in ovs code, now it's redundant. >>> >>> Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com> >>> Link: http://lkml.kernel.org/r/CALYGNiP-0MZ-FExV2HutTvE9U-QQtkKSoE--KN=JQE5STYsjAA@mail.gmail.com >> >> If this works I definitely prefer this approach to the other patch >> where the CB is copied back and forth. > > I quite frankly don't care and just like you to apply one or the other; > use coin toss if needed :-} > > I would prefer to use a on-stack state since there is no need to > use skb->cb (no queueing) but when I gave it a try it got out of hand > rather quick :-/ Be careful with the encap counter within SKB_GSO_CB when trying to put it on the stack. Bye, Hannes _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] net: preserve IP control block during GSO segmentation 2016-01-13 23:36 ` Florian Westphal [not found] ` <20160113233628.GA10507-E0PNVn5OA6ohrxcnuTQ+TQ@public.gmane.org> @ 2016-01-15 19:35 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2016-01-15 19:35 UTC (permalink / raw) To: fw Cc: koct9i, netdev, dev, cascardo, edumazet, linux-kernel, pshelar, xiyou.wangcong From: Florian Westphal <fw@strlen.de> Date: Thu, 14 Jan 2016 00:36:28 +0100 > David Miller <davem@davemloft.net> wrote: >> From: Konstantin Khlebnikov <koct9i@gmail.com> >> Date: Fri, 08 Jan 2016 15:21:46 +0300 >> >> > Skb_gso_segment() uses skb control block during segmentation. >> > This patch adds 32-bytes room for previous control block which >> > will be copied into all resulting segments. >> > >> > This patch fixes kernel crash during fragmenting forwarded packets. >> > Fragmentation requires valid IP CB in skb for clearing ip options. >> > Also patch removes custom save/restore in ovs code, now it's redundant. >> > >> > Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com> >> > Link: http://lkml.kernel.org/r/CALYGNiP-0MZ-FExV2HutTvE9U-QQtkKSoE--KN=JQE5STYsjAA@mail.gmail.com >> >> If this works I definitely prefer this approach to the other patch >> where the CB is copied back and forth. > > I quite frankly don't care and just like you to apply one or the other; > use coin toss if needed :-} So I am going to apply this patch from Konstantin and queued it up for -stable. Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-15 19:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-08 12:21 [PATCH v2] net: preserve IP control block during GSO segmentation Konstantin Khlebnikov
2016-01-13 20:51 ` David Miller
[not found] ` <20160113.155112.119405358547782630.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2016-01-13 23:36 ` Florian Westphal
[not found] ` <20160113233628.GA10507-E0PNVn5OA6ohrxcnuTQ+TQ@public.gmane.org>
2016-01-14 23:22 ` Hannes Frederic Sowa
2016-01-15 19:35 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).