netdev - Re: [PATCH -next 0/5] replace skb tc_verd member with 3 dedicated bit flags

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:	Tue, 05 May 2015 07:39:36 -0400
From:	Jamal Hadi Salim <jhs@...atatu.com>
To:	Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
CC:	alexei.starovoitov@...il.com
Subject: Re: [PATCH -next 0/5] replace skb tc_verd member with 3 dedicated
 bit flags

Florian,

Initial feedback on the series:
- Can you keep the macros around? eg SET_TC_NCLS is more readable
than skb->tc_nocls = 1 also hides the bit details.
Those macros are not used in user space (there was need earlier but
it is no longer necessary; dont worry about out of tree code).
So this is an opportunity to redefine them. Move them to 
include/net/pkt_cls.h and redefine them so readability is better. Maybe 
use set_bit
instead of the complex ones that exist there.
I think the ones that are no longer needed should just be deleted
as opposed to what you and Alexei did earlier.
- We need two bits for the location (ingress, egress, from stack)
from stack being 0 i.e when it is not set implicitly it is from the
host stack then we can check for ingress or egress when we choose.
Please keep the two bits.

cheers,
jamal

On 05/04/15 14:48, Florian Westphal wrote:
> This series removes the tc_verd member from sk_buff
> and uses 3 new flag bits instead.  It is on top of
> http://patchwork.ozlabs.org/patch/467186/
> (tc: remove unused redirect ttl) from Alexei.
>
> The patch set tries to not affect tc inner workings.
> I tested a few scenarios, namely:
>
> - htb on egress
> - netem attached to ifb with mirred redirect from ingress
>    qdisc
> - mirred to different device
> - bogus packet reclassify loop
>
> All of that works as expected (i.e. in last case most packets
> are dropped).
>
> motivation is two-fold:
> 1) provide better documentation as to what tc_verd is
>     used for and try to move some flag set/clear operations
>     into sch_ingress
>
> 2) provide a way to reduce skb size by 8 bytes
>     (s/u16 mac_len/u8 mac_len/ would result in
>      two 2 byte and one 4 byte hole, i.e. 8 byte reduction with
>      minor reshuffling).
>
> Florian Westphal (5):
>        net: sched: replace NCLS macro with tc_nocls bit flag
>        net: sched: use counter to break reclassify loops
>        net: sched: remove FROM INGRESS/EGRESS
>        net: sched: remove AT INGRESS/EGRESS
>        skbuff: remove tc_verd member
>
> Documentation/networking/tc-actions-env-rules.txt |    4 ----
>   drivers/net/ifb.c                                 |   15 +++++----------
>   drivers/staging/octeon/ethernet-tx.c              |    4 +++-
>   include/linux/skbuff.h                            |   17 +++++++++++------
>   include/net/sch_generic.h                         |   15 ---------------
>   include/uapi/linux/pkt_cls.h                      |    4 +---
>   net/core/dev.c                                    |   10 ++--------
>   net/core/skbuff.c                                 |    3 ---
>   net/sched/act_api.c                               |    4 ++--
>   net/sched/act_mirred.c                            |    9 ++++-----
>   net/sched/sch_api.c                               |   12 +++---------
>   net/sched/sch_ingress.c                           |    2 ++
>   net/sched/sch_netem.c                             |    2 +-
>   13 files changed, 34 insertions(+), 67 deletions(-)
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html