Re: [PATCH nf-next RFC,v2 1/6] netfilter: nf_conntrack: add IPS_OFFLOAD status bit

On Fri, Dec 08, 2017 at 07:47:02AM +0100, Florian Westphal wrote:

Pablo Neira Ayuso [off-list ref] wrote:

quoted

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
index dc947e59d03a..6b463b88182d 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h

@@ -100,6 +100,10 @@ enum ip_conntrack_status {
 	IPS_HELPER_BIT = 13,
 	IPS_HELPER = (1 << IPS_HELPER_BIT),
 
+	/* Conntrack has been offloaded to flow table. */
+	IPS_OFFLOAD_BIT = 14,
+	IPS_OFFLOAD = (1 << IPS_OFFLOAD_BIT),
+
 	/* Be careful here, modifying these bits can make things messy,
 	 * so don't let users modify them directly.
 	 */

I think this new bit has to be added to the UNCHANGEABLE mask below.

Right.

quoted

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 01130392b7c0..02e195accd47 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c

@@ -901,6 +901,9 @@ static unsigned int early_drop_list(struct net *net,
 	hlist_nulls_for_each_entry_rcu(h, n, head, hnnode) {
 		tmp = nf_ct_tuplehash_to_ctrack(h);
 
+		if (test_bit(IPS_OFFLOAD_BIT, &tmp->status))
+			continue;
+

nit: I would move this below the ASSURED bit check, AFAIU most
(all?) offloaded conntracks are not in ASSURED state since they never
see two-way communication but in case we've mixed flows or no offloading
in place then the ASSURED check takes care of skipping earlydrop
already.

Offload happens once we enter established state (or later if your rule
postpone it to a later stage), that is before we observe the full
3-way handshake in tcp, hence the assured bit. So I think we still
need this here.

quoted

+/* 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)
+{
+       ct->timeout = nfct_time_stamp + DAY;
+}

Not sure if its worth adding a test to avoid unconditional write,
e.g. something like

quoted

+static void nf_ct_offload_timeout(struct nf_conn *ct)
+{

	if (nf_ct_expires(ct) < DAY/2))

quoted

+       ct->timeout = nfct_time_stamp + DAY;

but perhaps not worth it, gc_worker is infrequent.

That's fine, I'll do this.