diff mbox series

[net-next] net/sched: act_mirred: Ensure mac_len is pulled prior redirect to a non mac_header_xmit target device

Message ID 20191223123336.13066-1-sladkani@proofpoint.com
State Superseded
Delegated to: David Miller
Headers show
Series [net-next] net/sched: act_mirred: Ensure mac_len is pulled prior redirect to a non mac_header_xmit target device | expand

Commit Message

Shmulik Ladkani Dec. 23, 2019, 12:33 p.m. UTC 
From: Shmulik Ladkani <sladkani@proofpoint.com>

There's no skb_pull performed when a mirred action is set at egress of a
mac device, with a target device/action that expects skb->data to point
at the network header.

As a result, either the target device is errornously given an skb with
data pointing to the mac (egress case), or the net stack receives the
skb with data pointing to the mac (ingress case).

E.g:
 # tc qdisc add dev eth9 root handle 1: prio
 # tc filter add dev eth9 parent 1: prio 9 protocol ip handle 9 basic \
   action mirred egress redirect dev tun0

 (tun0 is a tun device. result: tun0 errornously gets the eth header
  instead of the iph)

Revise the push/pull logic of tcf_mirred_act() to not rely on the
skb_at_tc_ingress() vs tcf_mirred_act_wants_ingress() comparison, as it
does not cover all "pull" cases.

Instead, calculate whether the required action on the target device
requires the data to point at the network header, and compare this to
whether skb->data points to network header - and make the push/pull
adjustments as necessary.

Signed-off-by: Shmulik Ladkani <sladkani@proofpoint.com>
---
 net/sched/act_mirred.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Jamal Hadi Salim Dec. 24, 2019, 12:45 p.m. UTC | #1 
On 2019-12-23 7:33 a.m., shmulik@metanetworks.com wrote:
> From: Shmulik Ladkani <sladkani@proofpoint.com>
> 
> There's no skb_pull performed when a mirred action is set at egress of a
> mac device, with a target device/action that expects skb->data to point
> at the network header.
> 
> As a result, either the target device is errornously given an skb with
> data pointing to the mac (egress case), or the net stack receives the
> skb with data pointing to the mac (ingress case).
> 
> E.g:
>   # tc qdisc add dev eth9 root handle 1: prio
>   # tc filter add dev eth9 parent 1: prio 9 protocol ip handle 9 basic \
>     action mirred egress redirect dev tun0
> 
>   (tun0 is a tun device. result: tun0 errornously gets the eth header
>    instead of the iph)
> 

Shmulik - are you able to add a tdc testcase? It will be helpful to
catch future regressions.
I did basic testing i.e have an app reading tun0 from user space
then mirrored(not redirect) packets to tun0 for before and after
patch.

> Revise the push/pull logic of tcf_mirred_act() to not rely on the
> skb_at_tc_ingress() vs tcf_mirred_act_wants_ingress() comparison, as it
> does not cover all "pull" cases.
> 
> Instead, calculate whether the required action on the target device
> requires the data to point at the network header, and compare this to
> whether skb->data points to network header - and make the push/pull
> adjustments as necessary.
> 
> Signed-off-by: Shmulik Ladkani <sladkani@proofpoint.com>

Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
Cong Wang Dec. 24, 2019, 8:43 p.m. UTC | #2 
On Mon, Dec 23, 2019 at 4:33 AM <shmulik@metanetworks.com> wrote:
>
> From: Shmulik Ladkani <sladkani@proofpoint.com>
>
> There's no skb_pull performed when a mirred action is set at egress of a
> mac device, with a target device/action that expects skb->data to point
> at the network header.
>
> As a result, either the target device is errornously given an skb with
> data pointing to the mac (egress case), or the net stack receives the
> skb with data pointing to the mac (ingress case).
>
> E.g:
>  # tc qdisc add dev eth9 root handle 1: prio
>  # tc filter add dev eth9 parent 1: prio 9 protocol ip handle 9 basic \
>    action mirred egress redirect dev tun0
>
>  (tun0 is a tun device. result: tun0 errornously gets the eth header
>   instead of the iph)
>
> Revise the push/pull logic of tcf_mirred_act() to not rely on the
> skb_at_tc_ingress() vs tcf_mirred_act_wants_ingress() comparison, as it
> does not cover all "pull" cases.
>
> Instead, calculate whether the required action on the target device
> requires the data to point at the network header, and compare this to
> whether skb->data points to network header - and make the push/pull
> adjustments as necessary.

This is a bug fix, please target it for -net and add a proper Fixes tag.

BTW, please make the subject shorter.

Thanks.
Shmulik Ladkani Dec. 25, 2019, 8:27 a.m. UTC | #3 
On Tue, 24 Dec 2019 07:45:08 -0500
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> Shmulik - are you able to add a tdc testcase? It will be helpful to
> catch future regressions.

Sure, I'll try to cook something, different submission.
Shmulik Ladkani Dec. 25, 2019, 8:29 a.m. UTC | #4 
On Tue, 24 Dec 2019 12:43:32 -0800
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> This is a bug fix, please target it for -net and add a proper Fixes tag.
> 
> BTW, please make the subject shorter.

Thanks. Will resubmit to -net
diff mbox series

Patch

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 1e3eb3a97532..1ad300e6dbc0 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -219,8 +219,10 @@  static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	bool use_reinsert;
 	bool want_ingress;
 	bool is_redirect;
+	bool expects_nh;
 	int m_eaction;
 	int mac_len;
+	bool at_nh;
 
 	rec_level = __this_cpu_inc_return(mirred_rec_level);
 	if (unlikely(rec_level > MIRRED_RECURSION_LIMIT)) {
@@ -261,19 +263,19 @@  static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 			goto out;
 	}
 
-	/* If action's target direction differs than filter's direction,
-	 * and devices expect a mac header on xmit, then mac push/pull is
-	 * needed.
-	 */
 	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
-	if (skb_at_tc_ingress(skb) != want_ingress && m_mac_header_xmit) {
-		if (!skb_at_tc_ingress(skb)) {
-			/* caught at egress, act ingress: pull mac */
-			mac_len = skb_network_header(skb) - skb_mac_header(skb);
+
+	expects_nh = want_ingress || !m_mac_header_xmit;
+	at_nh = skb->data == skb_network_header(skb);
+	if (at_nh != expects_nh) {
+		mac_len = skb_at_tc_ingress(skb) ? skb->mac_len :
+			  skb_network_header(skb) - skb_mac_header(skb);
+		if (expects_nh) {
+			/* target device/action expect data at nh */
 			skb_pull_rcsum(skb2, mac_len);
 		} else {
-			/* caught at ingress, act egress: push mac */
-			skb_push_rcsum(skb2, skb->mac_len);
+			/* target device/action expect data at mac */
+			skb_push_rcsum(skb2, mac_len);
 		}
 	}