netdev - Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules

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: <56CFFCFB.4060508@gmail.com>
Date:	Thu, 25 Feb 2016 23:21:31 -0800
From:	John Fastabend <john.fastabend@...il.com>
To:	"Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
	jiri@...nulli.us, daniel@...earbox.net, simon.horman@...ronome.com
CC:	netdev@...r.kernel.org, alexei.starovoitov@...il.com,
	davem@...emloft.net, jhs@...atatu.com
Subject: Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify
 software only rules

On 16-02-25 11:02 PM, Samudrala, Sridhar wrote:
> On 2/25/2016 3:20 PM, John Fastabend wrote:
>> In the initial implementation the only way to stop a rule from being
>> inserted into the hardware table was via the device feature flag.
>> However this doesn't work well when working on an end host system
>> where packets are expect to hit both the hardware and software
>> datapaths.
>>
>> For example we can imagine a rule that will match an IP address and
>> increment a field. If we install this rule in both hardware and
>> software we may increment the field twice. To date we have only
>> added support for the drop action so we have been able to ignore
>> these cases. But as we extend the action support we will hit this
>> example plus more such cases. Arguably these are not even corner
>> cases in many working systems these cases will be common.
>>
>> To avoid forcing the driver to always abort (i.e. the above example)
>> this patch adds a flag to add a rule in software only. A careful
>> user can use this flag to build software and hardware datapaths
>> that work together. One example we have found particularly useful
>> is to use hardware resources to set the skb->mark on the skb when
>> the match may be expensive to run in software but a mark lookup
>> in a hash table is cheap. The idea here is hardware can do in one
>> lookup what the u32 classifier may need to traverse multiple lists
>> and hash tables to compute. The flag is only passed down on inserts
>> on deletion to avoid stale references in hardware we always try
> 
> I think this is supposed to be a new sentence starting with 'On deletion'

Yep.

>> to remove a rule if it exists.
>>
>> The flags field is part of the classifier specific options. Although
>> it is tempting to lift this into the generic structure doing this
>> proves difficult do to how the tc netlink attributes are implemented
>> along with how the dump/change routines are called. There is also
>> precedence for putting seemingly generic pieces in the specific
>> classifier options such as TCA_U32_POLICE, TCA_U32_ACT, etc. So
>> although not ideal I've left FLAGS in the u32 options as well as it
>> simplifies the code greatly and user space has already learned how
>> to manage these bits ala 'tc' tool.
>>
>> Another thing if trying to update a rule we require the flags to
>> be unchanged. This is to force user space, software u32 and
>> the hardware u32 to keep in sync. Thanks to Simon Horman for
>> catching this case.
>>

[...]

>> u32_policy[TCA_U32_MAX + 1] = {
>>       [TCA_U32_SEL]        = { .len = sizeof(struct tc_u32_sel) },
>>       [TCA_U32_INDEV]        = { .type = NLA_STRING, .len = IFNAMSIZ },
>>       [TCA_U32_MARK]        = { .len = sizeof(struct tc_u32_mark) },
>> +    [TCA_U32_FLAGS]        = { .len = NLA_U32 },
> should be  .type = NLA_U32
> 

Yep stupid typo. I think I'm going to write some smatch files to
catch these sorts of things they should be detectable pragmatically.

Thanks.

>> <snip>
>