lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1d08f20e-a363-4405-ad97-1107cd34628a@mojatatu.com>
Date: Fri, 15 Dec 2023 10:56:48 -0300
From: Victor Nogueira <victor@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: jhs@...atatu.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, xiyou.wangcong@...il.com,
 mleitner@...hat.com, vladbu@...dia.com, paulb@...dia.com,
 pctammela@...atatu.com, netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next v7 3/3] net/sched: act_mirred: Allow mirred to
 block

On 15/12/2023 10:06, Jiri Pirko wrote:
> Fri, Dec 15, 2023 at 12:10:50PM CET, victor@...atatu.com wrote:
>> So far the mirred action has dealt with syntax that handles mirror/redirection for netdev.
>> A matching packet is redirected or mirrored to a target netdev.
>>
>> In this patch we enable mirred to mirror to a tc block as well.
>> IOW, the new syntax looks as follows:
>> ... mirred <ingress | egress> <mirror | redirect> [index INDEX] < <blockid BLOCKID> | <dev <devname>> >
>>
>> Examples of mirroring or redirecting to a tc block:
>> $ tc filter add block 22 protocol ip pref 25 \
>>   flower dst_ip 192.168.0.0/16 action mirred egress mirror blockid 22
>>
>> $ tc filter add block 22 protocol ip pref 25 \
>>   flower dst_ip 10.10.10.10/32 action mirred egress redirect blockid 22
>>
>> Co-developed-by: Jamal Hadi Salim <jhs@...atatu.com>
>> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
>> Co-developed-by: Pedro Tammela <pctammela@...atatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
>> Signed-off-by: Victor Nogueira <victor@...atatu.com>
>> ---
>> include/net/tc_act/tc_mirred.h        |   1 +
>> include/uapi/linux/tc_act/tc_mirred.h |   1 +
>> net/sched/act_mirred.c                | 278 +++++++++++++++++++-------
>> 3 files changed, 206 insertions(+), 74 deletions(-)
>>
>> diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>> index 32ce8ea36950..75722d967bf2 100644
>> --- a/include/net/tc_act/tc_mirred.h
>> +++ b/include/net/tc_act/tc_mirred.h
>> @@ -8,6 +8,7 @@
>> struct tcf_mirred {
>> 	struct tc_action	common;
>> 	int			tcfm_eaction;
>> +	u32                     tcfm_blockid;
>> 	bool			tcfm_mac_header_xmit;
>> 	struct net_device __rcu	*tcfm_dev;
>> 	netdevice_tracker	tcfm_dev_tracker;
>> diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
>> index 2500a0005d05..54df06658bc8 100644
>> --- a/include/uapi/linux/tc_act/tc_mirred.h
>> +++ b/include/uapi/linux/tc_act/tc_mirred.h
>> @@ -20,6 +20,7 @@ enum {
>> 	TCA_MIRRED_UNSPEC,
>> 	TCA_MIRRED_TM,
>> 	TCA_MIRRED_PARMS,
>> +	TCA_MIRRED_BLOCKID,
> 
> You just broke uapi. Make sure to add new attributes to the end.

My bad, don't know how we missed this one.
Will fix in v8.

> 
>> 	TCA_MIRRED_PAD,
>> 	__TCA_MIRRED_MAX
>> };
>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>> index 0a711c184c29..8b6d04d26c5a 100644
>> --- a/net/sched/act_mirred.c
>> +++ b/net/sched/act_mirred.c
>> @@ -85,10 +85,20 @@ static void tcf_mirred_release(struct tc_action *a)
>>
>> static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
>> 	[TCA_MIRRED_PARMS]	= { .len = sizeof(struct tc_mirred) },
>> +	[TCA_MIRRED_BLOCKID]	= { .type = NLA_U32 },
>> };
>>
>> static struct tc_action_ops act_mirred_ops;
>>
>> +static void tcf_mirred_replace_dev(struct tcf_mirred *m, struct net_device *ndev)
>> +{
>> +	struct net_device *odev;
>> +
>> +	odev = rcu_replace_pointer(m->tcfm_dev, ndev,
>> +				   lockdep_is_held(&m->tcf_lock));
>> +	netdev_put(odev, &m->tcfm_dev_tracker);
>> +}
>> +
>> static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>> 			   struct nlattr *est, struct tc_action **a,
>> 			   struct tcf_proto *tp,
>> @@ -126,6 +136,13 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>> 	if (exists && bind)
>> 		return 0;
>>
>> +	if (tb[TCA_MIRRED_BLOCKID] && parm->ifindex) {
>> +		NL_SET_ERR_MSG_MOD(extack,
>> +				   "Mustn't specify Block ID and dev simultaneously");
>> +		err = -EINVAL;
>> +		goto release_idr;
>> +	}
>> +
>> 	switch (parm->eaction) {
>> 	case TCA_EGRESS_MIRROR:
>> 	case TCA_EGRESS_REDIR:
>> @@ -142,9 +159,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>> 	}
>>
>> 	if (!exists) {
>> -		if (!parm->ifindex) {
>> +		if (!parm->ifindex && !tb[TCA_MIRRED_BLOCKID]) {
>> 			tcf_idr_cleanup(tn, index);
>> -			NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
>> +			NL_SET_ERR_MSG_MOD(extack, "Must specify device or block");
>> 			return -EINVAL;
>> 		}
>> 		ret = tcf_idr_create_from_flags(tn, index, est, a,
>> @@ -170,7 +187,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>> 	spin_lock_bh(&m->tcf_lock);
>>
>> 	if (parm->ifindex) {
>> -		struct net_device *odev, *ndev;
>> +		struct net_device *ndev;
>>
>> 		ndev = dev_get_by_index(net, parm->ifindex);
>> 		if (!ndev) {
>> @@ -179,11 +196,14 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>> 			goto put_chain;
>> 		}
>> 		mac_header_xmit = dev_is_mac_header_xmit(ndev);
>> -		odev = rcu_replace_pointer(m->tcfm_dev, ndev,
>> -					  lockdep_is_held(&m->tcf_lock));
>> -		netdev_put(odev, &m->tcfm_dev_tracker);
>> +		tcf_mirred_replace_dev(m, ndev);
> 
> This could be a separate patch, for better readability of the patches.
> 
> Skimming thought the rest of the patch, this is hard to follow (-ETOOBIG).
> What would help is to cut this patch into multiple ones. Do preparations
> first, then you finally add TCA_MIRRED_BLOCKID processin and blockid
> forwarding. Could you?

Will transform this one into two separate patches.

> 
>> 		netdev_tracker_alloc(ndev, &m->tcfm_dev_tracker, GFP_ATOMIC);
>> 		m->tcfm_mac_header_xmit = mac_header_xmit;
>> +		m->tcfm_blockid = 0;
>> +	} else if (tb[TCA_MIRRED_BLOCKID]) {
>> +		tcf_mirred_replace_dev(m, NULL);
>> +		m->tcfm_mac_header_xmit = false;
>> +		m->tcfm_blockid = nla_get_u32(tb[TCA_MIRRED_BLOCKID]);
>> 	}
>> 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>> 	m->tcfm_eaction = parm->eaction;
> 
> [...]

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