[nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Bodong Wang]

This bit indicates that the conntrack entry is offloaded to hardware
flow table. nf_conntrack entry will be tagged with [HW_OFFLOAD] if
it's offload to hardware.

cat /proc/net/nf_conntrack
	ipv4 2 tcp 6 \
	src=1.1.1.17 dst=1.1.1.16 sport=56394 dport=5001 \
	src=1.1.1.16 dst=1.1.1.17 sport=5001 dport=56394 [HW_OFFLOAD] \
	mark=0 zone=0 use=3

Note that HW_OFFLOAD/OFFLOAD/ASSURED are mutually exclusive.

Signed-off-by: Bodong Wang <bodong@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
Reviewed-by: Paul Blakey <paulb@mellanox.com>
---
 include/uapi/linux/netfilter/nf_conntrack_common.h |  8 ++++++--
 net/netfilter/nf_conntrack_standalone.c            |  4 +++-
 net/netfilter/nf_flow_table_offload.c              | 13 +++++++++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
index b6f0bb1dc799..4b3395082d15 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -114,15 +114,19 @@ enum ip_conntrack_status {
 	IPS_OFFLOAD_BIT = 14,
 	IPS_OFFLOAD = (1 << IPS_OFFLOAD_BIT),
 
+	/* Conntrack has been offloaded to hardware. */
+	IPS_HW_OFFLOAD_BIT = 15,
+	IPS_HW_OFFLOAD = (1 << IPS_HW_OFFLOAD_BIT),
+
 	/* Be careful here, modifying these bits can make things messy,
 	 * so don't let users modify them directly.
 	 */
 	IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
 				 IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
 				 IPS_SEQ_ADJUST | IPS_TEMPLATE | IPS_UNTRACKED |
-				 IPS_OFFLOAD),
+				 IPS_OFFLOAD | IPS_HW_OFFLOAD),
 
-	__IPS_MAX_BIT = 15,
+	__IPS_MAX_BIT = 16,
 };
 
 /* Connection tracking event types */
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 9b57330c81f8..5a3e6c43ee68 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -348,7 +348,9 @@ static int ct_seq_show(struct seq_file *s, void *v)
 	if (seq_print_acct(s, ct, IP_CT_DIR_REPLY))
 		goto release;
 
-	if (test_bit(IPS_OFFLOAD_BIT, &ct->status))
+	if (test_bit(IPS_HW_OFFLOAD_BIT, &ct->status))
+		seq_puts(s, "[HW_OFFLOAD] ");
+	else if (test_bit(IPS_OFFLOAD_BIT, &ct->status))
 		seq_puts(s, "[OFFLOAD] ");
 	else if (test_bit(IPS_ASSURED_BIT, &ct->status))
 		seq_puts(s, "[ASSURED] ");
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index e3b099c14eff..c9efd20ea65c 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -754,12 +754,15 @@ static void flow_offload_work_add(struct flow_offload_work *offload)
 	err = flow_offload_rule_add(offload, flow_rule);
 	if (err < 0)
 		set_bit(NF_FLOW_HW_REFRESH, &offload->flow->flags);
+	else
+		set_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
 
 	nf_flow_offload_destroy(flow_rule);
 }
 
 static void flow_offload_work_del(struct flow_offload_work *offload)
 {
+	clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
 	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
@@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
 				       FLOW_OFFLOAD_DIR_REPLY,
 				       stats[1].pkts, stats[1].bytes);
 	}
+
+	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
+	 * happen in two scenarios:
+	 *
+	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
+	 *    HW driver is unloaded.
+	 * 2. One of the shared block driver is unloaded.
+	 */
+	if (!lastused)
+		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
 }
 
 static void flow_offload_work_handler(struct work_struct *work)
-- 
2.21.1

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Pablo Neira Ayuso]

On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
[...]
> @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
>  				       FLOW_OFFLOAD_DIR_REPLY,
>  				       stats[1].pkts, stats[1].bytes);
>  	}
> +
> +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
> +	 * happen in two scenarios:
> +	 *
> +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
> +	 *    HW driver is unloaded.
> +	 * 2. One of the shared block driver is unloaded.
> +	 */
> +	if (!lastused)
> +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>  }

