netdev - Re: [PATCH net-next 1/1] net_sched: Introduce skbmod 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]

Message-ID: <20160718041933.GB36253@ast-mbp.thefacebook.com>
Date:	Sun, 17 Jul 2016 21:19:35 -0700
From:	Alexei Starovoitov <alexei.starovoitov@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org, daniel@...earbox.net,
	xiyou.wangcong@...il.com, nikolay@...ulusnetworks.com
Subject: Re: [PATCH net-next 1/1] net_sched: Introduce skbmod action

On Sun, Jul 17, 2016 at 04:41:24AM -0400, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@...atatu.com>
> 
> This action is intended to be an upgrade from a usability perspective
> from pedit (as well as operational debugability).
> Compare this:
> 
> sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
> u32 match ip protocol 1 0xff flowid 1:2 \
> action pedit munge offset -14 u8 set 0x02 \
>     munge offset -13 u8 set 0x15 \
>     munge offset -12 u8 set 0x15 \
>     munge offset -11 u8 set 0x15 \
>     munge offset -10 u16 set 0x1515 \
>     pipe
> 
> to:
> 
> sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
> u32 match ip protocol 1 0xff flowid 1:2 \
> action skbmod dmac 02:15:15:15:15:15
> 
> Or worse, try to debug a policy with destination mac, source mac and
> etherype. Then make that a hundred rules and you'll get my point.
> 
> In the future common use cases on pedit can be migrated to this action
> (as an example different fields in ip v4/6, transports like tcp/udp/sctp
> etc). For this first cut, this allows modifying basic ethernet header.
> 
> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
> ---
>  include/net/tc_act/tc_skbmod.h        |  35 +++++
>  include/uapi/linux/tc_act/tc_skbmod.h |  47 +++++++
>  net/sched/Kconfig                     |  11 ++
>  net/sched/Makefile                    |   1 +
>  net/sched/act_skbmod.c                | 245 ++++++++++++++++++++++++++++++++++
>  5 files changed, 339 insertions(+)
>  create mode 100644 include/net/tc_act/tc_skbmod.h
>  create mode 100644 include/uapi/linux/tc_act/tc_skbmod.h
>  create mode 100644 net/sched/act_skbmod.c

I agree with Cong's point that this new action adds 339 lines of kernel
code without adding any new functionality.
It is suppose to solve debugging issues, but it's hard to understand
from commit log.
I can imagine new tc command 'action skbmod dmac 02:15:15:15:15:15' that
uses kernel pedit action undercover, so from user space point of view
the same effect can be achieved by extending iproute2.
What is the reason to add this action to the kernel?
By debug you mean to dump kernel action list and multiple pedits
are hard to decipher? Anything else I'm missing?