netdev - Re: [PATCH net] net/sched: act_ct: Always fill offloading tuple iifidx

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: <87il6dldlp.fsf@nvidia.com>
Date: Tue, 7 Nov 2023 18:30:24 +0200
From: Vlad Buslov <vladbu@...dia.com>
To: Simon Horman <horms@...nel.org>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, <jhs@...atatu.com>, <xiyou.wangcong@...il.com>,
	<jiri@...nulli.us>, <pablo@...filter.org>, Paul Blakey <paulb@...dia.com>
Subject: Re: [PATCH net] net/sched: act_ct: Always fill offloading tuple iifidx


On Tue 07 Nov 2023 at 11:27, Simon Horman <horms@...nel.org> wrote:
> On Fri, Nov 03, 2023 at 04:14:10PM +0100, Vlad Buslov wrote:
>> Referenced commit doesn't always set iifidx when offloading the flow to
>> hardware. Fix the following cases:
>> 
>> - nf_conn_act_ct_ext_fill() is called before extension is created with
>> nf_conn_act_ct_ext_add() in tcf_ct_act(). This can cause rule offload with
>> unspecified iifidx when connection is offloaded after only single
>> original-direction packet has been processed by tc data path. Always fill
>> the new nf_conn_act_ct_ext instance after creating it in
>> nf_conn_act_ct_ext_add().
>> 
>> - Offloading of unidirectional UDP NEW connections is now supported, but ct
>> flow iifidx field is not updated when connection is promoted to
>> bidirectional which can result reply-direction iifidx to be zero when
>> refreshing the connection. Fill in the extension and update flow iifidx
>> before calling flow_offload_refresh().
>
> Hi Vlad,
>
> these changes look good to me. However, I do wonder if the changes for each
> of the two points above should be split into two patches, and
> if the fixes tag for the second point should be.
>
> Fixes: 6a9bad0069cf ("net/sched: act_ct: offload UDP NEW connections")

Hi Simon,

I considered this but decided to send as single patch because
connections 'refresh' mechanism has already existed before the UDP NEW
offload and it didn't update the iifidx. While yes, it wasn't
technically necessary because only established connections were
considered for offloading I'm still leaning more towards considering it
a flow in original implementation since UDP NEW support wasn't the first
change modifying the offload behavior (43332cf97425 ("net/sched: act_ct:
Offload only ASSURED connections") was before that), so further changes
should have been anticipated. Hope this clarifies my motivation.

Note that I don't have strong opinion about it and willing to split the
patch, if necessary but to me it appears as just more trouble for
maintainers without any benefits...

>
>> Fixes: 9795ded7f924 ("net/sched: act_ct: Fill offloading tuple iifidx")
>> Reviewed-by: Paul Blakey <paulb@...dia.com>
>> Signed-off-by: Vlad Buslov <vladbu@...dia.com>
>
> ...