[PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Often redirecting or mirroring requires that we set the dstMAC address
of the target device. While it is possible to pipe to a pedit action,
this patch obsoletes the need for that. This is a justified feature because
the dst MAC addresses rewrite is such a common use case.

Sample usage:
sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15

This will match all icmp packets going out on dev $ETH and
redirect them to dev $SPANPORT while setting their dst MAC address
to 02:15:15:15:15:15

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/tc_act/tc_mirred.h        |  4 +++-
 include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
 net/sched/act_mirred.c                | 24 +++++++++++++++++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index e891835..7e8bced 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -6,10 +6,12 @@
 
 struct tcf_mirred {
 	struct tcf_common	common;
+	struct net_device __rcu	*tcfm_dev;
 	int			tcfm_eaction;
 	int			tcfm_ifindex;
 	int			tcfm_ok_push;
-	struct net_device __rcu	*tcfm_dev;
+	u8			eth_dst[ETH_ALEN];
+	/* XXX 6 bytes hole here*/
 	struct list_head	tcfm_list;
 };
 #define to_mirred(a) \
diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
index 3d7a2b3..aaca1ff 100644
--- a/include/uapi/linux/tc_act/tc_mirred.h
+++ b/include/uapi/linux/tc_act/tc_mirred.h
@@ -9,20 +9,21 @@
 #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
 #define TCA_INGRESS_REDIR 3  /* packet redirect to INGRESS*/
 #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
-                                                                                
+
 struct tc_mirred {
 	tc_gen;
 	int                     eaction;   /* one of IN/EGRESS_MIRROR/REDIR */
 	__u32                   ifindex;  /* ifindex of egress port */
 };
-                                                                                
+
 enum {
 	TCA_MIRRED_UNSPEC,
 	TCA_MIRRED_TM,
 	TCA_MIRRED_PARMS,
 	TCA_MIRRED_PAD,
+	TCA_MIRRED_DMAC,
 	__TCA_MIRRED_MAX
 };
 #define TCA_MIRRED_MAX (__TCA_MIRRED_MAX - 1)
-                                                                                
+
 #endif
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 5b135d3..1cc14f5 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -49,6 +49,7 @@ static void tcf_mirred_release(struct tc_action *a, int bind)
 
 static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
 	[TCA_MIRRED_PARMS]	= { .len = sizeof(struct tc_mirred) },
+	[TCA_MIRRED_DMAC] = { .len = ETH_ALEN},
 };
 
 static int mirred_net_id;
@@ -64,15 +65,24 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	struct net_device *dev;
 	int ret, ok_push = 0;
 	bool exists = false;
+	u8 *daddr = NULL;
 
 	if (nla == NULL)
 		return -EINVAL;
+
 	ret = nla_parse_nested(tb, TCA_MIRRED_MAX, nla, mirred_policy);
 	if (ret < 0)
 		return ret;
+
 	if (tb[TCA_MIRRED_PARMS] == NULL)
 		return -EINVAL;
+
 	parm = nla_data(tb[TCA_MIRRED_PARMS]);
+	if (tb[TCA_MIRRED_DMAC]) {
+		daddr = nla_data(tb[TCA_MIRRED_DMAC]);
+		if (is_zero_ether_addr(daddr))
+			return -EINVAL;
+	}
 
 	exists = tcf_hash_check(tn, parm->index, a, bind);
 	if (exists && bind)
@@ -126,6 +136,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	}
 	m = to_mirred(a);
 
+	if (daddr)
+		ether_addr_copy(m->eth_dst, daddr);
+
 	ASSERT_RTNL();
 	m->tcf_action = parm->action;
 	m->tcfm_eaction = parm->eaction;
@@ -190,6 +203,9 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 
 	skb2->skb_iif = skb->dev->ifindex;
 	skb2->dev = dev;
+	if (!is_zero_ether_addr(m->eth_dst))
+		ether_addr_copy(eth_hdr(skb2)->h_dest, m->eth_dst);
+
 	err = dev_queue_xmit(skb2);
 
 	if (err) {
@@ -203,7 +219,8 @@ out:
 	return retval;
 }
 
