[PATCH net 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems

[PATCH net 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems

[Roi Dayan]

On heavily loaded systems the GC can take time to go over all existing
conns and reset their timeout. At that time other calls like from
nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
expired. To fix this when we set the offload bit we should also reset
the timeout instead of counting on GC to finish first iteration over
all conns before the initial timeout.

First commit is to expose the function that updates the timeout.
Second commit is to use it from act_ct.

Roi Dayan (2):
  netfilter: conntrack: Move nf_ct_offload_timeout to header file
  net/sched: act_ct: Set offload timeout when setting the offload bit

 include/net/netfilter/nf_conntrack.h | 12 ++++++++++++
 net/netfilter/nf_conntrack_core.c    | 12 ------------
 net/sched/act_ct.c                   |  2 ++
 3 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.8.4

[PATCH net 1/2] netfilter: conntrack: Move nf_ct_offload_timeout to header file

[Roi Dayan]

To be used by callers from other modules.

Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
---
 include/net/netfilter/nf_conntrack.h | 12 ++++++++++++
 net/netfilter/nf_conntrack_core.c    | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 90690e37a56f..8481819ff632 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -279,6 +279,18 @@ static inline bool nf_ct_should_gc(const struct nf_conn *ct)
 	       !nf_ct_is_dying(ct);
 }
 
+#define	DAY	(86400 * HZ)
+
+/* Set an arbitrary timeout large enough not to ever expire, this save
+ * us a check for the IPS_OFFLOAD_BIT from the packet path via
+ * nf_ct_is_expired().
+ */
+static inline void nf_ct_offload_timeout(struct nf_conn *ct)
+{
+	if (nf_ct_expires(ct) < DAY / 2)
+		ct->timeout = nfct_time_stamp + DAY;
+}
+
 struct kernel_param;
 
 int nf_conntrack_set_hashsize(const char *val, const struct kernel_param *kp);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 79cd9dde457b..947c6d9437c3 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1344,18 +1344,6 @@ static bool gc_worker_can_early_drop(const struct nf_conn *ct)
 	return false;
 }
 
-#define	DAY	(86400 * HZ)
-
-/* Set an arbitrary timeout large enough not to ever expire, this save
- * us a check for the IPS_OFFLOAD_BIT from the packet path via
- * nf_ct_is_expired().
- */
-static void nf_ct_offload_timeout(struct nf_conn *ct)
-{
-	if (nf_ct_expires(ct) < DAY / 2)
-		ct->timeout = nfct_time_stamp + DAY;
-}
-
 static void gc_worker(struct work_struct *work)
 {
 	unsigned int min_interval = max(HZ / GC_MAX_BUCKETS_DIV, 1u);
-- 
2.8.4

[PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit

[Roi Dayan]

On heavily loaded systems the GC can take time to go over all existing
conns and reset their timeout. At that time other calls like from
nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
expired. To fix this when we set the offload bit we should also reset
the timeout instead of counting on GC to finish first iteration over
all conns before the initial timeout.

Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table")
Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
---
 net/sched/act_ct.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index e9f3576cbf71..650c2d78a346 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
 	if (err)
 		goto err_add;
 
+	nf_ct_offload_timeout(ct);
+
 	return;
 
 err_add:
-- 
2.8.4

Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit

[Marcelo Ricardo Leitner]

On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote:
> On heavily loaded systems the GC can take time to go over all existing
> conns and reset their timeout. At that time other calls like from
> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
> expired. To fix this when we set the offload bit we should also reset
> the timeout instead of counting on GC to finish first iteration over
> all conns before the initial timeout.
> 
> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table")
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
> ---
>  net/sched/act_ct.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index e9f3576cbf71..650c2d78a346 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,

Extra context line:
	err = flow_offload_add(&ct_ft->nf_ft, entry);
>  	if (err)
>  		goto err_add;
>  
> +	nf_ct_offload_timeout(ct);
> +

What about adding this to flow_offload_add() instead?
It is already adjusting the flow_offload timeout there and then it
also effective for nft.

>  	return;
>  
>  err_add:
> -- 
> 2.8.4
> 

Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit

[Roi Dayan]

On 2020-07-28 5:42 PM, Marcelo Ricardo Leitner wrote:
> On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote:
>> On heavily loaded systems the GC can take time to go over all existing
>> conns and reset their timeout. At that time other calls like from
>> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
>> expired. To fix this when we set the offload bit we should also reset
>> the timeout instead of counting on GC to finish first iteration over
>> all conns before the initial timeout.
>>
>> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table")
>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
>> ---
>>  net/sched/act_ct.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index e9f3576cbf71..650c2d78a346 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
> 
> Extra context line:
> 	err = flow_offload_add(&ct_ft->nf_ft, entry);
>>  	if (err)
>>  		goto err_add;
>>  
>> +	nf_ct_offload_timeout(ct);
>> +
> 
> What about adding this to flow_offload_add() instead?
> It is already adjusting the flow_offload timeout there and then it
> also effective for nft.
> 

As you said, in flow_offload_add() we adjust the flow timeout.
Here we adjust the conn timeout.
So it's outside flow_offload_add() which only touch the flow struct.
I guess it's like conn offload bit is set outside here and for nft.
What do you think?

>>  	return;
>>  
>>  err_add:
>> -- 
>> 2.8.4
>>

Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit

[Marcelo Ricardo Leitner]

On Wed, Jul 29, 2020 at 03:55:53PM +0300, Roi Dayan wrote:
> 
> 
> On 2020-07-28 5:42 PM, Marcelo Ricardo Leitner wrote:
> > On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote:
> >> On heavily loaded systems the GC can take time to go over all existing
> >> conns and reset their timeout. At that time other calls like from
> >> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
> >> expired. To fix this when we set the offload bit we should also reset
> >> the timeout instead of counting on GC to finish first iteration over
> >> all conns before the initial timeout.
> >>
> >> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table")
> >> Signed-off-by: Roi Dayan <roid@mellanox.com>
> >> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
> >> ---
> >>  net/sched/act_ct.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> >> index e9f3576cbf71..650c2d78a346 100644
> >> --- a/net/sched/act_ct.c
> >> +++ b/net/sched/act_ct.c
> >> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
> > 
> > Extra context line:
> > 	err = flow_offload_add(&ct_ft->nf_ft, entry);
> >>  	if (err)
> >>  		goto err_add;
> >>  
> >> +	nf_ct_offload_timeout(ct);
> >> +
> > 
> > What about adding this to flow_offload_add() instead?
> > It is already adjusting the flow_offload timeout there and then it
> > also effective for nft.
> > 
> 
> As you said, in flow_offload_add() we adjust the flow timeout.
> Here we adjust the conn timeout.
> So it's outside flow_offload_add() which only touch the flow struct.
> I guess it's like conn offload bit is set outside here and for nft.

Right, but

> What do you think?

I don't see why it can't update both. flow_offload_fixup_ct_timeout(),
called by flow_offload_del(), is updating ct->timeout already. It
looks consistent to me to update it in _add as well then. 

> 
> >>  	return;
> >>  
> >>  err_add:
> >> -- 
> >> 2.8.4
> >>

Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit

[Roi Dayan]

On 2020-07-29 8:10 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 29, 2020 at 03:55:53PM +0300, Roi Dayan wrote:
>>
>>
>> On 2020-07-28 5:42 PM, Marcelo Ricardo Leitner wrote:
>>> On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote:
>>>> On heavily loaded systems the GC can take time to go over all existing
>>>> conns and reset their timeout. At that time other calls like from
>>>> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
>>>> expired. To fix this when we set the offload bit we should also reset
>>>> the timeout instead of counting on GC to finish first iteration over
>>>> all conns before the initial timeout.
>>>>
>>>> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table")
>>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>>> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
>>>> ---
>>>>  net/sched/act_ct.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>>>> index e9f3576cbf71..650c2d78a346 100644
>>>> --- a/net/sched/act_ct.c
>>>> +++ b/net/sched/act_ct.c
>>>> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
>>>
>>> Extra context line:
>>> 	err = flow_offload_add(&ct_ft->nf_ft, entry);
>>>>  	if (err)
>>>>  		goto err_add;
>>>>  
>>>> +	nf_ct_offload_timeout(ct);
>>>> +
>>>
>>> What about adding this to flow_offload_add() instead?
>>> It is already adjusting the flow_offload timeout there and then it
>>> also effective for nft.
>>>
>>
>> As you said, in flow_offload_add() we adjust the flow timeout.
>> Here we adjust the conn timeout.
>> So it's outside flow_offload_add() which only touch the flow struct.
>> I guess it's like conn offload bit is set outside here and for nft.
> 
> Right, but
> 
>> What do you think?
> 
> I don't see why it can't update both. flow_offload_fixup_ct_timeout(),
> called by flow_offload_del(), is updating ct->timeout already. It
> looks consistent to me to update it in _add as well then. 
> 

I don't mind. just add is not consistent with del.
del also clears the ips_offload_bit but add doesn't add it.
i'll send v2 with your suggestion.

>>
>>>>  	return;
>>>>  
>>>>  err_add:
>>>> -- 
>>>> 2.8.4
>>>>