[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <af05ea48-75c3-444c-86cb-ebf5c7bee2ec@mojatatu.com>
Date: Thu, 14 Mar 2024 11:47:15 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: renmingshuai <renmingshuai@...wei.com>, jiri@...nulli.us
Cc: caowangbao@...wei.com, davem@...emloft.net, jhs@...atatu.com,
liaichun@...wei.com, netdev@...r.kernel.org, vladbu@...dia.com,
xiyou.wangcong@...il.com, yanan@...wei.com
Subject: Re: [PATCH] net/sched: Forbid assigning mirred action to a filter
attached to the egress
On 14/03/2024 11:04, renmingshuai wrote:
>>> ---
>>> net/sched/act_mirred.c | 4 +++
>>> .../tc-testing/tc-tests/actions/mirred.json | 32 +++++++++++++++++++
>>> 2 files changed, 36 insertions(+)
>>>
>>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>>> index 5b3814365924..fc96705285fb 100644
>>> --- a/net/sched/act_mirred.c
>>> +++ b/net/sched/act_mirred.c
>>> @@ -120,6 +120,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>>> NL_SET_ERR_MSG_MOD(extack, "Mirred requires attributes to be passed");
>>> return -EINVAL;
>>> }
>>> + if (tp->chain->block->q->parent != TC_H_INGRESS) {
>>> + NL_SET_ERR_MSG_MOD(extack, "Mirred can only be assigned to the filter attached to ingress");
>>
>> Hmm, that is quite restrictive. I'm pretty sure you would break some
>> valid usecases.
> Hmm, that is really quite restrictive. It might be better to Forbid mirred attached to egress filter
> to mirror or redirect packets to the egress.
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index fc96705285fb..ab3841470992 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -152,6 +152,11 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> return -EINVAL;
> }
>
> + if ((tp->chain->block->q->parent != TC_H_INGRESS) && (parm->eaction == TCA_EGRESS_MIRROR || parm->eaction == TCA_EGRESS_REDIR)) {
> + NL_SET_ERR_MSG_MOD(extack, "Mirred assigned to egress filter can only mirror or redirect to ingress");
> + return -EINVAL;
> + }
> +
> switch (parm->eaction) {
> case TCA_EGRESS_MIRROR:
> case TCA_EGRESS_REDIR:
Are you sure this happens to egress mirred to netdevs other than the one
used to attach the filter?
It seems like in your example you are forcing mirred egress to the same
netdev as the filter
Powered by blists - more mailing lists