[PATCH] datapath: 16bit inner_network_header field in struct ovs_gso_cb

[PATCH] datapath: 16bit inner_network_header field in struct ovs_gso_cb

[Simon Horman]

The motivation for this is to create a 16bit hole in struct ovs_gso_cb
which may be used for the inner_protocol field which is needed
for the proposed implementation of compatibility for MPLS GSO segmentation.

This should be safe as inner_network_header is now an offset to
the inner_mac_header rather than skb->head.

As pointed out by Thomas Graf simply making both inner offsets 16bis is not
safe as there have been cases of overflow with "with collapsed TCP frames
on IB when the headroom grew beyond 64K. See commit 50bceae9bd ``tcp:
Reallocate headroom if it would overflow csum_start'' for additional
details."

This patch is based on suggestions by Thomas Graf and Jesse Gross.

Cc: Thomas Graf <tgraf@suug.ch>
Cc: Jesse Gross <jesse@nicira.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 datapath/linux/compat/gso.h | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
index b83a4c3..f459c18 100644
--- a/datapath/linux/compat/gso.h
+++ b/datapath/linux/compat/gso.h
@@ -11,8 +11,10 @@
 
 struct ovs_gso_cb {
 	struct ovs_skb_cb dp_cb;
-	sk_buff_data_t	inner_network_header;
-	sk_buff_data_t	inner_mac_header;
+	u16		inner_network_header;	/* Offset from
+						 * inner_mac_header */
+	/* 16bit hole */
+	sk_buff_data_t	inner_mac_header;	/* Offset from skb->head */
 	void (*fix_segment)(struct sk_buff *);
 };
 #define OVS_GSO_CB(skb) ((struct ovs_gso_cb *)(skb)->cb)