Better inconditionally clear off the flag after the entry is removed
from hardware instead of relying on the lastused field?

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Bodong Wang]

On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote:
> On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
> [...]
>> @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
>>   				       FLOW_OFFLOAD_DIR_REPLY,
>>   				       stats[1].pkts, stats[1].bytes);
>>   	}
>> +
>> +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
>> +	 * happen in two scenarios:
>> +	 *
>> +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
>> +	 *    HW driver is unloaded.
>> +	 * 2. One of the shared block driver is unloaded.
>> +	 */
>> +	if (!lastused)
>> +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>>   }
> Better inconditionally clear off the flag after the entry is removed
> from hardware instead of relying on the lastused field?

Functionality wise, it should work. Current way is more for containing 
the set/clear in the same domain, and no need to ask each vendor to take 
care of this bit.

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On Mon, Apr 20, 2020 at 10:28:00AM -0500, Bodong Wang wrote:
> On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote:
> > On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
> > [...]
> > > @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
> > >   				       FLOW_OFFLOAD_DIR_REPLY,
> > >   				       stats[1].pkts, stats[1].bytes);
> > >   	}
> > > +
> > > +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
> > > +	 * happen in two scenarios:
> > > +	 *
> > > +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
> > > +	 *    HW driver is unloaded.
> > > +	 * 2. One of the shared block driver is unloaded.
> > > +	 */
> > > +	if (!lastused)
> > > +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
> > >   }
> > Better inconditionally clear off the flag after the entry is removed
> > from hardware instead of relying on the lastused field?
> 
> Functionality wise, it should work. Current way is more for containing the
> set/clear in the same domain, and no need to ask each vendor to take care of
> this bit.

No need to ask each vendor, what I mean is to deal with this from
flow_offload_work_del(), see attached patch.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 619 bytes --]

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index e3b099c14eff..593fefd52ef7 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -763,6 +763,7 @@ static void flow_offload_work_del(struct flow_offload_work *offload)
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
 	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
+	clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
 }
 
 static void flow_offload_tuple_stats(struct flow_offload_work *offload,

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Bodong Wang]

On 4/20/2020 10:33 AM, Pablo Neira Ayuso wrote:
> On Mon, Apr 20, 2020 at 10:28:00AM -0500, Bodong Wang wrote:
>> On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote:
>>> On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
>>> [...]
>>>> @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
>>>>    				       FLOW_OFFLOAD_DIR_REPLY,
>>>>    				       stats[1].pkts, stats[1].bytes);
>>>>    	}
>>>> +
>>>> +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
>>>> +	 * happen in two scenarios:
>>>> +	 *
>>>> +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
>>>> +	 *    HW driver is unloaded.
>>>> +	 * 2. One of the shared block driver is unloaded.
>>>> +	 */
>>>> +	if (!lastused)
>>>> +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>>>>    }
>>> Better inconditionally clear off the flag after the entry is removed
>>> from hardware instead of relying on the lastused field?
>> Functionality wise, it should work. Current way is more for containing the
>> set/clear in the same domain, and no need to ask each vendor to take care of
>> this bit.
> No need to ask each vendor, what I mean is to deal with this from
> flow_offload_work_del(), see attached patch.

Oh, I see. That is already covered in my patch as below. Howerver, 
flow_offload_work_del will only be triggered after timeout 
expired(30sec). User will see incorrect CT state within this 30 seconds 
timeframe, which the clear_bit based on lastused can solve it.

  static void flow_offload_work_del(struct flow_offload_work *offload)
  {
+	clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
  	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
  	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
  	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Pablo Neira Ayuso]

