From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932214AbcAHMLj (ORCPT ); Fri, 8 Jan 2016 07:11:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56194 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932105AbcAHMLh (ORCPT ); Fri, 8 Jan 2016 07:11:37 -0500 Date: Fri, 8 Jan 2016 10:11:31 -0200 From: Thadeu Lima de Souza Cascardo To: Konstantin Khlebnikov Cc: netdev@vger.kernel.org, "David S. Miller" , dev@openvswitch.org, Eric Dumazet , Florian Westphal , linux-kernel@vger.kernel.org, Pravin Shelar , Cong Wang Subject: Re: [PATCH] net: preserve IP control block during GSO segmentation Message-ID: <20160108121130.GL27290@indiana.gru.redhat.com> References: <145225444176.22215.2003378381077166898.stgit@zurg> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <145225444176.22215.2003378381077166898.stgit@zurg> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 08, 2016 at 03:00:41PM +0300, Konstantin Khlebnikov wrote: > 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 > 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 | 4 +--- > net/xfrm/xfrm_output.c | 2 ++ > 5 files changed, 11 insertions(+), 4 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..b1b380ee667d 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) You are missing this hunk. --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -359,7 +359,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)) >