netdev - Re: [PATCH net-next v16 5/8] sch_cake: Add NAT awareness to packet classifier

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: <878t83tod5.fsf@toke.dk>
Date:   Tue, 29 May 2018 00:19:18 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netdev@...r.kernel.org, cake@...ts.bufferbloat.net,
        netfilter-devel@...r.kernel.org
Subject: Re: [PATCH net-next v16 5/8] sch_cake: Add NAT awareness to packet classifier

Pablo Neira Ayuso <pablo@...filter.org> writes:

> On Mon, May 28, 2018 at 04:27:46PM +0200, Toke Høiland-Jørgensen wrote:
>> When CAKE is deployed on a gateway that also performs NAT (which is a
>> common deployment mode), the host fairness mechanism cannot distinguish
>> internal hosts from each other, and so fails to work correctly.
>> 
>> To fix this, we add an optional NAT awareness mode, which will query the
>> kernel conntrack mechanism to obtain the pre-NAT addresses for each packet
>> and use that in the flow and host hashing.
>> 
>> When the shaper is enabled and the host is already performing NAT, the cost
>> of this lookup is negligible. However, in unlimited mode with no NAT being
>> performed, there is a significant CPU cost at higher bandwidths. For this
>> reason, the feature is turned off by default.
>> 
>> Cc: netfilter-devel@...r.kernel.org
>> Signed-off-by: Toke Høiland-Jørgensen <toke@...e.dk>
>> ---
>>  net/sched/sch_cake.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>> 
>> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
>> index 68ac908470f1..fecd9caac0cc 100644
>> --- a/net/sched/sch_cake.c
>> +++ b/net/sched/sch_cake.c
>> @@ -71,6 +71,10 @@
>>  #include <net/tcp.h>
>>  #include <net/flow_dissector.h>
>>  
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> +#include <net/netfilter/nf_conntrack_core.h>
>> +#endif
>> +
>>  #define CAKE_SET_WAYS (8)
>>  #define CAKE_MAX_TINS (8)
>>  #define CAKE_QUEUES (1024)
>> @@ -516,6 +520,29 @@ static bool cobalt_should_drop(struct cobalt_vars *vars,
>>  	return drop;
>>  }
>>  
>> +static void cake_update_flowkeys(struct flow_keys *keys,
>> +				 const struct sk_buff *skb)
>> +{
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>
> I would remove the ifdef, not really needed, it will simplify things.
>
> But I leave it to you to decide, this is not I deal breaker.

If I remove it I get a bunch of 'incomplete type' errors when compiling.
Besides, we use it to report an error to userspace when conntrack is
disabled anyway, so might as well keep the whole thing ifdef'ed.

-Toke