* [PATCH v2 1/4] net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len
2018-03-01 6:13 [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements Daniel Axtens
@ 2018-03-01 6:13 ` Daniel Axtens
2018-03-02 21:11 ` Marcelo Ricardo Leitner
2018-03-01 6:13 ` [PATCH v2 2/4] net: sched: tbf: handle GSO_BY_FRAGS case in enqueue Daniel Axtens
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Daniel Axtens @ 2018-03-01 6:13 UTC (permalink / raw)
To: netdev; +Cc: Daniel Axtens
If you take a GSO skb, and split it into packets, will the network
length (L3 headers + L4 headers + payload) of those packets be small
enough to fit within a given MTU?
skb_gso_validate_mtu gives you the answer to that question. However,
we recently added to add a way to validate the MAC length of a split GSO
skb (L2+L3+L4+payload), and the names get confusing, so rename
skb_gso_validate_mtu to skb_gso_validate_network_len
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
include/linux/skbuff.h | 2 +-
net/core/skbuff.c | 11 ++++++-----
net/ipv4/ip_forward.c | 2 +-
net/ipv4/ip_output.c | 2 +-
net/ipv4/netfilter/nf_flow_table_ipv4.c | 2 +-
net/ipv6/ip6_output.c | 2 +-
net/ipv6/netfilter/nf_flow_table_ipv6.c | 2 +-
net/mpls/af_mpls.c | 2 +-
net/xfrm/xfrm_device.c | 2 +-
9 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c1e66bdcf583..a057dd1a75c7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3286,7 +3286,7 @@ void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
void skb_scrub_packet(struct sk_buff *skb, bool xnet);
unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
-bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
+bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu);
bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 09bd89c90a71..b63767008824 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4955,19 +4955,20 @@ static inline bool skb_gso_size_check(const struct sk_buff *skb,
}
/**
- * skb_gso_validate_mtu - Return in case such skb fits a given MTU
+ * skb_gso_validate_network_len - Will a split GSO skb fit into a given MTU?
*
* @skb: GSO skb
* @mtu: MTU to validate against
*
- * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
- * once split.
+ * skb_gso_validate_network_len validates if a given skb will fit a
+ * wanted MTU once split. It considers L3 headers, L4 headers, and the
+ * payload.
*/
-bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
+bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu)
{
return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
}
-EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
+EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
/**
* skb_gso_validate_mac_len - Will a split GSO skb fit in a given length?
diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 2dd21c3281a1..b54b948b0596 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -55,7 +55,7 @@ static bool ip_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
if (skb->ignore_df)
return false;
- if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
+ if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
return false;
return true;
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index e8e675be60ec..66340ab750e6 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -248,7 +248,7 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk,
/* common case: seglen is <= mtu
*/
- if (skb_gso_validate_mtu(skb, mtu))
+ if (skb_gso_validate_network_len(skb, mtu))
return ip_finish_output2(net, sk, skb);
/* Slowpath - GSO segment length exceeds the egress MTU.
diff --git a/net/ipv4/netfilter/nf_flow_table_ipv4.c b/net/ipv4/netfilter/nf_flow_table_ipv4.c
index 25d2975da156..2447077d163d 100644
--- a/net/ipv4/netfilter/nf_flow_table_ipv4.c
+++ b/net/ipv4/netfilter/nf_flow_table_ipv4.c
@@ -185,7 +185,7 @@ static bool __nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
if ((ip_hdr(skb)->frag_off & htons(IP_DF)) == 0)
return false;
- if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
+ if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
return false;
return true;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 997c7f19ad62..a8a919520090 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -412,7 +412,7 @@ static bool ip6_pkt_too_big(const struct sk_buff *skb, unsigned int mtu)
if (skb->ignore_df)
return false;
- if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
+ if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
return false;
return true;
diff --git a/net/ipv6/netfilter/nf_flow_table_ipv6.c b/net/ipv6/netfilter/nf_flow_table_ipv6.c
index d346705d6ee6..207cb35569b1 100644
--- a/net/ipv6/netfilter/nf_flow_table_ipv6.c
+++ b/net/ipv6/netfilter/nf_flow_table_ipv6.c
@@ -178,7 +178,7 @@ static bool __nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
if (skb->len <= mtu)
return false;
- if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
+ if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
return false;
return true;
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index e545a3c9365f..7a4de6d618b1 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -122,7 +122,7 @@ bool mpls_pkt_too_big(const struct sk_buff *skb, unsigned int mtu)
if (skb->len <= mtu)
return false;
- if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
+ if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
return false;
return true;
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 8e70291e586a..e87d6c4dd5b6 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -217,7 +217,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
if (skb->len <= mtu)
goto ok;
- if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
+ if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
goto ok;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 1/4] net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len
2018-03-01 6:13 ` [PATCH v2 1/4] net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len Daniel Axtens
@ 2018-03-02 21:11 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-03-02 21:11 UTC (permalink / raw)
To: Daniel Axtens; +Cc: netdev
On Thu, Mar 01, 2018 at 05:13:37PM +1100, Daniel Axtens wrote:
> If you take a GSO skb, and split it into packets, will the network
> length (L3 headers + L4 headers + payload) of those packets be small
> enough to fit within a given MTU?
>
> skb_gso_validate_mtu gives you the answer to that question. However,
> we recently added to add a way to validate the MAC length of a split GSO
> skb (L2+L3+L4+payload), and the names get confusing, so rename
> skb_gso_validate_mtu to skb_gso_validate_network_len
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> include/linux/skbuff.h | 2 +-
> net/core/skbuff.c | 11 ++++++-----
> net/ipv4/ip_forward.c | 2 +-
> net/ipv4/ip_output.c | 2 +-
> net/ipv4/netfilter/nf_flow_table_ipv4.c | 2 +-
> net/ipv6/ip6_output.c | 2 +-
> net/ipv6/netfilter/nf_flow_table_ipv6.c | 2 +-
> net/mpls/af_mpls.c | 2 +-
> net/xfrm/xfrm_device.c | 2 +-
> 9 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index c1e66bdcf583..a057dd1a75c7 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3286,7 +3286,7 @@ void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
> int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
> void skb_scrub_packet(struct sk_buff *skb, bool xnet);
> unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
> -bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
> +bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu);
> bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 09bd89c90a71..b63767008824 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4955,19 +4955,20 @@ static inline bool skb_gso_size_check(const struct sk_buff *skb,
> }
>
> /**
> - * skb_gso_validate_mtu - Return in case such skb fits a given MTU
> + * skb_gso_validate_network_len - Will a split GSO skb fit into a given MTU?
> *
> * @skb: GSO skb
> * @mtu: MTU to validate against
> *
> - * skb_gso_validate_mtu validates if a given skb will fit a wanted MTU
> - * once split.
> + * skb_gso_validate_network_len validates if a given skb will fit a
> + * wanted MTU once split. It considers L3 headers, L4 headers, and the
> + * payload.
> */
> -bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
> +bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu)
> {
> return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
> }
> -EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
> +EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
>
> /**
> * skb_gso_validate_mac_len - Will a split GSO skb fit in a given length?
> diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
> index 2dd21c3281a1..b54b948b0596 100644
> --- a/net/ipv4/ip_forward.c
> +++ b/net/ipv4/ip_forward.c
> @@ -55,7 +55,7 @@ static bool ip_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> if (skb->ignore_df)
> return false;
>
> - if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
> + if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
> return false;
>
> return true;
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index e8e675be60ec..66340ab750e6 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -248,7 +248,7 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk,
>
> /* common case: seglen is <= mtu
> */
> - if (skb_gso_validate_mtu(skb, mtu))
> + if (skb_gso_validate_network_len(skb, mtu))
> return ip_finish_output2(net, sk, skb);
>
> /* Slowpath - GSO segment length exceeds the egress MTU.
> diff --git a/net/ipv4/netfilter/nf_flow_table_ipv4.c b/net/ipv4/netfilter/nf_flow_table_ipv4.c
> index 25d2975da156..2447077d163d 100644
> --- a/net/ipv4/netfilter/nf_flow_table_ipv4.c
> +++ b/net/ipv4/netfilter/nf_flow_table_ipv4.c
> @@ -185,7 +185,7 @@ static bool __nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> if ((ip_hdr(skb)->frag_off & htons(IP_DF)) == 0)
> return false;
>
> - if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
> + if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
> return false;
>
> return true;
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 997c7f19ad62..a8a919520090 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -412,7 +412,7 @@ static bool ip6_pkt_too_big(const struct sk_buff *skb, unsigned int mtu)
> if (skb->ignore_df)
> return false;
>
> - if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
> + if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
> return false;
>
> return true;
> diff --git a/net/ipv6/netfilter/nf_flow_table_ipv6.c b/net/ipv6/netfilter/nf_flow_table_ipv6.c
> index d346705d6ee6..207cb35569b1 100644
> --- a/net/ipv6/netfilter/nf_flow_table_ipv6.c
> +++ b/net/ipv6/netfilter/nf_flow_table_ipv6.c
> @@ -178,7 +178,7 @@ static bool __nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> if (skb->len <= mtu)
> return false;
>
> - if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
> + if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
> return false;
>
> return true;
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index e545a3c9365f..7a4de6d618b1 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -122,7 +122,7 @@ bool mpls_pkt_too_big(const struct sk_buff *skb, unsigned int mtu)
> if (skb->len <= mtu)
> return false;
>
> - if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
> + if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
> return false;
>
> return true;
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 8e70291e586a..e87d6c4dd5b6 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -217,7 +217,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
> if (skb->len <= mtu)
> goto ok;
>
> - if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu))
> + if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
> goto ok;
> }
>
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] net: sched: tbf: handle GSO_BY_FRAGS case in enqueue
2018-03-01 6:13 [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements Daniel Axtens
2018-03-01 6:13 ` [PATCH v2 1/4] net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len Daniel Axtens
@ 2018-03-01 6:13 ` Daniel Axtens
2018-03-02 21:11 ` Marcelo Ricardo Leitner
2018-03-01 6:13 ` [PATCH v2 3/4] net: xfrm: use skb_gso_validate_network_len() to check gso sizes Daniel Axtens
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Daniel Axtens @ 2018-03-01 6:13 UTC (permalink / raw)
To: netdev; +Cc: Daniel Axtens
tbf_enqueue() checks the size of a packet before enqueuing it.
However, the GSO size check does not consider the GSO_BY_FRAGS
case, and so will drop GSO SCTP packets, causing a massive drop
in throughput.
Use skb_gso_validate_mac_len() instead, as it does consider that
case.
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
net/sched/sch_tbf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 229172d509cc..03225a8df973 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -188,7 +188,8 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch,
int ret;
if (qdisc_pkt_len(skb) > q->max_size) {
- if (skb_is_gso(skb) && skb_gso_mac_seglen(skb) <= q->max_size)
+ if (skb_is_gso(skb) &&
+ skb_gso_validate_mac_len(skb, q->max_size))
return tbf_segment(skb, sch, to_free);
return qdisc_drop(skb, sch, to_free);
}
--
2.14.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] net: sched: tbf: handle GSO_BY_FRAGS case in enqueue
2018-03-01 6:13 ` [PATCH v2 2/4] net: sched: tbf: handle GSO_BY_FRAGS case in enqueue Daniel Axtens
@ 2018-03-02 21:11 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-03-02 21:11 UTC (permalink / raw)
To: Daniel Axtens; +Cc: netdev
On Thu, Mar 01, 2018 at 05:13:38PM +1100, Daniel Axtens wrote:
> tbf_enqueue() checks the size of a packet before enqueuing it.
> However, the GSO size check does not consider the GSO_BY_FRAGS
> case, and so will drop GSO SCTP packets, causing a massive drop
> in throughput.
>
> Use skb_gso_validate_mac_len() instead, as it does consider that
> case.
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sched/sch_tbf.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> index 229172d509cc..03225a8df973 100644
> --- a/net/sched/sch_tbf.c
> +++ b/net/sched/sch_tbf.c
> @@ -188,7 +188,8 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> int ret;
>
> if (qdisc_pkt_len(skb) > q->max_size) {
> - if (skb_is_gso(skb) && skb_gso_mac_seglen(skb) <= q->max_size)
> + if (skb_is_gso(skb) &&
> + skb_gso_validate_mac_len(skb, q->max_size))
> return tbf_segment(skb, sch, to_free);
> return qdisc_drop(skb, sch, to_free);
> }
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] net: xfrm: use skb_gso_validate_network_len() to check gso sizes
2018-03-01 6:13 [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements Daniel Axtens
2018-03-01 6:13 ` [PATCH v2 1/4] net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len Daniel Axtens
2018-03-01 6:13 ` [PATCH v2 2/4] net: sched: tbf: handle GSO_BY_FRAGS case in enqueue Daniel Axtens
@ 2018-03-01 6:13 ` Daniel Axtens
2018-03-02 21:12 ` Marcelo Ricardo Leitner
2018-03-01 6:13 ` [PATCH v2 4/4] net: make skb_gso_*_seglen functions private Daniel Axtens
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Daniel Axtens @ 2018-03-01 6:13 UTC (permalink / raw)
To: netdev; +Cc: Daniel Axtens
Replace skb_gso_network_seglen() with
skb_gso_validate_network_len(), as it considers the GSO_BY_FRAGS
case.
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
net/ipv4/xfrm4_output.c | 3 ++-
net/ipv6/xfrm6_output.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
index 94b8702603bc..be980c195fc5 100644
--- a/net/ipv4/xfrm4_output.c
+++ b/net/ipv4/xfrm4_output.c
@@ -30,7 +30,8 @@ static int xfrm4_tunnel_check_size(struct sk_buff *skb)
mtu = dst_mtu(skb_dst(skb));
if ((!skb_is_gso(skb) && skb->len > mtu) ||
- (skb_is_gso(skb) && skb_gso_network_seglen(skb) > ip_skb_dst_mtu(skb->sk, skb))) {
+ (skb_is_gso(skb) &&
+ !skb_gso_validate_network_len(skb, ip_skb_dst_mtu(skb->sk, skb)))) {
skb->protocol = htons(ETH_P_IP);
if (skb->sk)
diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index 8ae87d4ec5ff..5959ce9620eb 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -82,7 +82,7 @@ static int xfrm6_tunnel_check_size(struct sk_buff *skb)
if ((!skb_is_gso(skb) && skb->len > mtu) ||
(skb_is_gso(skb) &&
- skb_gso_network_seglen(skb) > ip6_skb_dst_mtu(skb))) {
+ !skb_gso_validate_network_len(skb, ip6_skb_dst_mtu(skb)))) {
skb->dev = dst->dev;
skb->protocol = htons(ETH_P_IPV6);
--
2.14.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/4] net: xfrm: use skb_gso_validate_network_len() to check gso sizes
2018-03-01 6:13 ` [PATCH v2 3/4] net: xfrm: use skb_gso_validate_network_len() to check gso sizes Daniel Axtens
@ 2018-03-02 21:12 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-03-02 21:12 UTC (permalink / raw)
To: Daniel Axtens; +Cc: netdev
On Thu, Mar 01, 2018 at 05:13:39PM +1100, Daniel Axtens wrote:
> Replace skb_gso_network_seglen() with
> skb_gso_validate_network_len(), as it considers the GSO_BY_FRAGS
> case.
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/ipv4/xfrm4_output.c | 3 ++-
> net/ipv6/xfrm6_output.c | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
> index 94b8702603bc..be980c195fc5 100644
> --- a/net/ipv4/xfrm4_output.c
> +++ b/net/ipv4/xfrm4_output.c
> @@ -30,7 +30,8 @@ static int xfrm4_tunnel_check_size(struct sk_buff *skb)
>
> mtu = dst_mtu(skb_dst(skb));
> if ((!skb_is_gso(skb) && skb->len > mtu) ||
> - (skb_is_gso(skb) && skb_gso_network_seglen(skb) > ip_skb_dst_mtu(skb->sk, skb))) {
> + (skb_is_gso(skb) &&
> + !skb_gso_validate_network_len(skb, ip_skb_dst_mtu(skb->sk, skb)))) {
> skb->protocol = htons(ETH_P_IP);
>
> if (skb->sk)
> diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
> index 8ae87d4ec5ff..5959ce9620eb 100644
> --- a/net/ipv6/xfrm6_output.c
> +++ b/net/ipv6/xfrm6_output.c
> @@ -82,7 +82,7 @@ static int xfrm6_tunnel_check_size(struct sk_buff *skb)
>
> if ((!skb_is_gso(skb) && skb->len > mtu) ||
> (skb_is_gso(skb) &&
> - skb_gso_network_seglen(skb) > ip6_skb_dst_mtu(skb))) {
> + !skb_gso_validate_network_len(skb, ip6_skb_dst_mtu(skb)))) {
> skb->dev = dst->dev;
> skb->protocol = htons(ETH_P_IPV6);
>
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] net: make skb_gso_*_seglen functions private
2018-03-01 6:13 [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements Daniel Axtens
` (2 preceding siblings ...)
2018-03-01 6:13 ` [PATCH v2 3/4] net: xfrm: use skb_gso_validate_network_len() to check gso sizes Daniel Axtens
@ 2018-03-01 6:13 ` Daniel Axtens
2018-03-02 21:13 ` Marcelo Ricardo Leitner
[not found] ` <CAP-MU4PDm-5WaGorMUa4J9GVkmXPJjbyAAaUMvefEsqzFrxQWg@mail.gmail.com>
2018-03-04 23:11 ` David Miller
5 siblings, 1 reply; 11+ messages in thread
From: Daniel Axtens @ 2018-03-01 6:13 UTC (permalink / raw)
To: netdev; +Cc: Daniel Axtens
They're very hard to use properly as they do not consider the
GSO_BY_FRAGS case. Code should use skb_gso_validate_network_len
and skb_gso_validate_mac_len as they do consider this case.
Make the seglen functions static, which stops people using them
outside of skbuff.c
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
v2: drop inline from functions.
---
include/linux/skbuff.h | 33 ---------------------------------
net/core/skbuff.c | 37 +++++++++++++++++++++++++++++++++++--
2 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a057dd1a75c7..ddf77cf4ff2d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3285,7 +3285,6 @@ int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,
void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
void skb_scrub_packet(struct sk_buff *skb, bool xnet);
-unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu);
bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
@@ -4104,38 +4103,6 @@ static inline bool skb_head_is_locked(const struct sk_buff *skb)
return !skb->head_frag || skb_cloned(skb);
}
-/**
- * skb_gso_network_seglen - Return length of individual segments of a gso packet
- *
- * @skb: GSO skb
- *
- * skb_gso_network_seglen is used to determine the real size of the
- * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
- *
- * The MAC/L2 header is not accounted for.
- */
-static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
-{
- unsigned int hdr_len = skb_transport_header(skb) -
- skb_network_header(skb);
- return hdr_len + skb_gso_transport_seglen(skb);
-}
-
-/**
- * skb_gso_mac_seglen - Return length of individual segments of a gso packet
- *
- * @skb: GSO skb
- *
- * skb_gso_mac_seglen is used to determine the real size of the
- * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
- * headers (TCP/UDP).
- */
-static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
-{
- unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
- return hdr_len + skb_gso_transport_seglen(skb);
-}
-
/* Local Checksum Offload.
* Compute outer checksum based on the assumption that the
* inner checksum will be offloaded later.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b63767008824..0bb0d8877954 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4891,7 +4891,7 @@ EXPORT_SYMBOL_GPL(skb_scrub_packet);
*
* The MAC/L2 or network (IP, IPv6) headers are not accounted for.
*/
-unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
+static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
{
const struct skb_shared_info *shinfo = skb_shinfo(skb);
unsigned int thlen = 0;
@@ -4913,7 +4913,40 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
*/
return thlen + shinfo->gso_size;
}
-EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
+
+/**
+ * skb_gso_network_seglen - Return length of individual segments of a gso packet
+ *
+ * @skb: GSO skb
+ *
+ * skb_gso_network_seglen is used to determine the real size of the
+ * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
+ *
+ * The MAC/L2 header is not accounted for.
+ */
+static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
+{
+ unsigned int hdr_len = skb_transport_header(skb) -
+ skb_network_header(skb);
+
+ return hdr_len + skb_gso_transport_seglen(skb);
+}
+
+/**
+ * skb_gso_mac_seglen - Return length of individual segments of a gso packet
+ *
+ * @skb: GSO skb
+ *
+ * skb_gso_mac_seglen is used to determine the real size of the
+ * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
+ * headers (TCP/UDP).
+ */
+static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
+{
+ unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
+
+ return hdr_len + skb_gso_transport_seglen(skb);
+}
/**
* skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
--
2.14.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/4] net: make skb_gso_*_seglen functions private
2018-03-01 6:13 ` [PATCH v2 4/4] net: make skb_gso_*_seglen functions private Daniel Axtens
@ 2018-03-02 21:13 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 11+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-03-02 21:13 UTC (permalink / raw)
To: Daniel Axtens; +Cc: netdev
On Thu, Mar 01, 2018 at 05:13:40PM +1100, Daniel Axtens wrote:
> They're very hard to use properly as they do not consider the
> GSO_BY_FRAGS case. Code should use skb_gso_validate_network_len
> and skb_gso_validate_mac_len as they do consider this case.
>
> Make the seglen functions static, which stops people using them
> outside of skbuff.c
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>
> ---
>
> v2: drop inline from functions.
> ---
> include/linux/skbuff.h | 33 ---------------------------------
> net/core/skbuff.c | 37 +++++++++++++++++++++++++++++++++++--
> 2 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a057dd1a75c7..ddf77cf4ff2d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3285,7 +3285,6 @@ int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,
> void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
> int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
> void skb_scrub_packet(struct sk_buff *skb, bool xnet);
> -unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
> bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu);
> bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> @@ -4104,38 +4103,6 @@ static inline bool skb_head_is_locked(const struct sk_buff *skb)
> return !skb->head_frag || skb_cloned(skb);
> }
>
> -/**
> - * skb_gso_network_seglen - Return length of individual segments of a gso packet
> - *
> - * @skb: GSO skb
> - *
> - * skb_gso_network_seglen is used to determine the real size of the
> - * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
> - *
> - * The MAC/L2 header is not accounted for.
> - */
> -static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
> -{
> - unsigned int hdr_len = skb_transport_header(skb) -
> - skb_network_header(skb);
> - return hdr_len + skb_gso_transport_seglen(skb);
> -}
> -
> -/**
> - * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> - *
> - * @skb: GSO skb
> - *
> - * skb_gso_mac_seglen is used to determine the real size of the
> - * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> - * headers (TCP/UDP).
> - */
> -static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> -{
> - unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> - return hdr_len + skb_gso_transport_seglen(skb);
> -}
> -
> /* Local Checksum Offload.
> * Compute outer checksum based on the assumption that the
> * inner checksum will be offloaded later.
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b63767008824..0bb0d8877954 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4891,7 +4891,7 @@ EXPORT_SYMBOL_GPL(skb_scrub_packet);
> *
> * The MAC/L2 or network (IP, IPv6) headers are not accounted for.
> */
> -unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
> +static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
> {
> const struct skb_shared_info *shinfo = skb_shinfo(skb);
> unsigned int thlen = 0;
> @@ -4913,7 +4913,40 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
> */
> return thlen + shinfo->gso_size;
> }
> -EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
> +
> +/**
> + * skb_gso_network_seglen - Return length of individual segments of a gso packet
> + *
> + * @skb: GSO skb
> + *
> + * skb_gso_network_seglen is used to determine the real size of the
> + * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
> + *
> + * The MAC/L2 header is not accounted for.
> + */
> +static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
> +{
> + unsigned int hdr_len = skb_transport_header(skb) -
> + skb_network_header(skb);
> +
> + return hdr_len + skb_gso_transport_seglen(skb);
> +}
> +
> +/**
> + * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> + *
> + * @skb: GSO skb
> + *
> + * skb_gso_mac_seglen is used to determine the real size of the
> + * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> + * headers (TCP/UDP).
> + */
> +static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> +{
> + unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> +
> + return hdr_len + skb_gso_transport_seglen(skb);
> +}
>
> /**
> * skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CAP-MU4PDm-5WaGorMUa4J9GVkmXPJjbyAAaUMvefEsqzFrxQWg@mail.gmail.com>]
* Re: [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements
[not found] ` <CAP-MU4PDm-5WaGorMUa4J9GVkmXPJjbyAAaUMvefEsqzFrxQWg@mail.gmail.com>
@ 2018-03-01 20:41 ` Shannon Nelson
0 siblings, 0 replies; 11+ messages in thread
From: Shannon Nelson @ 2018-03-01 20:41 UTC (permalink / raw)
To: netdev@vger.kernel.org, Daniel Axtens
> From: Daniel Axtens <dja@axtens.net>
> Date: Wed, Feb 28, 2018 at 10:13 PM
>
> As requested [1], I went through and had a look at users of gso_size to
> see if there were things that need to be fixed to consider
> GSO_BY_FRAGS, and I have tried to improve our helper functions to deal
> with this case.
>
> I found a few. This fixes bugs relating to the use of
> skb_gso_*_seglen() where GSO_BY_FRAGS is not considered.
>
> Patch 1 renames skb_gso_validate_mtu to skb_gso_validate_network_len.
> This is follow-up to my earlier patch 2b16f048729b ("net: create
> skb_gso_validate_mac_len()"), and just makes everything a bit clearer.
>
> Patches 2 and 3 replace the final users of skb_gso_network_seglen() -
> which doesn't consider GSO_BY_FRAGS - with
> skb_gso_validate_network_len(), which does. This allows me to make the
> skb_gso_*_seglen functions private in patch 4 - now future users won't
> accidentally do the wrong comparison.
>
> Two things remain. One is qdisc_pkt_len_init, which is discussed at
> [2] - it's caught up in the GSO_DODGY mess. I don't have any expertise
> in GSO_DODGY, and it looks like a good clean fix will involve
> unpicking the whole validation mess, so I have left it for now.
>
> Secondly, there are 3 eBPF opcodes that change the gso_size of an SKB
> and don't consider GSO_BY_FRAGS. This is going through the bpf tree.
>
> Regards,
> Daniel
>
> [1] https://patchwork.ozlabs.org/comment/1852414/
> [2] https://www.spinics.net/lists/netdev/msg482397.html
>
> PS: This is all in the core networking stack. For a driver to be
> affected by this it would need to support NETIF_F_GSO_SCTP /
> NETIF_F_GSO_SOFTWARE and then either use gso_size or not be a purely
> virtual device. (Many drivers look at gso_size, but do not support
> SCTP segmentation, so the core network will segment an SCTP gso before
> it hits them.) Based on that, the only driver that may be affected is
> sunvnet, but I have no way of testing it, so I haven't looked at it.
I took a quick peek into sunvnet to check on this, and it looks like the
code in sunvnet_common.c::vnet_handle_offloads() is already mis-handling
SCTP gso packets, so I don't think you're adding any more breakage.
I suspect we should change sunvnet's use of NETIF_F_GSO_SOFTWARE to
NETIF_F_ALL_TSO...
sln
>
> v2: split out bpf stuff
> fix review comments from Dave Miller
>
> Daniel Axtens (4):
> net: rename skb_gso_validate_mtu -> skb_gso_validate_network_len
> net: sched: tbf: handle GSO_BY_FRAGS case in enqueue
> net: xfrm: use skb_gso_validate_network_len() to check gso sizes
> net: make skb_gso_*_seglen functions private
>
> include/linux/skbuff.h | 35 +-----------------------
> net/core/skbuff.c | 48 ++++++++++++++++++++++++++++-----
> net/ipv4/ip_forward.c | 2 +-
> net/ipv4/ip_output.c | 2 +-
> net/ipv4/netfilter/nf_flow_table_ipv4.c | 2 +-
> net/ipv4/xfrm4_output.c | 3 ++-
> net/ipv6/ip6_output.c | 2 +-
> net/ipv6/netfilter/nf_flow_table_ipv6.c | 2 +-
> net/ipv6/xfrm6_output.c | 2 +-
> net/mpls/af_mpls.c | 2 +-
> net/sched/sch_tbf.c | 3 ++-
> net/xfrm/xfrm_device.c | 2 +-
> 12 files changed, 54 insertions(+), 51 deletions(-)
>
> --
> 2.14.1
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements
2018-03-01 6:13 [PATCH v2 0/4] GSO_BY_FRAGS correctness improvements Daniel Axtens
` (4 preceding siblings ...)
[not found] ` <CAP-MU4PDm-5WaGorMUa4J9GVkmXPJjbyAAaUMvefEsqzFrxQWg@mail.gmail.com>
@ 2018-03-04 23:11 ` David Miller
5 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2018-03-04 23:11 UTC (permalink / raw)
To: dja; +Cc: netdev
From: Daniel Axtens <dja@axtens.net>
Date: Thu, 1 Mar 2018 17:13:36 +1100
> As requested [1], I went through and had a look at users of gso_size to
> see if there were things that need to be fixed to consider
> GSO_BY_FRAGS, and I have tried to improve our helper functions to deal
> with this case.
...
Series applied, thanks Daniel.
^ permalink raw reply [flat|nested] 11+ messages in thread