* [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action
@ 2019-10-12 11:55 Davide Caratti
2019-10-12 11:55 ` [PATCH net v2 1/2] net: avoid errors when trying to pop MLPS header on non-MPLS packets Davide Caratti
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Davide Caratti @ 2019-10-12 11:55 UTC (permalink / raw)
To: lkp
Cc: davem, dcaratti, john.hurley, kbuild-all, lorenzo, netdev,
xiyou.wangcong, Simon Horman
this series contains two fixes for TC 'act_mpls', that try to address
two problems that can be observed configuring simple 'push' / 'pop'
operations:
- patch 1/2 avoids dropping non-MPLS packets that pass through the MPLS
'pop' action.
- patch 2/2 fixes corruption of the L2 header that occurs when 'push'
or 'pop' actions are configured in TC egress path.
v2: - change commit message in patch 1/2 to better describe that the
patch impacts only TC, thanks to Simon Horman
- fix missing documentation of 'mac_len' in patch 2/2
Davide Caratti (2):
net: avoid errors when trying to pop MLPS header on non-MPLS packets
net/sched: fix corrupted L2 header with MPLS 'push' and 'pop' actions
include/linux/skbuff.h | 5 +++--
net/core/skbuff.c | 21 ++++++++++++---------
net/openvswitch/actions.c | 5 +++--
net/sched/act_mpls.c | 12 ++++++++----
4 files changed, 26 insertions(+), 17 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH net v2 1/2] net: avoid errors when trying to pop MLPS header on non-MPLS packets 2019-10-12 11:55 [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Davide Caratti @ 2019-10-12 11:55 ` Davide Caratti 2019-10-12 11:55 ` [PATCH net v2 2/2] net/sched: fix corrupted L2 header with MPLS 'push' and 'pop' actions Davide Caratti ` (2 subsequent siblings) 3 siblings, 0 replies; 5+ messages in thread From: Davide Caratti @ 2019-10-12 11:55 UTC (permalink / raw) To: lkp Cc: davem, dcaratti, john.hurley, kbuild-all, lorenzo, netdev, xiyou.wangcong, Simon Horman the following script: # tc qdisc add dev eth0 clsact # tc filter add dev eth0 egress matchall action mpls pop implicitly makes the kernel drop all packets transmitted by eth0, if they don't have a MPLS header. This behavior is uncommon: other encapsulations (like VLAN) just let the packet pass unmodified. Since the result of MPLS 'pop' operation would be the same regardless of the presence / absence of MPLS header(s) in the original packet, we can let skb_mpls_pop() return 0 when dealing with non-MPLS packets. For the OVS use-case, this is acceptable because __ovs_nla_copy_actions() already ensures that MPLS 'pop' operation only occurs with packets having an MPLS Ethernet type (and there are no other callers in current code, so the semantic change should be ok). v2: better documentation of use-cases for skb_mpls_pop(), thanks to Simon Horman Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC") Reviewed-by: Simon Horman <simon.horman@netronome.com> Acked-by: John Hurley <john.hurley@netronome.com> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/core/skbuff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 529133611ea2..cd59ccd6da57 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -5536,7 +5536,7 @@ int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto) int err; if (unlikely(!eth_p_mpls(skb->protocol))) - return -EINVAL; + return 0; err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN); if (unlikely(err)) -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net v2 2/2] net/sched: fix corrupted L2 header with MPLS 'push' and 'pop' actions 2019-10-12 11:55 [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Davide Caratti 2019-10-12 11:55 ` [PATCH net v2 1/2] net: avoid errors when trying to pop MLPS header on non-MPLS packets Davide Caratti @ 2019-10-12 11:55 ` Davide Caratti 2019-10-12 12:41 ` [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Simon Horman 2019-10-16 0:16 ` David Miller 3 siblings, 0 replies; 5+ messages in thread From: Davide Caratti @ 2019-10-12 11:55 UTC (permalink / raw) To: lkp Cc: davem, dcaratti, john.hurley, kbuild-all, lorenzo, netdev, xiyou.wangcong, Simon Horman the following script: # tc qdisc add dev eth0 clsact # tc filter add dev eth0 egress protocol ip matchall \ > action mpls push protocol mpls_uc label 0x355aa bos 1 causes corruption of all IP packets transmitted by eth0. On TC egress, we can't rely on the value of skb->mac_len, because it's 0 and a MPLS 'push' operation will result in an overwrite of the first 4 octets in the packet L2 header (e.g. the Destination Address if eth0 is an Ethernet); the same error pattern is present also in the MPLS 'pop' operation. Fix this error in act_mpls data plane, computing 'mac_len' as the difference between the network header and the mac header (when not at TC ingress), and use it in MPLS 'push'/'pop' core functions. v2: unbreak 'make htmldocs' because of missing documentation of 'mac_len' in skb_mpls_pop(), reported by kbuild test robot CC: Lorenzo Bianconi <lorenzo@kernel.org> Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC") Reviewed-by: Simon Horman <simon.horman@netronome.com> Acked-by: John Hurley <john.hurley@netronome.com> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- include/linux/skbuff.h | 5 +++-- net/core/skbuff.c | 19 +++++++++++-------- net/openvswitch/actions.c | 5 +++-- net/sched/act_mpls.c | 12 ++++++++---- 4 files changed, 25 insertions(+), 16 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 4351577b14d7..7914fdaf4226 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -3510,8 +3510,9 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len); int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci); int skb_vlan_pop(struct sk_buff *skb); int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci); -int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto); -int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto); +int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto, + int mac_len); +int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto, int mac_len); int skb_mpls_update_lse(struct sk_buff *skb, __be32 mpls_lse); int skb_mpls_dec_ttl(struct sk_buff *skb); struct sk_buff *pskb_extract(struct sk_buff *skb, int off, int to_copy, diff --git a/net/core/skbuff.c b/net/core/skbuff.c index cd59ccd6da57..c81b59177859 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -5477,12 +5477,14 @@ static void skb_mod_eth_type(struct sk_buff *skb, struct ethhdr *hdr, * @skb: buffer * @mpls_lse: MPLS label stack entry to push * @mpls_proto: ethertype of the new MPLS header (expects 0x8847 or 0x8848) + * @mac_len: length of the MAC header * * Expects skb->data at mac header. * * Returns 0 on success, -errno otherwise. */ -int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto) +int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto, + int mac_len) { struct mpls_shim_hdr *lse; int err; @@ -5499,15 +5501,15 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto) return err; if (!skb->inner_protocol) { - skb_set_inner_network_header(skb, skb->mac_len); + skb_set_inner_network_header(skb, mac_len); skb_set_inner_protocol(skb, skb->protocol); } skb_push(skb, MPLS_HLEN); memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb), - skb->mac_len); + mac_len); skb_reset_mac_header(skb); - skb_set_network_header(skb, skb->mac_len); + skb_set_network_header(skb, mac_len); lse = mpls_hdr(skb); lse->label_stack_entry = mpls_lse; @@ -5526,29 +5528,30 @@ EXPORT_SYMBOL_GPL(skb_mpls_push); * * @skb: buffer * @next_proto: ethertype of header after popped MPLS header + * @mac_len: length of the MAC header * * Expects skb->data at mac header. * * Returns 0 on success, -errno otherwise. */ -int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto) +int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto, int mac_len) { int err; if (unlikely(!eth_p_mpls(skb->protocol))) return 0; - err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN); + err = skb_ensure_writable(skb, mac_len + MPLS_HLEN); if (unlikely(err)) return err; skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN); memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb), - skb->mac_len); + mac_len); __skb_pull(skb, MPLS_HLEN); skb_reset_mac_header(skb); - skb_set_network_header(skb, skb->mac_len); + skb_set_network_header(skb, mac_len); if (skb->dev && skb->dev->type == ARPHRD_ETHER) { struct ethhdr *hdr; diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 3572e11b6f21..1c77f520f474 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -165,7 +165,8 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key, { int err; - err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype); + err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype, + skb->mac_len); if (err) return err; @@ -178,7 +179,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key, { int err; - err = skb_mpls_pop(skb, ethertype); + err = skb_mpls_pop(skb, ethertype, skb->mac_len); if (err) return err; diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c index e168df0e008a..4cf6c553bb0b 100644 --- a/net/sched/act_mpls.c +++ b/net/sched/act_mpls.c @@ -55,7 +55,7 @@ static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a, struct tcf_mpls *m = to_mpls(a); struct tcf_mpls_params *p; __be32 new_lse; - int ret; + int ret, mac_len; tcf_lastuse_update(&m->tcf_tm); bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb); @@ -63,8 +63,12 @@ static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a, /* Ensure 'data' points at mac_header prior calling mpls manipulating * functions. */ - if (skb_at_tc_ingress(skb)) + if (skb_at_tc_ingress(skb)) { skb_push_rcsum(skb, skb->mac_len); + mac_len = skb->mac_len; + } else { + mac_len = skb_network_header(skb) - skb_mac_header(skb); + } ret = READ_ONCE(m->tcf_action); @@ -72,12 +76,12 @@ static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a, switch (p->tcfm_action) { case TCA_MPLS_ACT_POP: - if (skb_mpls_pop(skb, p->tcfm_proto)) + if (skb_mpls_pop(skb, p->tcfm_proto, mac_len)) goto drop; break; case TCA_MPLS_ACT_PUSH: new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb->protocol)); - if (skb_mpls_push(skb, new_lse, p->tcfm_proto)) + if (skb_mpls_push(skb, new_lse, p->tcfm_proto, mac_len)) goto drop; break; case TCA_MPLS_ACT_MODIFY: -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action 2019-10-12 11:55 [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Davide Caratti 2019-10-12 11:55 ` [PATCH net v2 1/2] net: avoid errors when trying to pop MLPS header on non-MPLS packets Davide Caratti 2019-10-12 11:55 ` [PATCH net v2 2/2] net/sched: fix corrupted L2 header with MPLS 'push' and 'pop' actions Davide Caratti @ 2019-10-12 12:41 ` Simon Horman 2019-10-16 0:16 ` David Miller 3 siblings, 0 replies; 5+ messages in thread From: Simon Horman @ 2019-10-12 12:41 UTC (permalink / raw) To: Davide Caratti Cc: lkp, davem, john.hurley, kbuild-all, lorenzo, netdev, xiyou.wangcong On Sat, Oct 12, 2019 at 01:55:05PM +0200, Davide Caratti wrote: > this series contains two fixes for TC 'act_mpls', that try to address > two problems that can be observed configuring simple 'push' / 'pop' > operations: > - patch 1/2 avoids dropping non-MPLS packets that pass through the MPLS > 'pop' action. > - patch 2/2 fixes corruption of the L2 header that occurs when 'push' > or 'pop' actions are configured in TC egress path. > > v2: - change commit message in patch 1/2 to better describe that the > patch impacts only TC, thanks to Simon Horman > - fix missing documentation of 'mac_len' in patch 2/2 Thanks for the follow-up, this looks good to me. > Davide Caratti (2): > net: avoid errors when trying to pop MLPS header on non-MPLS packets > net/sched: fix corrupted L2 header with MPLS 'push' and 'pop' actions > > include/linux/skbuff.h | 5 +++-- > net/core/skbuff.c | 21 ++++++++++++--------- > net/openvswitch/actions.c | 5 +++-- > net/sched/act_mpls.c | 12 ++++++++---- > 4 files changed, 26 insertions(+), 17 deletions(-) > > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action 2019-10-12 11:55 [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Davide Caratti ` (2 preceding siblings ...) 2019-10-12 12:41 ` [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Simon Horman @ 2019-10-16 0:16 ` David Miller 3 siblings, 0 replies; 5+ messages in thread From: David Miller @ 2019-10-16 0:16 UTC (permalink / raw) To: dcaratti Cc: lkp, john.hurley, kbuild-all, lorenzo, netdev, xiyou.wangcong, simon.horman From: Davide Caratti <dcaratti@redhat.com> Date: Sat, 12 Oct 2019 13:55:05 +0200 > this series contains two fixes for TC 'act_mpls', that try to address > two problems that can be observed configuring simple 'push' / 'pop' > operations: > - patch 1/2 avoids dropping non-MPLS packets that pass through the MPLS > 'pop' action. > - patch 2/2 fixes corruption of the L2 header that occurs when 'push' > or 'pop' actions are configured in TC egress path. > > v2: - change commit message in patch 1/2 to better describe that the > patch impacts only TC, thanks to Simon Horman > - fix missing documentation of 'mac_len' in patch 2/2 Series applied and queued up for -stable, thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-16 0:16 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-12 11:55 [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Davide Caratti 2019-10-12 11:55 ` [PATCH net v2 1/2] net: avoid errors when trying to pop MLPS header on non-MPLS packets Davide Caratti 2019-10-12 11:55 ` [PATCH net v2 2/2] net/sched: fix corrupted L2 header with MPLS 'push' and 'pop' actions Davide Caratti 2019-10-12 12:41 ` [PATCH net v2 0/2] net/sched: fix wrong behavior of MPLS push/pop action Simon Horman 2019-10-16 0:16 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).