netdev - Re: [PATCH net 1/1] ice: identify aRFS flows using L3/L4 dissector info

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:   Thu, 13 Apr 2023 10:27:56 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Ahmed Zaki <ahmed.zaki@...el.com>,
        Leon Romanovsky <leon@...nel.org>,
        Tony Nguyen <anthony.l.nguyen@...el.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <edumazet@...gle.com>, <netdev@...r.kernel.org>,
        Arpana Arland <arpanax.arland@...el.com>
Subject: Re: [PATCH net 1/1] ice: identify aRFS flows using L3/L4 dissector
 info



On 4/10/2023 11:54 AM, Ahmed Zaki wrote:
> 
> On 2023-04-09 04:45, Leon Romanovsky wrote:
>> On Fri, Apr 07, 2023 at 02:08:20PM -0700, Tony Nguyen wrote:
>>> From: Ahmed Zaki <ahmed.zaki@...el.com>
>>>
>>> The flow ID passed to ice_rx_flow_steer() is computed like this:
>>>
>>>      flow_id = skb_get_hash(skb) & flow_table->mask;
>>>
>>> With smaller aRFS tables (for example, size 256) and higher number of
>>> flows, there is a good chance of flow ID collisions where two or more
>>> different flows are using the same flow ID. This results in the aRFS
>>> destination queue constantly changing for all flows sharing that ID.
>>>
>>> Use the full L3/L4 flow dissector info to identify the steered flow
>>> instead of the passed flow ID.
>>>
>>> Fixes: 28bf26724fdb ("ice: Implement aRFS")
>>> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.com>
>>> Tested-by: Arpana Arland <arpanax.arland@...el.com> (A Contingent worker at Intel)
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>>> ---
>>>   drivers/net/ethernet/intel/ice/ice_arfs.c | 44 +++++++++++++++++++++--
>>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_arfs.c b/drivers/net/ethernet/intel/ice/ice_arfs.c
>>> index fba178e07600..d7ae64d21e01 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_arfs.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_arfs.c
>>> @@ -345,6 +345,44 @@ ice_arfs_build_entry(struct ice_vsi *vsi, const struct flow_keys *fk,
>>>   	return arfs_entry;
>>>   }
>>>   
>>> +/**
>>> + * ice_arfs_cmp - compare flow to a saved ARFS entry's filter info
>>> + * @fltr_info: filter info of the saved ARFS entry
>>> + * @fk: flow dissector keys
>>> + *
>>> + * Caller must hold arfs_lock if @fltr_info belongs to arfs_fltr_list
>>> + */
>>> +static bool
>>> +ice_arfs_cmp(struct ice_fdir_fltr *fltr_info, const struct flow_keys *fk)
>>> +{
>>> +	bool is_ipv4;
>>> +
>>> +	if (!fltr_info || !fk)
>>> +		return false;
>>> +
>>> +	is_ipv4 = (fltr_info->flow_type == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
>>> +		fltr_info->flow_type == ICE_FLTR_PTYPE_NONF_IPV4_TCP);
>>> +
>>> +	if (fk->basic.n_proto == htons(ETH_P_IP) && is_ipv4)
>>> +		return (fltr_info->ip.v4.proto == fk->basic.ip_proto &&
>>> +			fltr_info->ip.v4.src_port == fk->ports.src &&
>>> +			fltr_info->ip.v4.dst_port == fk->ports.dst &&
>>> +			fltr_info->ip.v4.src_ip == fk->addrs.v4addrs.src &&
>>> +			fltr_info->ip.v4.dst_ip == fk->addrs.v4addrs.dst);
>>> +	else if (fk->basic.n_proto == htons(ETH_P_IPV6) && !is_ipv4)
>>> +		return (fltr_info->ip.v6.proto == fk->basic.ip_proto &&
>>> +			fltr_info->ip.v6.src_port == fk->ports.src &&
>>> +			fltr_info->ip.v6.dst_port == fk->ports.dst &&
>>> +			!memcmp(&fltr_info->ip.v6.src_ip,
>>> +				&fk->addrs.v6addrs.src,
>>> +				sizeof(struct in6_addr)) &&
>>> +			!memcmp(&fltr_info->ip.v6.dst_ip,
>>> +				&fk->addrs.v6addrs.dst,
>>> +				sizeof(struct in6_addr)));
>> I'm confident that you can write this function more clear with
>> comparisons in one "return ..." instruction.
>>>> Thanks
> 
> Do you mean remove the "if condition"? how?
> 
> I wrote it this way to match how I'd think:
> 
> If (IPv4 and V4 flows), test IPv4 flow keys, else if (IPv6 and V6 
> flows), test IPv6 keys, else false.
> 

You can use a || chain, something like:

return (is_ipv4 && (<check ipv4 fields)) || (!is_ipv4 && (<check ip6
fields>)

There might be other ways to simplify the conditional. You could
possibly combine the n_proto check with the is_ipv4 check above as well.


> I m not sure how can I make it more clearer.
> 
> Thanks.
>