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

Jamal Hadi Salim
2016-07-02T14:02:33Z

On 16-07-02 09:49 AM, Nikolay Aleksandrov wrote:
> On 02/07/16 15:26, Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Often redirecting or mirroring requires that we set the MAC address
>> of the target device. While it is possible to pipe to a pedit action
>> this obsoletes the need for that. This is 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 | 20 +++++++++++++++++++-
>> 3 files changed, 26 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,
>
> Hi Jamal,
> I think you should update "mirred_policy" in order to ensure that the attribute has
> the minimum length for a mac address.

Good point. Will do in the next update.

> Also a minor suggestion - maybe err out on a
> zero mac address, otherwise the user might think the operation was successful.
>

Is a zero mac address wrong? What if that was policy intent?

cheers,
jamal