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

[Vlad Buslov <vladbu@nvidia.com>]

On Tue 07 Nov 2023 at 11:27, Simon Horman <horms@kernel.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@nvidia.com>
>> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
>
> ...