* [PATCH net v2 1/3] net/sched: act_csum: pull all VLAN headers before checksumming
2019-05-30 18:03 [PATCH net v2 0/3] net/sched: fix QinQ when actions read IPv4/IPv6 header Davide Caratti
@ 2019-05-30 18:03 ` Davide Caratti
2019-05-30 18:08 ` Stephen Hemminger
2019-05-30 18:03 ` [PATCH net v2 2/3] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet Davide Caratti
2019-05-30 18:03 ` [PATCH net v2 3/3] net/sched: act_skbedit: fix 'inheritdsfield' " Davide Caratti
2 siblings, 1 reply; 6+ messages in thread
From: Davide Caratti @ 2019-05-30 18:03 UTC (permalink / raw)
To: Eric Dumazet, Cong Wang, Jiri Pirko, Jamal Hadi Salim,
David S . Miller, netdev
Cc: shuali, Eli Britstein
TC 'csum' action needs to read the packets' ethertype. In case the value
is encoded in unstripped/nested VLAN headers, call pskb_may_pull() to be
sure that all tags are in the linear part of skb. While at it, move this
code in a helper function: other TC actions (like 'pedit' and 'skbedit')
might need to read the innermost ethertype.
Fixes: 2ecba2d1e45b ("net: sched: act_csum: Fix csum calc for tagged packets")
CC: Eli Britstein <elibr@mellanox.com>
CC: Li Shuang <shuali@redhat.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
include/net/pkt_cls.h | 21 +++++++++++++++++++++
net/sched/act_csum.c | 14 ++------------
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 514e3c80ecc1..0215dfbda9ac 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -953,4 +953,25 @@ struct tc_root_qopt_offload {
bool ingress;
};
+static inline int tc_skb_pull_vlans(struct sk_buff *skb,
+ unsigned int *hdr_count,
+ __be16 *proto)
+{
+ if (skb_vlan_tag_present(skb))
+ *proto = skb->protocol;
+
+ while (eth_type_vlan(*proto)) {
+ struct vlan_hdr *vlan;
+
+ if (unlikely(!pskb_may_pull(skb, VLAN_HLEN)))
+ return -ENOMEM;
+
+ vlan = (struct vlan_hdr *)skb->data;
+ *proto = vlan->h_vlan_encapsulated_proto;
+ skb_pull(skb, VLAN_HLEN);
+ skb_reset_network_header(skb);
+ (*hdr_count)++;
+ }
+ return 0;
+}
#endif
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 14bb525e355e..e8308ddcae9d 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -574,7 +574,6 @@ static int tcf_csum_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
struct tcf_csum *p = to_tcf_csum(a);
- bool orig_vlan_tag_present = false;
unsigned int vlan_hdr_count = 0;
struct tcf_csum_params *params;
u32 update_flags;
@@ -604,17 +603,8 @@ static int tcf_csum_act(struct sk_buff *skb, const struct tc_action *a,
break;
case cpu_to_be16(ETH_P_8021AD): /* fall through */
case cpu_to_be16(ETH_P_8021Q):
- if (skb_vlan_tag_present(skb) && !orig_vlan_tag_present) {
- protocol = skb->protocol;
- orig_vlan_tag_present = true;
- } else {
- struct vlan_hdr *vlan = (struct vlan_hdr *)skb->data;
-
- protocol = vlan->h_vlan_encapsulated_proto;
- skb_pull(skb, VLAN_HLEN);
- skb_reset_network_header(skb);
- vlan_hdr_count++;
- }
+ if (tc_skb_pull_vlans(skb, &vlan_hdr_count, &protocol))
+ goto drop;
goto again;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net v2 1/3] net/sched: act_csum: pull all VLAN headers before checksumming
2019-05-30 18:03 ` [PATCH net v2 1/3] net/sched: act_csum: pull all VLAN headers before checksumming Davide Caratti
@ 2019-05-30 18:08 ` Stephen Hemminger
2019-05-31 9:02 ` Davide Caratti
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2019-05-30 18:08 UTC (permalink / raw)
To: Davide Caratti
Cc: Eric Dumazet, Cong Wang, Jiri Pirko, Jamal Hadi Salim,
David S . Miller, netdev, shuali, Eli Britstein
On Thu, 30 May 2019 20:03:41 +0200
Davide Caratti <dcaratti@redhat.com> wrote:
>
> +static inline int tc_skb_pull_vlans(struct sk_buff *skb,
> + unsigned int *hdr_count,
> + __be16 *proto)
> +{
> + if (skb_vlan_tag_present(skb))
> + *proto = skb->protocol;
> +
> + while (eth_type_vlan(*proto)) {
> + struct vlan_hdr *vlan;
> +
> + if (unlikely(!pskb_may_pull(skb, VLAN_HLEN)))
> + return -ENOMEM;
> +
> + vlan = (struct vlan_hdr *)skb->data;
> + *proto = vlan->h_vlan_encapsulated_proto;
> + skb_pull(skb, VLAN_HLEN);
> + skb_reset_network_header(skb);
> + (*hdr_count)++;
> + }
> + return 0;
> +}
Does this really need to be an inline, or could it just be
part of the sched_api?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net v2 1/3] net/sched: act_csum: pull all VLAN headers before checksumming
2019-05-30 18:08 ` Stephen Hemminger
@ 2019-05-31 9:02 ` Davide Caratti
0 siblings, 0 replies; 6+ messages in thread
From: Davide Caratti @ 2019-05-31 9:02 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Eric Dumazet, Cong Wang, Jiri Pirko, Jamal Hadi Salim,
David S . Miller, netdev, shuali, Eli Britstein
On Thu, 2019-05-30 at 11:08 -0700, Stephen Hemminger wrote:
> On Thu, 30 May 2019 20:03:41 +0200
> Davide Caratti <dcaratti@redhat.com> wrote:
>
> >
> > +static inline int tc_skb_pull_vlans(struct sk_buff *skb,
> > + unsigned int *hdr_count,
> > + __be16 *proto)
> > +{
> > + if (skb_vlan_tag_present(skb))
> > + *proto = skb->protocol;
> > +
> > + while (eth_type_vlan(*proto)) {
> > + struct vlan_hdr *vlan;
> > +
> > + if (unlikely(!pskb_may_pull(skb, VLAN_HLEN)))
> > + return -ENOMEM;
> > +
> > + vlan = (struct vlan_hdr *)skb->data;
> > + *proto = vlan->h_vlan_encapsulated_proto;
> > + skb_pull(skb, VLAN_HLEN);
> > + skb_reset_network_header(skb);
> > + (*hdr_count)++;
> > + }
> > + return 0;
> > +}
>
> Does this really need to be an inline, or could it just be
> part of the sched_api?
yes, you are right: I will send a v3.
thanks,
--
davide
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v2 2/3] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet
2019-05-30 18:03 [PATCH net v2 0/3] net/sched: fix QinQ when actions read IPv4/IPv6 header Davide Caratti
2019-05-30 18:03 ` [PATCH net v2 1/3] net/sched: act_csum: pull all VLAN headers before checksumming Davide Caratti
@ 2019-05-30 18:03 ` Davide Caratti
2019-05-30 18:03 ` [PATCH net v2 3/3] net/sched: act_skbedit: fix 'inheritdsfield' " Davide Caratti
2 siblings, 0 replies; 6+ messages in thread
From: Davide Caratti @ 2019-05-30 18:03 UTC (permalink / raw)
To: Eric Dumazet, Cong Wang, Jiri Pirko, Jamal Hadi Salim,
David S . Miller, netdev
Cc: shuali, Eli Britstein
like it has been done in commit 2ecba2d1e45b ("net: sched: act_csum: Fix
csum calc for tagged packets"), also 'pedit' needs to adjust the network
offset when multiple tags are present in the packets: otherwise wrong IP
headers (but good checksums) can be observed with the following command:
# tc filter add dev test0 parent ffff: protocol 802.1Q flower \
vlan_ethtype ipv4 action \
pedit ex munge ip ttl set 10 pipe \
csum ip and icmp pipe \
mirred egress redirect dev test1
Fixes: d8b9605d2697 ("net: sched: fix skb->protocol use in case of accelerated vlan path")
Reported-by: Li Shuang <shuali@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_pedit.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index d790c02b9c6c..26e43d300160 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -277,9 +277,11 @@ static bool offset_valid(struct sk_buff *skb, int offset)
}
static int pedit_skb_hdr_offset(struct sk_buff *skb,
- enum pedit_header_type htype, int *hoffset)
+ enum pedit_header_type htype, int *hoffset,
+ unsigned int *vlan_hdr_count)
{
int ret = -EINVAL;
+ __be16 protocol;
switch (htype) {
case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
@@ -291,8 +293,18 @@ static int pedit_skb_hdr_offset(struct sk_buff *skb,
case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK:
case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
- *hoffset = skb_network_offset(skb);
- ret = 0;
+ protocol = tc_skb_protocol(skb);
+again:
+ switch (protocol) {
+ case cpu_to_be16(ETH_P_8021AD): /* fall through */
+ case cpu_to_be16(ETH_P_8021Q):
+ if (!tc_skb_pull_vlans(skb, vlan_hdr_count, &protocol))
+ goto again;
+ return ret;
+ default:
+ *hoffset = skb_network_offset(skb);
+ ret = 0;
+ }
break;
case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
@@ -313,6 +325,7 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
struct tcf_pedit *p = to_pedit(a);
+ unsigned int vlan_hdr_count = 0;
int i;
if (skb_unclone(skb, GFP_ATOMIC))
@@ -343,7 +356,8 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
tkey_ex++;
}
- rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
+ rc = pedit_skb_hdr_offset(skb, htype, &hoffset,
+ &vlan_hdr_count);
if (rc) {
pr_info("tc action pedit bad header type specified (0x%x)\n",
htype);
@@ -407,6 +421,10 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
bad:
p->tcf_qstats.overlimits++;
done:
+ while (vlan_hdr_count--) {
+ skb_push(skb, VLAN_HLEN);
+ skb_reset_network_header(skb);
+ }
bstats_update(&p->tcf_bstats, skb);
spin_unlock(&p->tcf_lock);
return p->tcf_action;
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net v2 3/3] net/sched: act_skbedit: fix 'inheritdsfield' in case of QinQ packet
2019-05-30 18:03 [PATCH net v2 0/3] net/sched: fix QinQ when actions read IPv4/IPv6 header Davide Caratti
2019-05-30 18:03 ` [PATCH net v2 1/3] net/sched: act_csum: pull all VLAN headers before checksumming Davide Caratti
2019-05-30 18:03 ` [PATCH net v2 2/3] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet Davide Caratti
@ 2019-05-30 18:03 ` Davide Caratti
2 siblings, 0 replies; 6+ messages in thread
From: Davide Caratti @ 2019-05-30 18:03 UTC (permalink / raw)
To: Eric Dumazet, Cong Wang, Jiri Pirko, Jamal Hadi Salim,
David S . Miller, netdev
Cc: shuali, Eli Britstein
like it was previously done on TC 'csum' (see commit 2ecba2d1e45b ("net:
sched: act_csum: Fix csum calc for tagged packets")), TC 'skbedit' might
need to adjust the network offset, if the packet has unstripped/multiple
tags: otherwise 'inheritdsfield' will just be skipped for QinQ packets.
Fixes: e7e3728bd776 ("net:sched: add action inheritdsfield to skbedit")
CC: Li Shuang <shuali@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_skbedit.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 7ec159b95364..693a4317de6e 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -39,6 +39,7 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
{
struct tcf_skbedit *d = to_skbedit(a);
struct tcf_skbedit_params *params;
+ unsigned int vlan_hdr_count = 0;
int action;
tcf_lastuse_update(&d->tcf_tm);
@@ -50,9 +51,17 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
if (params->flags & SKBEDIT_F_PRIORITY)
skb->priority = params->priority;
if (params->flags & SKBEDIT_F_INHERITDSFIELD) {
- int wlen = skb_network_offset(skb);
-
- switch (tc_skb_protocol(skb)) {
+ __be16 proto = tc_skb_protocol(skb);
+ int wlen;
+
+again:
+ wlen = skb_network_offset(skb);
+ switch (proto) {
+ case htons(ETH_P_8021AD): /* Fall Through */
+ case htons(ETH_P_8021Q):
+ if (tc_skb_pull_vlans(skb, &vlan_hdr_count, &proto))
+ goto err;
+ goto again;
case htons(ETH_P_IP):
wlen += sizeof(struct iphdr);
if (!pskb_may_pull(skb, wlen))
@@ -61,7 +70,7 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
break;
case htons(ETH_P_IPV6):
- wlen += sizeof(struct ipv6hdr);
+ wlen += sizeof(struct ipv6hdr);
if (!pskb_may_pull(skb, wlen))
goto err;
skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
@@ -77,11 +86,18 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
}
if (params->flags & SKBEDIT_F_PTYPE)
skb->pkt_type = params->ptype;
+
+out:
+ while (vlan_hdr_count--) {
+ skb_push(skb, VLAN_HLEN);
+ skb_reset_network_header(skb);
+ }
return action;
err:
qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
- return TC_ACT_SHOT;
+ action = TC_ACT_SHOT;
+ goto out;
}
static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread