Re: [PATCH net] netfilter: flowtable: additional checks for outdated flows

[Vlad Buslov <vladbu@nvidia.com>]

On Tue 24 Oct 2023 at 22:07, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Oct 24, 2023 at 10:45:31PM +0300, Vlad Buslov wrote:
>> On Tue 24 Oct 2023 at 21:40, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > Hi Vlad,
>> >
>> > On Tue, Oct 24, 2023 at 07:17:18PM +0200, Vlad Buslov wrote:
>> >> Current nf_flow_is_outdated() implementation considers any flow table flow
>> >> which state diverged from its underlying CT connection status for teardown
>> >> which can be problematic in the following cases:
>> >> 
>> >> - Flow has never been offloaded to hardware in the first place either
>> >> because flow table has hardware offload disabled (flag
>> >> NF_FLOWTABLE_HW_OFFLOAD is not set) or because it is still pending on 'add'
>> >> workqueue to be offloaded for the first time. The former is incorrect, the
>> >> later generates excessive deletions and additions of flows.
>> >> 
>> >> - Flow is already pending to be updated on the workqueue. Tearing down such
>> >> flows will also generate excessive removals from the flow table, especially
>> >> on highly loaded system where the latency to re-offload a flow via 'add'
>> >> workqueue can be quite high.
>> >> 
>> >> When considering a flow for teardown as outdated verify that it is both
>> >> offloaded to hardware and doesn't have any pending updates.
>> >
>> > Thanks.
>> >
>> > I have posted an alternative patch to move the handling of
>> > NF_FLOW_HW_ESTABLISHED, which is specific for sched/act_ct:
>> >
>> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231024193815.1987-1-pablo@netfilter.org/
>> >
>> > it is a bit more code, but it makes it easier to understand for the
>> > code reader that this bit is net/sched specific.
>> >
>> 
>> Thanks for refactoring this, I agree that separating the act_ct-specific
>> check makes it more obvious.
>> 
>> How would you prefer to solve the conflict with my fix? Should I wait
>> for your patch to be accepted to net, rebase my fix on top and submit
>> V2? Or you can incorporate the checks from my fix together with my
>> signoff and submit it as a single change?
>
> Rebased here as per your request:
>
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231024200243.50784-1-pablo@netfilter.org/
>
> I took the freedom to take your Signed-off-by: and Paul's Reviewed-by:
> which is not the best way to go, but please acknowledge this is fine
> in this exceptional case.

Ack. Replied to the patch with my signed-off-by. Thanks!

>
> We can handle this via nf.git tree, there were no plans to send a PR
> to netdev, but I think these fixes are worth to (try to) get them
> there in time for the 6.6 release.
>
> Thanks.