netdev - Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

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]

Date: Mon, 27 Nov 2023 10:50:48 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Jamal Hadi Salim <hadi@...atatu.com>, Victor Nogueira <victor@...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, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce
 blockcast tc action

On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@...atatu.com wrote:
> >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@...nulli.us> wrote:
> >>
> >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@...atatu.com wrote:
> >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@...nulli.us> wrote:
> >> >>
> >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@...atatu.com wrote:
> >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@...nulli.us> wrote:
> >> >> >>
> >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@...atatu.com wrote:
> >> >> >> >This action takes advantage of the presence of tc block ports set in the
> >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> >> >> >> >broadcast the packet to a block, that is send to all members of the block except
> >> >> >> >the port in which the packet arrived on. However, the user may specify
> >> >> >> >the option "tx_type all", which will send the packet to all members of the
> >> >> >> >block indiscriminately.
> >> >> >> >
> >> >> >> >Example usage:
> >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> >> >> >> >
> >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >> >>
> >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> >> >> >> sure this is aligned netlink-wise as well.
> >> >> >>
> >> >> >>
> >> >> >> >
> >> >> >> >Or if we wish to send to all ports in the block:
> >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >> >> >>
> >> >> >> I read the discussion the the previous version again. I suggested this
> >> >> >> to be part of mirred. Why exactly that was not addressed?
> >> >> >>
> >> >> >
> >> >> >I am the one who pushed back (in that discussion). Actions should be
> >> >> >small and specific. Like i had said in that earlier discussion it was
> >> >> >a mistake to make mirred do both mirror and redirect - they should
> >> >>
> >> >> For mirror and redirect, I agree. For redirect and redirect, does not
> >> >> make much sense. It's just confusing for the user.
> >> >>
> >> >
> >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> >> >any sense because once you redirect the packet is gone.
> >>
> >> How is it mirror? It is redirect to multiple, isn't it?
> >>
> >>
> >> >
> >> >> >have been two actions. So i feel like adding a block to mirred is
> >> >> >adding more knobs. We are also going to add dev->group as a way to
> >> >> >select what devices to mirror to. Should that be in mirred as well?
> >> >>
> >> >> I need more details.
> >> >>
> >> >
> >> >You set any port you want to be mirrored to using ip link, example:
> >> >ip link set dev $DEV1 group 2
> >> >ip link set dev $DEV2 group 2
> >>
> >> That does not looks correct at all. Do tc stuff in tc, no?
> >>
> >>
> >> >...
> >> >
> >> >Then you can blockcast:
> >> >tc filter add devx protocol ip pref 25 \
> >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
> >>
> >> "blockcasting" to something that is not a block anymore. Not nice.
> >>
> >
> >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
> >perhaps a different action altogether.
>
> mirret redirect? :)
>
> With target of:
> 1) dev (the current one)
> 2) block
> 3) group
> ?

tbh, I dont like it - but we need to make progress. I will defer to Marcelo.

cheers,
jamal

>
> >
> >cheers,
> >jamal
> >> >
> >> >cheers,
> >> >jamal
> >> >
> >> >>
> >> >> >
> >> >> >cheers,
> >> >> >jamal
> >> >> >
> >> >> >> Instead of:
> >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >> >> You'd have:
> >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >> >> >>
> >> >> >> I don't see why we need special action for this.
> >> >> >>
> >> >> >> Regarding "tx_type all":
> >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> >> >> >> to have this as "no_src_skip" or some other similar arg, without value
> >> >> >> acting as a bool (flag) on netlink level.
> >> >> >>
> >> >> >>