netdev - Re: [PATCH net-next v4 0/3] net/sched: Introduce tc block ports tracking and use

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: <ZSAEp+tr1oXHOy/C@nanopsycho>
Date: Fri, 6 Oct 2023 14:59:19 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Victor Nogueira <victor@...atatu.com>
Cc: jhs@...atatu.com, xiyou.wangcong@...il.com, davem@...emloft.net,
	pabeni@...hat.com, edumazet@...gle.com, kuba@...nel.org,
	mleitner@...hat.com, vladbu@...dia.com, simon.horman@...igine.com,
	pctammela@...atatu.com, netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next v4 0/3] net/sched: Introduce tc block ports
 tracking and use

Thu, Oct 05, 2023 at 08:42:25PM CEST, victor@...atatu.com wrote:
>__Context__
>The "tc block" is a collection of netdevs/ports which allow qdiscs to share
>match-action block instances (as opposed to the traditional tc filter per
>netdev/port)[1].
>
>Example setup:
>$ tc qdisc add dev ens7 ingress block 22
>$ tc qdisc add dev ens8 ingress block 22
>
>Once the block is created we can add a filter using the block index:
>$ tc filter add block 22 protocol ip pref 25 \
>  flower dst_ip 192.168.0.0/16 action drop
>
>A packet with dst IP matching 192.168.0.0/16 arriving on the ingress of
>either ens7 or ens8 is dropped.
>
>__This patchset__
>Up to this point in the implementation, the block is unaware of its ports.
>This patch fixes that and makes the tc block ports available to the

Odd. You fix a bug. Is there a bug? If yes, you need to describe it. If
no, don't use "fix".


>datapath.
>
>For the datapath we provide a use case of the tc block in an action
>we call "blockcast" in patch 3. This action can be used in an example as
>such:
>
>$ tc qdisc add dev ens7 ingress block 22
>$ tc qdisc add dev ens8 ingress block 22
>$ tc qdisc add dev ens9 ingress block 22
>$ tc filter add block 22 protocol ip pref 25 \
>  flower dst_ip 192.168.0.0/16 action blockcast

Seems to me a bit odd that the action works with the entity (block) is
is connected to. I would expect rather to give the action configuration:

$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast block 22
                                                ^^^^^^^^

Then this is more flexible and allows user to use this action for any
packet, no matter from where it was received.

Looks like this is functionality-wise similar to mirred redirect. Why
can't we have that action extended to accept block number instead of
netdev and have something like:

$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22

This would be very much alike we do either "tc filter add dev X" or "tc
filter add block Y".

Regarding the filtering, that could be a simple flag config of mirred
action:

$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
  srcfilter

Or something like that.

Makes sense?



>
>When a packet(matching dst IP 192.168.0.0/16) arrives on the ingress of any
>of ens7, ens8 or ens9 it will be copied to all ports other than itself.
>For example, if it arrives on ens8 then a copy of the packet will be
>"blockcasted";-> to both ens7 and ens9 (unmodified), but not to ens8.
>
>Patch 1 introduces the required infra. Patch 2 exposes the tc block to the
>tc datapath and patch 3 implements datapath usage via a new tc action
>"blockcast".
>
>__Acknowledgements__
>Suggestions from Vlad Buslov and Marcelo Ricardo Leitner made this patchset
>better. The idea of integrating the ports into the tc block was suggested
>by Jiri Pirko.
>
>[1] See commit ca46abd6f89f ("Merge branch 'net-sched-allow-qdiscs-to-share-filter-block-instances'")
>
>Changes in v2:
>  - Remove RFC tag
>  - Add more details in patch 0(Jiri)
>  - When CONFIG_NET_TC_SKB_EXT is selected we have unused qdisc_cb
>    Reported-by: kernel test robot <lkp@...el.com> (and horms@...nel.org)
>  - Fix bad dev dereference in printk of blockcast action (Simon)
>
>Changes in v3:
>  - Add missing xa_destroy (pointed out by Vlad)
>  - Remove bugfix pointed by Vlad (will send in separate patch)
>  - Removed ports from subject in patch #2 and typos (suggested by Marcelo)
>  - Remove net_notice_ratelimited debug messages in error
>    cases (suggested by Marcelo)
>  - Minor changes to appease sparse's lock context warning
>
>Changes in v4:
>  - Avoid code repetition using gotos in cast_one (suggested by Paolo)
>  - Fix typo in cover letter (pointed out by Paolo)
>  - Create a module description for act_blockcast
>    (reported by Paolo and CI)
>
>Victor Nogueira (3):
>  net/sched: Introduce tc block netdev tracking infra
>  net/sched: cls_api: Expose tc block to the datapath
>  net/sched: act_blockcast: Introduce blockcast tc action
>
> include/net/sch_generic.h    |   8 +
> include/net/tc_wrapper.h     |   5 +
> include/uapi/linux/pkt_cls.h |   1 +
> net/sched/Kconfig            |  13 ++
> net/sched/Makefile           |   1 +
> net/sched/act_blockcast.c    | 297 +++++++++++++++++++++++++++++++++++
> net/sched/cls_api.c          |  12 +-
> net/sched/sch_api.c          |  58 +++++++
> net/sched/sch_generic.c      |  34 +++-
> 9 files changed, 426 insertions(+), 3 deletions(-)
> create mode 100644 net/sched/act_blockcast.c
>
>-- 
>2.25.1
>