netdev - Re: [PATCH net-next v2 2/2] openvswitch: Support conntrack zone limit

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG1aQh+KXPxB3yvmrdxgV6eO63CiwDFSB_RGcsMhGd4rUvSxSQ@mail.gmail.com>
Date:   Wed, 25 Apr 2018 14:51:42 -0700
From:   Yi-Hung Wei <yihung.wei@...il.com>
To:     Pravin Shelar <pshelar@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/2] openvswitch: Support conntrack zone limit

>> +#if    IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>> +#define OVS_CT_LIMIT_UNLIMITED 0
>> +#define OVS_CT_LIMIT_DEFAULT OVS_CT_LIMIT_UNLIMITED
>> +#define CT_LIMIT_HASH_BUCKETS 512
>> +
> Can you use static key when the limit is not set.
> This would avoid overhead in datapath when these limits are not used.
>
Thanks Parvin for the review.  I'm not sure about the 'static key'
part, are you suggesting that say if we can have a flag to check if no
one has ever set the ct_limit?   If the ct_limit feature is not used
so far, then instead of look up the hash table for the zone limit, we
just return the default unlimited value.  So that we can avoid the
overhead of checking ct_limit.

>> +struct ovs_ct_limit {
>> +       /* Elements in ovs_ct_limit_info->limits hash table */
>> +       struct hlist_node hlist_node;
>> +       struct rcu_head rcu;
>> +       u16 zone;
>> +       u32 limit;
>> +};
>> +
> ...


>>  /* Lookup connection and confirm if unconfirmed. */
>>  static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>>                          const struct ovs_conntrack_info *info,
>> @@ -1054,6 +1176,13 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>>         if (!ct)
>>                 return 0;
>>
>> +#if    IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>> +       err = ovs_ct_check_limit(net, info,
>> +                                &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
>> +       if (err)
>> +               return err;
>> +#endif
>> +
>
> This could be checked during flow install time, so that only permitted
> flows would have 'ct commit' action, we can avoid per packet cost
> checking the limit.
It seems to me that it would be hard to check the # of connections of
a flow in the flow installation stage.  For example, if we have a
datapath flow that performs “ct commit” action on all UDP traffic from
in_port 1, then it could be various combinations of 5-tuple that form
various # of connections.  Therefore, it would be hard to do the
admission control there.


> returning error code form ovs_ct_commit() is lost in datapath and it
> would be hard to debug packet lost in case of the limit is reached. So
> another advantage of checking the limit in flow install be better
> traceability. datapath would return error to usespace and it can log
> the error code.
Thanks for pointing out the error code from ovs_ct_commit() will be
lost in the datapath.  In this case, shall we consider to report the
packet drop by some rate_limit logging such as net_warn_ratelimited()
or net_info_ratelimited()?

Thanks,

-Yi-Hung