netdev - Re: [PATCH net-next 4/5] ipv4: route: dissect flow in input path if fib rules need it

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] [day] [month] [year] [list]

Message-ID: <CAJieiUgUeQZcqwyHzEr-P23xgL33TZ3biH1JTvNUEsni2EvM7Q@mail.gmail.com>
Date:   Mon, 26 Feb 2018 07:36:41 -0800
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        David Ahern <dsa@...ulusnetworks.com>,
        Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 4/5] ipv4: route: dissect flow in input path if
 fib rules need it

On Mon, Feb 26, 2018 at 1:10 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> On Sat, 2018-02-24 at 21:44 -0800, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@...ulusnetworks.com>
>>
>> Dissect flow in fwd path if fib rules require it. Controlled by
>> a flag to avoid penatly for the common case. Flag is set when fib
>> rules with sport, dport and proto match that require flow dissect
>> are installed. Also passes the dissected hash keys to the multipath
>> hash function when applicable to avoid dissecting the flow again.
>> icmp packets will continue to use inner header for hash
>> calculations (Thanks to Nikolay Aleksandrov for some review here).
>>
>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>> ---
>>  include/net/ip_fib.h     |  2 +-
>>  include/net/netns/ipv4.h |  1 +
>>  net/ipv4/fib_rules.c     |  6 ++++++
>>  net/ipv4/fib_semantics.c |  2 +-
>>  net/ipv4/route.c         | 52 +++++++++++++++++++++++++++++++++++-------------
>>  5 files changed, 47 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
>> index f805243..5ada772 100644
>> --- a/include/net/ip_fib.h
>> +++ b/include/net/ip_fib.h
>> @@ -371,7 +371,7 @@ int fib_sync_up(struct net_device *dev, unsigned int nh_flags);
>>
>>  #ifdef CONFIG_IP_ROUTE_MULTIPATH
>>  int fib_multipath_hash(const struct fib_info *fi, const struct flowi4 *fl4,
>> -                    const struct sk_buff *skb);
>> +                    const struct sk_buff *skb, struct flow_keys *flkeys);
>>  #endif
>>  void fib_select_multipath(struct fib_result *res, int hash);
>>  void fib_select_path(struct net *net, struct fib_result *res,
>> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
>> index 44668c2..87b8fdc 100644
>> --- a/include/net/netns/ipv4.h
>> +++ b/include/net/netns/ipv4.h
>> @@ -52,6 +52,7 @@ struct netns_ipv4 {
>>  #ifdef CONFIG_IP_MULTIPLE_TABLES
>>       struct fib_rules_ops    *rules_ops;
>>       bool                    fib_has_custom_rules;
>> +     bool                    fib_rules_require_fldissect;
>>       struct fib_table __rcu  *fib_main;
>>       struct fib_table __rcu  *fib_default;
>>  #endif
>> diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
>> index 9d55c90..83aa786 100644
>> --- a/net/ipv4/fib_rules.c
>> +++ b/net/ipv4/fib_rules.c
>> @@ -253,6 +253,11 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
>>       }
>>  #endif
>>
>> +     if (rule->ip_proto ||
>> +         fib_rule_port_range_valid(&rule->sport_range) ||
>> +         fib_rule_port_range_valid(&rule->dport_range))
>> +             net->ipv4.fib_rules_require_fldissect = true;
>> +
>>       rule4->src_len = frh->src_len;
>>       rule4->srcmask = inet_make_mask(rule4->src_len);
>>       rule4->dst_len = frh->dst_len;
>
> What about using 'fib_rules_require_fldissect' to conditionally avoid
> all the tests introduced by patch 2/5 ? Perhaps even using a static key
> for that?

yep, ack

>
> It would be great if the kernel would be able to clear this flag when
> no more needed. I know it's not the current behaviour for other similar
> flags, but I hope we can improve ;)
>

yep agreed. I did lean on the existing fib rule flags behavior.
Nikolay suggested I can move this to a counter (which i had been
debating as well).
that gives the ability to clear on deletes.


> Both points above also apply to the ipv6 code path.
>

ack.