-static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
+			   int ref)
 {
 	unsigned char *b = skb_tail_pointer(skb);
 	struct tcf_mirred *m = a->priv;
@@ -220,6 +237,11 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, i
 	if (nla_put(skb, TCA_MIRRED_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
 
+	if (!is_zero_ether_addr(m->eth_dst)) {
+		if (nla_put(skb, TCA_MIRRED_DMAC, ETH_ALEN, m->eth_dst))
+			goto nla_put_failure;
+	}
+
 	tcf_tm_dump(&t, &m->tcf_tm);
 	if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
 		goto nla_put_failure;
-- 
1.9.1

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Nikolay Aleksandrov]

On 02/07/16 16:34, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Often redirecting or mirroring requires that we set the dstMAC address
> of the target device. While it is possible to pipe to a pedit action,
> this patch obsoletes the need for that. This is a justified feature because
> the dst MAC addresses rewrite is such a common use case.
> 
> Sample usage:
> sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
> u32 match ip protocol 1 0xff flowid 1:2 \
> action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
> 
> This will match all icmp packets going out on dev $ETH and
> redirect them to dev $SPANPORT while setting their dst MAC address
> to 02:15:15:15:15:15
> 
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  include/net/tc_act/tc_mirred.h        |  4 +++-
>  include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
>  net/sched/act_mirred.c                | 24 +++++++++++++++++++++++-
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 

LGTM, thanks!

Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Cong Wang]

On Sat, Jul 2, 2016 at 7:34 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Often redirecting or mirroring requires that we set the dstMAC address
> of the target device. While it is possible to pipe to a pedit action,
> this patch obsoletes the need for that. This is a justified feature because
> the dst MAC addresses rewrite is such a common use case.

Maybe, instead of allowing to set arbitrary MAC address, how about
just allow setting to the MAC address of the target device?
If so we only need a boolean flag for user-space.

IOW, do we really need such flexibility?

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Jamal Hadi Salim]

On 16-07-02 12:58 PM, Cong Wang wrote:
> On Sat, Jul 2, 2016 at 7:34 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Often redirecting or mirroring requires that we set the dstMAC address
>> of the target device. While it is possible to pipe to a pedit action,
>> this patch obsoletes the need for that. This is a justified feature because
>> the dst MAC addresses rewrite is such a common use case.
>
> Maybe, instead of allowing to set arbitrary MAC address, how about
> just allow setting to the MAC address of the target device?

You cant discover the remote MAC address; I have a wire
connected between two machines. I set it to the MAC address
of the remote machine i know of. And there are times I really
want to set it to some arbitrary values (although that is more
test mode than production).

cheers,
jamal

> If so we only need a boolean flag for user-space.
>
> IOW, do we really need such flexibility?
>

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Jiri Pirko]

Sat, Jul 02, 2016 at 04:34:25PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>Often redirecting or mirroring requires that we set the dstMAC address
>of the target device. While it is possible to pipe to a pedit action,
>this patch obsoletes the need for that. This is a justified feature because
>the dst MAC addresses rewrite is such a common use case.

