Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1304461 > unrolled thread

[PATCH] net: preserve IP control block during GSO segmentation

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: preserve IP control block during GSO segmentation Konstantin Khlebnikov <koct9i@gmail.com> - 2016-01-08 13:10 +0100
    Re: [PATCH] net: preserve IP control block during GSO segmentation Konstantin Khlebnikov <koct9i@gmail.com> - 2016-01-08 13:20 +0100
    RE: [PATCH] net: preserve IP control block during GSO segmentation David Laight <David.Laight@ACULAB.COM> - 2016-01-08 13:20 +0100
      Re: [PATCH] net: preserve IP control block during GSO segmentation Thadeu Lima de Souza Cascardo <cascardo@redhat.com> - 2016-01-08 13:30 +0100
      Re: [PATCH] net: preserve IP control block during GSO segmentation Zang MingJie <zealot0630@gmail.com> - 2016-01-11 09:20 +0100
        Re: [PATCH] net: preserve IP control block during GSO segmentation Cong Wang <xiyou.wangcong@gmail.com> - 2016-01-12 02:00 +0100
    Re: [PATCH] net: preserve IP control block during GSO segmentation Thadeu Lima de Souza Cascardo <cascardo@redhat.com> - 2016-01-08 13:20 +0100

#1304461 — [PATCH] net: preserve IP control block during GSO segmentation

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 |    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)
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))

[toc] | [next] | [standalone]

#1304462

On Fri, Jan 8, 2016 at 3:00 PM, Konstantin Khlebnikov <koct9i@gmail.com> 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 <koct9i@gmail.com>
> Link: http://lkml.kernel.org/r/CALYGNiP-0MZ-FExV2HutTvE9U-QQtkKSoE--KN=JQE5STYsjAA@mail.gmail.com

This patch is alternative for [PATCH] net: prevent corruption of skb when using
skb_gso_segment by Thadeu Lima de Souza Cascardo. This version have
no stack allocations and no changes in code. It just shifts gso cb.

[toc] | [prev] | [next] | [standalone]

#1304468

From: Of Konstantin Khlebnikov
> Sent: 08 January 2016 12:01
> 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.
> 
...
> 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))

You could set SKB_SGO_CB_OFFSET to sizeof ((skb)->cb) - sizeof (struct skb_gso_cb)
so that the end of 'cb' is always used.
(Assuming the former is a multiple of 4.)

It might be worth using an on-stack structure passed through as a separate
parameter - it doesn't look as though it has to be queued with the skb.
(Clearly a bigger change.)

	David

[toc] | [prev] | [next] | [standalone]

#1304479

On Fri, Jan 08, 2016 at 12:13:49PM +0000, David Laight wrote:
> From: Of Konstantin Khlebnikov
> > Sent: 08 January 2016 12:01
> > 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.
> > 
> ...
> > 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))
> 
> You could set SKB_SGO_CB_OFFSET to sizeof ((skb)->cb) - sizeof (struct skb_gso_cb)
> so that the end of 'cb' is always used.
> (Assuming the former is a multiple of 4.)
> 
> It might be worth using an on-stack structure passed through as a separate
> parameter - it doesn't look as though it has to be queued with the skb.
> (Clearly a bigger change.)
> 

I considered that as an option. But the bigger change and the use of the extra
stack for all users, plus the extra parameters indicated I should go the other
way.

In my opinion, at least in the IP fragmentation case, saving/restoring cb is not
such a big problem since we are in slow path already.

Cascardo.

> 	David
> 

[toc] | [prev] | [next] | [standalone]

#1305941

On 01/08/2016 08:13 PM, David Laight wrote:
> You could set SKB_SGO_CB_OFFSET to sizeof ((skb)->cb) - sizeof (struct skb_gso_cb)
> so that the end of 'cb' is always used.
> (Assuming the former is a multiple of 4.)
>
> It might be worth using an on-stack structure passed through as a separate
> parameter - it doesn't look as though it has to be queued with the skb.
> (Clearly a bigger change.)

I would definitely prefer the stack structure.

As a kernel developer, sometime I can hardly figure out which struct 
current cb is without debug it, and the worst they are not documented 
anywhere. I can hardly know the life time of the cb types.

If using a stack, things can be much easier. only an extra var to store 
the stack top:

    int cb_top;

and several macro to manage the stack:

    SKB_CB_PUSH(skb, type)
    SKB_CB_POP(skb)
    SKB_CB_TOP(skb, type)

and maybe a debug variable to store current cb type.

All current cb macro can be replaced by SKB_CB_TOP.


Although it is a big change, I think it worths, for both performance and 
maintainability

[toc] | [prev] | [next] | [standalone]

#1306875

On Sun, Jan 10, 2016 at 11:45 PM, Zang MingJie <zealot0630@gmail.com> wrote:
> On 01/08/2016 08:13 PM, David Laight wrote:
>>
>> You could set SKB_SGO_CB_OFFSET to sizeof ((skb)->cb) - sizeof (struct
>> skb_gso_cb)
>> so that the end of 'cb' is always used.
>> (Assuming the former is a multiple of 4.)
>>
>> It might be worth using an on-stack structure passed through as a separate
>> parameter - it doesn't look as though it has to be queued with the skb.
>> (Clearly a bigger change.)
>
>
> I would definitely prefer the stack structure.
>
> As a kernel developer, sometime I can hardly figure out which struct current
> cb is without debug it, and the worst they are not documented anywhere. I
> can hardly know the life time of the cb types.

NAK.

Skb control block was not designed to be used like a stack but like a union
for different layers, just that people begin to mess it up.

[toc] | [prev] | [next] | [standalone]

#1304470

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 <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 |    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))
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web