@@ -20,18 +22,19 @@ struct ovs_gso_cb {
 #define skb_inner_network_header rpl_skb_inner_network_header
 
 #ifdef NET_SKBUFF_DATA_USES_OFFSET
-#define skb_inner_network_header rpl_skb_inner_network_header
-static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
-{
-	return skb->head + OVS_GSO_CB(skb)->inner_network_header;
-}
-
 #define skb_inner_mac_header rpl_skb_inner_mac_header
 static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
 {
 	return skb->head + OVS_GSO_CB(skb)->inner_mac_header;
 }
 
+#define skb_inner_network_header rpl_skb_inner_network_header
+static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
+{
+	return skb_inner_mac_header(skb) +
+		OVS_GSO_CB(skb)->inner_network_header;
+}
+
 #else
 
 #define skb_inner_network_header rpl_skb_inner_network_header
@@ -64,7 +67,8 @@ static inline int skb_inner_mac_offset(const struct sk_buff *skb)
 static inline void skb_reset_inner_headers(struct sk_buff *skb)
 {
 	BUILD_BUG_ON(sizeof(struct ovs_gso_cb) > FIELD_SIZEOF(struct sk_buff, cb));
-	OVS_GSO_CB(skb)->inner_network_header = skb->network_header;
+	OVS_GSO_CB(skb)->inner_network_header = skb->network_header -
+		skb->mac_header;
 	OVS_GSO_CB(skb)->inner_mac_header = skb->mac_header;
 
 	OVS_GSO_CB(skb)->fix_segment = NULL;
-- 
1.8.4

Re: [PATCH] datapath: 16bit inner_network_header field in struct ovs_gso_cb

[Jesse Gross]

On Tue, May 20, 2014 at 3:41 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
> index b83a4c3..f459c18 100644
> --- a/datapath/linux/compat/gso.h
> +++ b/datapath/linux/compat/gso.h
>  #ifdef NET_SKBUFF_DATA_USES_OFFSET
> -#define skb_inner_network_header rpl_skb_inner_network_header
> -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
> -{
> -       return skb->head + OVS_GSO_CB(skb)->inner_network_header;
> -}
> -
>  #define skb_inner_mac_header rpl_skb_inner_mac_header
>  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
>  {
>         return skb->head + OVS_GSO_CB(skb)->inner_mac_header;
>  }
>
> +#define skb_inner_network_header rpl_skb_inner_network_header
> +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
> +{
> +       return skb_inner_mac_header(skb) +
> +               OVS_GSO_CB(skb)->inner_network_header;
> +}
> +
>  #else

This looks mostly fine to me but doesn't this need to be outside the
#ifdef NET_SKBUFF_DATA_USES_OFFSET block? Otherwise we will treat the
offset as a pointer on 32 bit kernels.

Re: [PATCH] datapath: 16bit inner_network_header field in struct ovs_gso_cb

[Simon Horman]

On Tue, May 20, 2014 at 12:54:32PM -0700, Jesse Gross wrote:
> On Tue, May 20, 2014 at 3:41 AM, Simon Horman <horms@verge.net.au> wrote:
> > diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
> > index b83a4c3..f459c18 100644
> > --- a/datapath/linux/compat/gso.h
> > +++ b/datapath/linux/compat/gso.h
> >  #ifdef NET_SKBUFF_DATA_USES_OFFSET
> > -#define skb_inner_network_header rpl_skb_inner_network_header
> > -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
> > -{
> > -       return skb->head + OVS_GSO_CB(skb)->inner_network_header;
> > -}
> > -
> >  #define skb_inner_mac_header rpl_skb_inner_mac_header
> >  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
> >  {
> >         return skb->head + OVS_GSO_CB(skb)->inner_mac_header;
> >  }
> >
> > +#define skb_inner_network_header rpl_skb_inner_network_header
> > +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
> > +{
> > +       return skb_inner_mac_header(skb) +
> > +               OVS_GSO_CB(skb)->inner_network_header;
> > +}
> > +
> >  #else
> 
> This looks mostly fine to me but doesn't this need to be outside the
> #ifdef NET_SKBUFF_DATA_USES_OFFSET block? Otherwise we will treat the
> offset as a pointer on 32 bit kernels.

Thanks, Somehow I completely missed that.
How about this?


From: Simon Horman <horms@verge.net.au>

[PATCH v1.1] datapath: 16bit inner_network_header field in struct ovs_gso_cb

The motivation for this is to create a 16bit hole in struct ovs_gso_cb
which may be used for the inner_protocol field which is needed
for the proposed implementation of compatibility for MPLS GSO segmentation.

This should be safe as inner_network_header is now an offset to
the inner_mac_header rather than skb->head.

As pointed out by Thomas Graf simply making both inner offsets 16bis is not
safe as there have been cases of overflow with "with collapsed TCP frames
on IB when the headroom grew beyond 64K. See commit 50bceae9bd ``tcp:
Reallocate headroom if it would overflow csum_start'' for additional
details."

This patch is based on suggestions by Thomas Graf and Jesse Gross.

Cc: Thomas Graf <tgraf@suug.ch>
Cc: Jesse Gross <jesse@nicira.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---
v1.1
* Move skb_inner_network_header outside #ifdef NET_SKBUFF_DATA_USES_OFFSET
  block so that the 16bit offset is not treated as a pointer on
  32bit kernels.
---
 datapath/linux/compat/gso.h | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
index b83a4c3..3041e88 100644
--- a/datapath/linux/compat/gso.h
+++ b/datapath/linux/compat/gso.h
@@ -11,8 +11,10 @@
 
 struct ovs_gso_cb {
 	struct ovs_skb_cb dp_cb;
-	sk_buff_data_t	inner_network_header;
-	sk_buff_data_t	inner_mac_header;
+	u16		inner_network_header;	/* Offset from
+						 * inner_mac_header */
+	/* 16bit hole */
+	sk_buff_data_t	inner_mac_header;	/* Offset from skb->head */
 	void (*fix_segment)(struct sk_buff *);
 };
 #define OVS_GSO_CB(skb) ((struct ovs_gso_cb *)(skb)->cb)
@@ -20,12 +22,6 @@ struct ovs_gso_cb {
 #define skb_inner_network_header rpl_skb_inner_network_header
 
 #ifdef NET_SKBUFF_DATA_USES_OFFSET
-#define skb_inner_network_header rpl_skb_inner_network_header
-static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
-{
-	return skb->head + OVS_GSO_CB(skb)->inner_network_header;
-}
-
 #define skb_inner_mac_header rpl_skb_inner_mac_header
 static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
 {
@@ -34,12 +30,6 @@ static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
 
 #else
 
-#define skb_inner_network_header rpl_skb_inner_network_header
-static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
-{
-	return OVS_GSO_CB(skb)->inner_network_header;
-}
-
 #define skb_inner_mac_header rpl_skb_inner_mac_header
 static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
 {
@@ -48,6 +38,13 @@ static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
 
 #endif
 
+#define skb_inner_network_header rpl_skb_inner_network_header
+static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
+{
+	return skb_inner_mac_header(skb) +
+		OVS_GSO_CB(skb)->inner_network_header;
+}
+
 #define skb_inner_network_offset rpl_skb_inner_network_offset
 static inline int skb_inner_network_offset(const struct sk_buff *skb)
 {
@@ -64,7 +61,8 @@ static inline int skb_inner_mac_offset(const struct sk_buff *skb)
 static inline void skb_reset_inner_headers(struct sk_buff *skb)
 {
 	BUILD_BUG_ON(sizeof(struct ovs_gso_cb) > FIELD_SIZEOF(struct sk_buff, cb));
-	OVS_GSO_CB(skb)->inner_network_header = skb->network_header;
+	OVS_GSO_CB(skb)->inner_network_header = skb->network_header -
+		skb->mac_header;
 	OVS_GSO_CB(skb)->inner_mac_header = skb->mac_header;
 
 	OVS_GSO_CB(skb)->fix_segment = NULL;
-- 
1.8.4

Re: [PATCH] datapath: 16bit inner_network_header field in struct ovs_gso_cb

[Jesse Gross]

On Tue, May 20, 2014 at 4:31 PM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> On Tue, May 20, 2014 at 12:54:32PM -0700, Jesse Gross wrote:
>> On Tue, May 20, 2014 at 3:41 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>> > diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
>> > index b83a4c3..f459c18 100644
>> > --- a/datapath/linux/compat/gso.h
>> > +++ b/datapath/linux/compat/gso.h
>> >  #ifdef NET_SKBUFF_DATA_USES_OFFSET
>> > -#define skb_inner_network_header rpl_skb_inner_network_header
>> > -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
>> > -{
>> > -       return skb->head + OVS_GSO_CB(skb)->inner_network_header;
>> > -}
>> > -
>> >  #define skb_inner_mac_header rpl_skb_inner_mac_header
>> >  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
>> >  {
>> >         return skb->head + OVS_GSO_CB(skb)->inner_mac_header;
>> >  }
>> >
>> > +#define skb_inner_network_header rpl_skb_inner_network_header
>> > +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
>> > +{
>> > +       return skb_inner_mac_header(skb) +
>> > +               OVS_GSO_CB(skb)->inner_network_header;
>> > +}
>> > +
>> >  #else
>>
>> This looks mostly fine to me but doesn't this need to be outside the
>> #ifdef NET_SKBUFF_DATA_USES_OFFSET block? Otherwise we will treat the
>> offset as a pointer on 32 bit kernels.
>
> Thanks, Somehow I completely missed that.
> How about this?
>
>
> From: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
>
> [PATCH v1.1] datapath: 16bit inner_network_header field in struct ovs_gso_cb
>
> The motivation for this is to create a 16bit hole in struct ovs_gso_cb
> which may be used for the inner_protocol field which is needed
> for the proposed implementation of compatibility for MPLS GSO segmentation.
>
> This should be safe as inner_network_header is now an offset to
> the inner_mac_header rather than skb->head.
>
> As pointed out by Thomas Graf simply making both inner offsets 16bis is not
> safe as there have been cases of overflow with "with collapsed TCP frames
> on IB when the headroom grew beyond 64K. See commit 50bceae9bd ``tcp:
> Reallocate headroom if it would overflow csum_start'' for additional
> details."
>
> This patch is based on suggestions by Thomas Graf and Jesse Gross.
>
> Cc: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
> Cc: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>

Applied, thanks.

Re: [PATCH] datapath: 16bit inner_network_header field in struct ovs_gso_cb

[Simon Horman]

On Tue, May 20, 2014 at 05:14:25PM -0700, Jesse Gross wrote:
> On Tue, May 20, 2014 at 4:31 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, May 20, 2014 at 12:54:32PM -0700, Jesse Gross wrote:
> >> On Tue, May 20, 2014 at 3:41 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
> >> > index b83a4c3..f459c18 100644
> >> > --- a/datapath/linux/compat/gso.h
> >> > +++ b/datapath/linux/compat/gso.h
> >> >  #ifdef NET_SKBUFF_DATA_USES_OFFSET
> >> > -#define skb_inner_network_header rpl_skb_inner_network_header
> >> > -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
> >> > -{
> >> > -       return skb->head + OVS_GSO_CB(skb)->inner_network_header;
> >> > -}
> >> > -
> >> >  #define skb_inner_mac_header rpl_skb_inner_mac_header
> >> >  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
> >> >  {
> >> >         return skb->head + OVS_GSO_CB(skb)->inner_mac_header;
> >> >  }
> >> >
> >> > +#define skb_inner_network_header rpl_skb_inner_network_header
> >> > +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb)
> >> > +{
> >> > +       return skb_inner_mac_header(skb) +
> >> > +               OVS_GSO_CB(skb)->inner_network_header;
> >> > +}
> >> > +
> >> >  #else
> >>
> >> This looks mostly fine to me but doesn't this need to be outside the
> >> #ifdef NET_SKBUFF_DATA_USES_OFFSET block? Otherwise we will treat the
> >> offset as a pointer on 32 bit kernels.
> >
> > Thanks, Somehow I completely missed that.
> > How about this?
> >
> >
> > From: Simon Horman <horms@verge.net.au>
> >
> > [PATCH v1.1] datapath: 16bit inner_network_header field in struct ovs_gso_cb
> >
> > The motivation for this is to create a 16bit hole in struct ovs_gso_cb
> > which may be used for the inner_protocol field which is needed
> > for the proposed implementation of compatibility for MPLS GSO segmentation.
> >
> > This should be safe as inner_network_header is now an offset to
> > the inner_mac_header rather than skb->head.
> >
> > As pointed out by Thomas Graf simply making both inner offsets 16bis is not
> > safe as there have been cases of overflow with "with collapsed TCP frames
> > on IB when the headroom grew beyond 64K. See commit 50bceae9bd ``tcp:
> > Reallocate headroom if it would overflow csum_start'' for additional
> > details."
> >
> > This patch is based on suggestions by Thomas Graf and Jesse Gross.
> >
> > Cc: Thomas Graf <tgraf@suug.ch>
> > Cc: Jesse Gross <jesse@nicira.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> Applied, thanks.

Thanks