We can't we have set of actions each doing its own job? I don't like
merging functionality into actions :(


>
>Sample usage:
>sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
>u32 match ip protocol 1 0xff flowid 1:2 \
>action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
>
>This will match all icmp packets going out on dev $ETH and
>redirect them to dev $SPANPORT while setting their dst MAC address
>to 02:15:15:15:15:15
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/net/tc_act/tc_mirred.h        |  4 +++-
> include/uapi/linux/tc_act/tc_mirred.h |  7 ++++---
> net/sched/act_mirred.c                | 24 +++++++++++++++++++++++-
> 3 files changed, 30 insertions(+), 5 deletions(-)
>
>diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>index e891835..7e8bced 100644
>--- a/include/net/tc_act/tc_mirred.h
>+++ b/include/net/tc_act/tc_mirred.h
>@@ -6,10 +6,12 @@
> 
> struct tcf_mirred {
> 	struct tcf_common	common;
>+	struct net_device __rcu	*tcfm_dev;
> 	int			tcfm_eaction;
> 	int			tcfm_ifindex;
> 	int			tcfm_ok_push;
>-	struct net_device __rcu	*tcfm_dev;
>+	u8			eth_dst[ETH_ALEN];
>+	/* XXX 6 bytes hole here*/
> 	struct list_head	tcfm_list;
> };
> #define to_mirred(a) \
>diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
>index 3d7a2b3..aaca1ff 100644
>--- a/include/uapi/linux/tc_act/tc_mirred.h
>+++ b/include/uapi/linux/tc_act/tc_mirred.h
>@@ -9,20 +9,21 @@
> #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
> #define TCA_INGRESS_REDIR 3  /* packet redirect to INGRESS*/
> #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
>-                                                                                
>+
> struct tc_mirred {
> 	tc_gen;
> 	int                     eaction;   /* one of IN/EGRESS_MIRROR/REDIR */
> 	__u32                   ifindex;  /* ifindex of egress port */
> };
>-                                                                                
>+
> enum {
> 	TCA_MIRRED_UNSPEC,
> 	TCA_MIRRED_TM,
> 	TCA_MIRRED_PARMS,
> 	TCA_MIRRED_PAD,
>+	TCA_MIRRED_DMAC,
> 	__TCA_MIRRED_MAX
> };
> #define TCA_MIRRED_MAX (__TCA_MIRRED_MAX - 1)
>-                                                                                
>+
> #endif
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 5b135d3..1cc14f5 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -49,6 +49,7 @@ static void tcf_mirred_release(struct tc_action *a, int bind)
> 
> static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
> 	[TCA_MIRRED_PARMS]	= { .len = sizeof(struct tc_mirred) },
>+	[TCA_MIRRED_DMAC] = { .len = ETH_ALEN},
> };
> 
> static int mirred_net_id;
>@@ -64,15 +65,24 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 	struct net_device *dev;
> 	int ret, ok_push = 0;
> 	bool exists = false;
>+	u8 *daddr = NULL;
> 
> 	if (nla == NULL)
> 		return -EINVAL;
>+
> 	ret = nla_parse_nested(tb, TCA_MIRRED_MAX, nla, mirred_policy);
> 	if (ret < 0)
> 		return ret;
>+
> 	if (tb[TCA_MIRRED_PARMS] == NULL)
> 		return -EINVAL;
>+
> 	parm = nla_data(tb[TCA_MIRRED_PARMS]);
>+	if (tb[TCA_MIRRED_DMAC]) {
>+		daddr = nla_data(tb[TCA_MIRRED_DMAC]);
>+		if (is_zero_ether_addr(daddr))
>+			return -EINVAL;
>+	}
> 
> 	exists = tcf_hash_check(tn, parm->index, a, bind);
> 	if (exists && bind)
>@@ -126,6 +136,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 	}
> 	m = to_mirred(a);
> 
>+	if (daddr)
>+		ether_addr_copy(m->eth_dst, daddr);
>+
> 	ASSERT_RTNL();
> 	m->tcf_action = parm->action;
> 	m->tcfm_eaction = parm->eaction;
>@@ -190,6 +203,9 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
> 
> 	skb2->skb_iif = skb->dev->ifindex;
> 	skb2->dev = dev;
>+	if (!is_zero_ether_addr(m->eth_dst))
>+		ether_addr_copy(eth_hdr(skb2)->h_dest, m->eth_dst);
>+
> 	err = dev_queue_xmit(skb2);
> 
> 	if (err) {
>@@ -203,7 +219,8 @@ out:
> 	return retval;
> }
> 
>-static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>+static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
>+			   int ref)
> {
> 	unsigned char *b = skb_tail_pointer(skb);
> 	struct tcf_mirred *m = a->priv;
>@@ -220,6 +237,11 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, i
> 	if (nla_put(skb, TCA_MIRRED_PARMS, sizeof(opt), &opt))
> 		goto nla_put_failure;
> 
>+	if (!is_zero_ether_addr(m->eth_dst)) {
>+		if (nla_put(skb, TCA_MIRRED_DMAC, ETH_ALEN, m->eth_dst))
>+			goto nla_put_failure;
>+	}
>+
> 	tcf_tm_dump(&t, &m->tcf_tm);
> 	if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
> 		goto nla_put_failure;
>-- 
>1.9.1
>

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sat,  2 Jul 2016 10:34:25 -0400

> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Often redirecting or mirroring requires that we set the dstMAC address
> of the target device. While it is possible to pipe to a pedit action,
> this patch obsoletes the need for that. This is a justified feature because
> the dst MAC addresses rewrite is such a common use case.
> 
> Sample usage:
> sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
> u32 match ip protocol 1 0xff flowid 1:2 \
> action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
> 
> This will match all icmp packets going out on dev $ETH and
> redirect them to dev $SPANPORT while setting their dst MAC address
> to 02:15:15:15:15:15
> 
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

I agree with Jiri.

The whole model is to chain together the actions that you need to
accomplish whatever it is you want to do.

This means actions are as specialized as possible, and do one thing
as precisely and efficiently as possible.

It seems unnecessary to break this model and start to make swiss army
knife actions.

Every time we find some "common use case" will we turn yet another
action into a Frankenstein thing that does more than it was designed
to do?

I don't want to apply this and start us down this road, seriously...

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Jamal Hadi Salim]

On 16-07-04 06:14 PM, David Miller wrote:

> I agree with Jiri.
>
> The whole model is to chain together the actions that you need to
> accomplish whatever it is you want to do.
>
> This means actions are as specialized as possible, and do one thing
> as precisely and efficiently as possible.
>
> It seems unnecessary to break this model and start to make swiss army
> knife actions.
>
> Every time we find some "common use case" will we turn yet another
> action into a Frankenstein thing that does more than it was designed
> to do?
>

I agree there is grey line and i too subscribe to the faith of
small-actions granularity. But it is not exact dogma guys.
Even grep ends up adding features over time;->
I am/was on the fence on submitting this patch. Here's the
thought process that convinced my fingers to type the patch:

When you have a switch between you and the destination target (in my
case several that i have no control over) and when said switches
learn the MAC addresses, then there is confusion when they start
seeing the same MAC address showing up in conflicting ports.

Second arguement  usability, from:

sudo $TC filter add dev $ETH parent ffff: pref 11 protocol ip u32 \
match ip protocol 1 0xff flowid 1:2 \
action pedit munge offset -14 u8 set 0x02 \
         munge offset -13 u8 set 0x15 \
         munge offset -12 u8 set 0x15 \
         munge offset -11 u8 set 0x15 \
         munge offset -10 u16 set 0x1515 \
         pipe \
action mirred egress redirect dev $SPANPORT

to:
$TC filter add dev $ETH parent 1: protocol ip prio 10 \
     u32 match ip protocol 1 0xff flowid 1:2 \
     action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15

given that I have to do this many many times in scripts and the
second policy is better eye candy.

And last but not least: there is a small performance improvement
because i have to execute less instructions.

Choices are ugly and slow vs fast and nicer;->

Of course what i am doing makes it more complex to offload with
existing ASICs. But it is an optional feature so it could be
worked around; we need to start dealing with capabilities and
reject offloads for certain policies.

Alternative: introduce another action, I'll call it skbmod
(like skbedit) and anytime someone finds something they are
prototyping with pedit is ugly they could migrate the modifier
into skbmod. First user dstmac.
So my policy would be:

$TC filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action skbmod dstmac 02:15:15:15:15:15 \
action mirred egress redirect dev $SPANPORT

> I don't want to apply this and start us down this road, seriously...

That cat is out of the bag, at least for tc bpf. An opaque blob
of a single action could do whatever it wants.

cheers,
jamal

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Cong Wang]

On Tue, Jul 5, 2016 at 4:04 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Second arguement  usability, from:
>
> sudo $TC filter add dev $ETH parent ffff: pref 11 protocol ip u32 \
> match ip protocol 1 0xff flowid 1:2 \
> action pedit munge offset -14 u8 set 0x02 \
>         munge offset -13 u8 set 0x15 \
>         munge offset -12 u8 set 0x15 \
>         munge offset -11 u8 set 0x15 \
>         munge offset -10 u16 set 0x1515 \
>         pipe \
> action mirred egress redirect dev $SPANPORT
>
> to:
> $TC filter add dev $ETH parent 1: protocol ip prio 10 \
>     u32 match ip protocol 1 0xff flowid 1:2 \
>     action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
>
> given that I have to do this many many times in scripts and the
> second policy is better eye candy.

How about adding a "wrapper" in iproute2 pedit action to make
it accept "dst mac xx:xx:xx:xx:xx:xx"? Like what we did for u32
filters.

Re: [PATCH v2 net-next 1/1] net sched actions: mirred add support for setting Dst MAC address

[Jamal Hadi Salim]

On 16-07-05 02:19 PM, Cong Wang wrote:
> On Tue, Jul 5, 2016 at 4:04 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> Second arguement  usability, from:

>> to:
>> $TC filter add dev $ETH parent 1: protocol ip prio 10 \
>>      u32 match ip protocol 1 0xff flowid 1:2 \
>>      action mirred egress redirect dev $SPANPORT dst 02:15:15:15:15:15
>>
>> given that I have to do this many many times in scripts and the
>> second policy is better eye candy.
>
> How about adding a "wrapper" in iproute2 pedit action to make
> it accept "dst mac xx:xx:xx:xx:xx:xx"? Like what we did for u32
> filters.
>

Thats what we do today. Its fugly and debugging is nasty
(having to stare at 10s or hundreds of outputs looking at
offset and bytes when something goes wrong).
It is ok to use pedit as a starting point because you can
quickly craft things without any kernel changes.
But when you start heavily using something handcrafted
its usability starts affecting you.
I will work on and send the skbmod patch next time i
get cycles.

cheers,
jamal