On Mon, Apr 20, 2020 at 10:46:54AM -0500, Bodong Wang wrote:
> On 4/20/2020 10:33 AM, Pablo Neira Ayuso wrote:
> > On Mon, Apr 20, 2020 at 10:28:00AM -0500, Bodong Wang wrote:
> > > On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote:
> > > > On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
> > > > [...]
> > > > > @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
> > > > >    				       FLOW_OFFLOAD_DIR_REPLY,
> > > > >    				       stats[1].pkts, stats[1].bytes);
> > > > >    	}
> > > > > +
> > > > > +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
> > > > > +	 * happen in two scenarios:
> > > > > +	 *
> > > > > +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
> > > > > +	 *    HW driver is unloaded.
> > > > > +	 * 2. One of the shared block driver is unloaded.
> > > > > +	 */
> > > > > +	if (!lastused)
> > > > > +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
> > > > >    }
> > > > Better inconditionally clear off the flag after the entry is removed
> > > > from hardware instead of relying on the lastused field?
> > > Functionality wise, it should work. Current way is more for containing the
> > > set/clear in the same domain, and no need to ask each vendor to take care of
> > > this bit.
> > No need to ask each vendor, what I mean is to deal with this from
> > flow_offload_work_del(), see attached patch.
> 
> Oh, I see. That is already covered in my patch as below. Howerver,
> flow_offload_work_del will only be triggered after timeout expired(30sec).
> User will see incorrect CT state within this 30 seconds timeframe, which the
> clear_bit based on lastused can solve it.

For TCP fin/rst the removal from hardware occurs once once the
workqueue has a chance to run.

For UDP, or in case the TCP connection stalls or no packets are seeing
after 30 seconds, then the flow is removed from hardware after 30
seconds.

The IPS_HW_OFFLOAD_BIT flag should be cleaned up when the flow is
effectively removed from hardware.

Why do you want to clean it up earlier than that? With your approach,
the flag is cleared but the flow is still in hardware?

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Bodong Wang]

On 4/20/2020 10:58 AM, Pablo Neira Ayuso wrote:
> On Mon, Apr 20, 2020 at 10:46:54AM -0500, Bodong Wang wrote:
>> On 4/20/2020 10:33 AM, Pablo Neira Ayuso wrote:
>>> On Mon, Apr 20, 2020 at 10:28:00AM -0500, Bodong Wang wrote:
>>>> On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote:
>>>>> On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
>>>>> [...]
>>>>>> @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
>>>>>>     				       FLOW_OFFLOAD_DIR_REPLY,
>>>>>>     				       stats[1].pkts, stats[1].bytes);
>>>>>>     	}
>>>>>> +
>>>>>> +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
>>>>>> +	 * happen in two scenarios:
>>>>>> +	 *
>>>>>> +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
>>>>>> +	 *    HW driver is unloaded.
>>>>>> +	 * 2. One of the shared block driver is unloaded.
>>>>>> +	 */
>>>>>> +	if (!lastused)
>>>>>> +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>>>>>>     }
>>>>> Better inconditionally clear off the flag after the entry is removed
>>>>> from hardware instead of relying on the lastused field?
>>>> Functionality wise, it should work. Current way is more for containing the
>>>> set/clear in the same domain, and no need to ask each vendor to take care of
>>>> this bit.
>>> No need to ask each vendor, what I mean is to deal with this from
>>> flow_offload_work_del(), see attached patch.
>> Oh, I see. That is already covered in my patch as below. Howerver,
>> flow_offload_work_del will only be triggered after timeout expired(30sec).
>> User will see incorrect CT state within this 30 seconds timeframe, which the
>> clear_bit based on lastused can solve it.
> For TCP fin/rst the removal from hardware occurs once once the
> workqueue has a chance to run.
>
> For UDP, or in case the TCP connection stalls or no packets are seeing
> after 30 seconds, then the flow is removed from hardware after 30
> seconds.
>
> The IPS_HW_OFFLOAD_BIT flag should be cleaned up when the flow is
> effectively removed from hardware.
>
> Why do you want to clean it up earlier than that? With your approach,
> the flag is cleared but the flow is still in hardware?

In normally cases(no driver unload, etc), it is imdediately removed by 
flow_offload_work_del. Requests to remove the HW flow are from netfilter 
layer to driver via block_cb->cb. We're well covered in such cases.

The lastused is more for conner cases such as: iperf is still running, 
but driver is unloaded. In such case, driver removed all HW flows 
without notifying netfilter. Flow_offload_work_del will only be called 
once timeout expired, and the indication of HW_OFFLOAD is incorrect 
within the timeout period. Meanwhile, lastused stopped updating once 
driver unloading process destroied flow couters. So, relying on this 
field to clear the HW_OFFLOAD bit to cover such conner cases.

Re: [nf-next] netfilter: nf_conntrack, add IPS_HW_OFFLOAD status bit

[Pablo Neira Ayuso]

On Mon, Apr 20, 2020 at 11:45:44AM -0500, Bodong Wang wrote:
> On 4/20/2020 10:58 AM, Pablo Neira Ayuso wrote:
> > On Mon, Apr 20, 2020 at 10:46:54AM -0500, Bodong Wang wrote:
> > > On 4/20/2020 10:33 AM, Pablo Neira Ayuso wrote:
> > > > On Mon, Apr 20, 2020 at 10:28:00AM -0500, Bodong Wang wrote:
> > > > > On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote:
> > > > > > On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote:
> > > > > > [...]
> > > > > > > @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
> > > > > > >     				       FLOW_OFFLOAD_DIR_REPLY,
> > > > > > >     				       stats[1].pkts, stats[1].bytes);
> > > > > > >     	}
> > > > > > > +
> > > > > > > +	/* Clear HW_OFFLOAD immediately when lastused stopped updating, this can
> > > > > > > +	 * happen in two scenarios:
> > > > > > > +	 *
> > > > > > > +	 * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but
> > > > > > > +	 *    HW driver is unloaded.
> > > > > > > +	 * 2. One of the shared block driver is unloaded.
> > > > > > > +	 */
> > > > > > > +	if (!lastused)
> > > > > > > +		clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
> > > > > > >     }
> > > > > > Better inconditionally clear off the flag after the entry is removed
> > > > > > from hardware instead of relying on the lastused field?
> > > > > Functionality wise, it should work. Current way is more for containing the
> > > > > set/clear in the same domain, and no need to ask each vendor to take care of
> > > > > this bit.
> > > > No need to ask each vendor, what I mean is to deal with this from
> > > > flow_offload_work_del(), see attached patch.
> > > Oh, I see. That is already covered in my patch as below. Howerver,
> > > flow_offload_work_del will only be triggered after timeout expired(30sec).
> > > User will see incorrect CT state within this 30 seconds timeframe, which the
> > > clear_bit based on lastused can solve it.
> > For TCP fin/rst the removal from hardware occurs once once the
> > workqueue has a chance to run.
> > 
> > For UDP, or in case the TCP connection stalls or no packets are seeing
> > after 30 seconds, then the flow is removed from hardware after 30
> > seconds.
> > 
> > The IPS_HW_OFFLOAD_BIT flag should be cleaned up when the flow is
> > effectively removed from hardware.
> > 
> > Why do you want to clean it up earlier than that? With your approach,
> > the flag is cleared but the flow is still in hardware?
> 
> In normally cases(no driver unload, etc), it is imdediately removed by
> flow_offload_work_del. Requests to remove the HW flow are from netfilter
> layer to driver via block_cb->cb. We're well covered in such cases.
> 
> The lastused is more for conner cases such as: iperf is still running, but
> driver is unloaded. In such case, driver removed all HW flows without
> notifying netfilter.

The driver should invoke the flowtable garbage collector let it clean
up the entries before the driver is unloaded.

> Flow_offload_work_del will only be called once timeout expired, and
> the indication of HW_OFFLOAD is incorrect within the timeout period.
> Meanwhile, lastused stopped updating once driver unloading process
> destroied flow couters. So, relying on this field to clear the
> HW_OFFLOAD bit to cover such conner cases.

This corner case is interesting.

Could you submit this patch without the lastused trick? Then, we can
revisit how the driver invokes the garbage collector to deal with the
cleanup?

All this model is based on the garbage collector being the one that is
responsible to cleaning up the flowtable. If there are multiple entry
points to add/remove entries to the flowtable, we'll end up with
complicated locking sooner or later.

Thanks.