userspace conntrack helper and confirming the master conntrack

userspace conntrack helper and confirming the master conntrack

[Michal Kubecek]

Hello,

to clean up some skeletons in the closet of our distribution kernels,
I'm trying to add a userspace conntrack helper for SLP into conntrackd.

A helper is needed to handle SLP queries which are sent as multicast UDP
packets but replied to with unicast packets so that reply's source
address does not much request's destination. This is exactly the same
problem as for mDNS so that I started by copying existing mdns helper in
conntrackd and changing the default timeout. But I found that it does
not work with 5.2 kernel.

The setup looks like this (omitting some log rules):

  nfct helper add slp inet udp
  iptables -t raw -A OUTPUT -m addrtype --dst-type MULTICAST \
      -p udp --dport 427 -j CT --helper slp
  iptables -t raw -A OUTPUT -m addrtype --dst-type BROADCAST
      -p udp --dport 427 -j CT --helper slp
  iptables -A INPUT -i lo -j ACCEPT
  iptables -A INPUT -p tcp --dport 22 -j ACCEPT
  iptables -A INPUT -m conntrack --ctstate ESTABLISHED -j ACCEPT
  iptables -A INPUT -m conntrack --ctstate RELATED -j ACCEPT
  iptables -P INPUT DROP
  iptables -P OUTPUT ACCEPT

The helper rules apply, outgoing multicast packet is sent away but the
unicast reply is not recognized as related and rejected. Monitring with
"conntrack -E expect" shows that an expectation is created but it is
immediately destroyed and "conntrack -E" does not show the conntrack for
the original multicast packet (which is created when I omit the helper
rules in raw table). Kernel side tracing confirms that the conntrack is
never confirmed and inserted into the hash table so that the expectation
is destroyed once the request packet is sent out (and skb_consume()-ed).

I added some more tracing and this is what seems to happen:

  - ipv4_confirm() is called for the conntrack from ip_output() via hook
  - nf_confirm() calls attached helper and calls its help() function
    which is nfnl_userspace_cthelper(), that returns 0x78003
  - nf_confirm() returns that without calling nf_confirm_conntrack()
  - verdict 0x78003 is returned to nf_hook_slow() which therefore calls
    nf_queue() to pass this to userspace helper on queue 7
  - nf_queue() returns 0 which is also returned by nf_hook_slow()
  - the packet reappears in nf_reinject() where it passes through
    nf_reroute() and nf_iterate() to the main switch statement
  - it takes NF_ACCEPT branch to call okfn which is ip_finish_output()
  - unless I missed something, there is nothing that could confirm the
    conntrack after that

Did I forget to do something in the helper (essentially a copy of
existing mdns helper which is supposed to work) or in the setup above
(which is based on the documentation)? Or is this a problem on kernel
side? Should the conntrack be confirmed while processing the original
packet (which ends up queued) or the reinjected one? And where should it
happen?

Thanks in advance,
Michal Kubecek

Re: userspace conntrack helper and confirming the master conntrack

[Florian Westphal]

Michal Kubecek <mkubecek@suse.cz> wrote:
> Hello,
> 
> to clean up some skeletons in the closet of our distribution kernels,
> I'm trying to add a userspace conntrack helper for SLP into conntrackd.
> 
> A helper is needed to handle SLP queries which are sent as multicast UDP
> packets but replied to with unicast packets so that reply's source
> address does not much request's destination. This is exactly the same
> problem as for mDNS so that I started by copying existing mdns helper in
> conntrackd and changing the default timeout. But I found that it does
> not work with 5.2 kernel.
> 
> The setup looks like this (omitting some log rules):
> 
>   nfct helper add slp inet udp
>   iptables -t raw -A OUTPUT -m addrtype --dst-type MULTICAST \
>       -p udp --dport 427 -j CT --helper slp
>   iptables -t raw -A OUTPUT -m addrtype --dst-type BROADCAST
>       -p udp --dport 427 -j CT --helper slp
>   iptables -A INPUT -i lo -j ACCEPT
>   iptables -A INPUT -p tcp --dport 22 -j ACCEPT
>   iptables -A INPUT -m conntrack --ctstate ESTABLISHED -j ACCEPT
>   iptables -A INPUT -m conntrack --ctstate RELATED -j ACCEPT
>   iptables -P INPUT DROP
>   iptables -P OUTPUT ACCEPT
> 
> The helper rules apply, outgoing multicast packet is sent away but the
> unicast reply is not recognized as related and rejected. Monitring with
> "conntrack -E expect" shows that an expectation is created but it is
> immediately destroyed and "conntrack -E" does not show the conntrack for
> the original multicast packet (which is created when I omit the helper
> rules in raw table). Kernel side tracing confirms that the conntrack is
> never confirmed and inserted into the hash table so that the expectation
> is destroyed once the request packet is sent out (and skb_consume()-ed).
> 
> I added some more tracing and this is what seems to happen:
> 
>   - ipv4_confirm() is called for the conntrack from ip_output() via hook
>   - nf_confirm() calls attached helper and calls its help() function
>     which is nfnl_userspace_cthelper(), that returns 0x78003
>   - nf_confirm() returns that without calling nf_confirm_conntrack()
>   - verdict 0x78003 is returned to nf_hook_slow() which therefore calls
>     nf_queue() to pass this to userspace helper on queue 7
>   - nf_queue() returns 0 which is also returned by nf_hook_slow()
>   - the packet reappears in nf_reinject() where it passes through
>     nf_reroute() and nf_iterate() to the main switch statement
>   - it takes NF_ACCEPT branch to call okfn which is ip_finish_output()
>   - unless I missed something, there is nothing that could confirm the
>     conntrack after that

I broke this with
commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad
("netfilter: conntrack: remove helper hook again").

Seems we have to revert, i see no other solution at this time.

Re: userspace conntrack helper and confirming the master conntrack

[Michal Kubecek]

On Thu, Jul 18, 2019 at 11:21:28AM +0200, Florian Westphal wrote:
> > I added some more tracing and this is what seems to happen:
> > 
> >   - ipv4_confirm() is called for the conntrack from ip_output() via hook
> >   - nf_confirm() calls attached helper and calls its help() function
> >     which is nfnl_userspace_cthelper(), that returns 0x78003
> >   - nf_confirm() returns that without calling nf_confirm_conntrack()
> >   - verdict 0x78003 is returned to nf_hook_slow() which therefore calls
> >     nf_queue() to pass this to userspace helper on queue 7
> >   - nf_queue() returns 0 which is also returned by nf_hook_slow()
> >   - the packet reappears in nf_reinject() where it passes through
> >     nf_reroute() and nf_iterate() to the main switch statement
> >   - it takes NF_ACCEPT branch to call okfn which is ip_finish_output()
> >   - unless I missed something, there is nothing that could confirm the
> >     conntrack after that
> 
> I broke this with
> commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad
> ("netfilter: conntrack: remove helper hook again").
> 
> Seems we have to revert, i see no other solution at this time.

Thanks for the quick reply. I can confirm that with commit 827318feb69c
reverted, the helper works as expected.

Michal 

Re: userspace conntrack helper and confirming the master conntrack

[Pablo Neira Ayuso]

On Thu, Jul 18, 2019 at 12:18:06PM +0200, Michal Kubecek wrote:
> On Thu, Jul 18, 2019 at 11:21:28AM +0200, Florian Westphal wrote:
> > > I added some more tracing and this is what seems to happen:
> > > 
> > >   - ipv4_confirm() is called for the conntrack from ip_output() via hook
> > >   - nf_confirm() calls attached helper and calls its help() function
> > >     which is nfnl_userspace_cthelper(), that returns 0x78003
> > >   - nf_confirm() returns that without calling nf_confirm_conntrack()
> > >   - verdict 0x78003 is returned to nf_hook_slow() which therefore calls
> > >     nf_queue() to pass this to userspace helper on queue 7
> > >   - nf_queue() returns 0 which is also returned by nf_hook_slow()
> > >   - the packet reappears in nf_reinject() where it passes through
> > >     nf_reroute() and nf_iterate() to the main switch statement
> > >   - it takes NF_ACCEPT branch to call okfn which is ip_finish_output()
> > >   - unless I missed something, there is nothing that could confirm the
> > >     conntrack after that
> > 
> > I broke this with
> > commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad
> > ("netfilter: conntrack: remove helper hook again").
> > 
> > Seems we have to revert, i see no other solution at this time.
> 
> Thanks for the quick reply. I can confirm that with commit 827318feb69c
> reverted, the helper works as expected.

I'll schedule a revert in the next net batch.

Re: userspace conntrack helper and confirming the master conntrack

[Michal Kubecek]

On Fri, Jul 19, 2019 at 06:47:42PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jul 18, 2019 at 12:18:06PM +0200, Michal Kubecek wrote:
> > On Thu, Jul 18, 2019 at 11:21:28AM +0200, Florian Westphal wrote:
> > > > I added some more tracing and this is what seems to happen:
> > > > 
> > > >   - ipv4_confirm() is called for the conntrack from ip_output() via hook
> > > >   - nf_confirm() calls attached helper and calls its help() function
> > > >     which is nfnl_userspace_cthelper(), that returns 0x78003
> > > >   - nf_confirm() returns that without calling nf_confirm_conntrack()
> > > >   - verdict 0x78003 is returned to nf_hook_slow() which therefore calls
> > > >     nf_queue() to pass this to userspace helper on queue 7
> > > >   - nf_queue() returns 0 which is also returned by nf_hook_slow()
> > > >   - the packet reappears in nf_reinject() where it passes through
> > > >     nf_reroute() and nf_iterate() to the main switch statement
> > > >   - it takes NF_ACCEPT branch to call okfn which is ip_finish_output()
> > > >   - unless I missed something, there is nothing that could confirm the
> > > >     conntrack after that
> > > 
> > > I broke this with
> > > commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad
> > > ("netfilter: conntrack: remove helper hook again").
> > > 
> > > Seems we have to revert, i see no other solution at this time.
> > 
> > Thanks for the quick reply. I can confirm that with commit 827318feb69c
> > reverted, the helper works as expected.
> 
> I'll schedule a revert in the next net batch.

This seems to have fallen through the cracks. I tried to do the revert
but it's not completely straightforward as bridge conntrack has been
introduced in between and I'm not sure I got the bridge part right.
Could someone more familiar with the code take a look?

Michal

---
 include/net/netfilter/nf_conntrack_core.h  |   3 -
 net/bridge/netfilter/nf_conntrack_bridge.c |  89 ++++++++++---
 net/netfilter/nf_conntrack_proto.c         | 141 ++++++++++++++++-----
 3 files changed, 178 insertions(+), 55 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index de10faf2ce91..ae41e92251dd 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -64,9 +64,6 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
 	return ret;
 }
 
-unsigned int nf_confirm(struct sk_buff *skb, unsigned int protoff,
-			struct nf_conn *ct, enum ip_conntrack_info ctinfo);
-
 void print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
 		 const struct nf_conntrack_l4proto *proto);
 
diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 4f5444d2a526..0c54a4e3ead9 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -15,6 +15,7 @@
 #include <net/netfilter/nf_conntrack_core.h>
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_bridge.h>
+#include <net/netfilter/nf_conntrack_seqadj.h>
 
 #include <linux/netfilter/nf_tables.h>
 #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
@@ -353,6 +354,26 @@ static int nf_ct_bridge_refrag_post(struct net *net, struct sock *sk,
 	return br_dev_queue_push_xmit(net, sk, skb);
 }
 
+static int nf_ct_bridge_skb_protoff(struct sk_buff *skb)
+{
+	if (skb->protocol == ETH_P_IP)
+		return skb_network_offset(skb) + ip_hdrlen(skb);
+
+	if (skb->protocol == ETH_P_IPV6) {
+		unsigned char pnum = ipv6_hdr(skb)->nexthdr;
+		__be16 frag_off;
+		int protoff;
+
+		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
+					   &frag_off);
+		if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
+			return -EINVAL;
+		return protoff;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static unsigned int nf_ct_bridge_confirm(struct sk_buff *skb)
 {
 	enum ip_conntrack_info ctinfo;
@@ -361,26 +382,25 @@ static unsigned int nf_ct_bridge_confirm(struct sk_buff *skb)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
-		return nf_conntrack_confirm(skb);
+		goto out;
 
-	switch (skb->protocol) {
-	case htons(ETH_P_IP):
-		protoff = skb_network_offset(skb) + ip_hdrlen(skb);
-		break;
-	case htons(ETH_P_IPV6): {
-		 unsigned char pnum = ipv6_hdr(skb)->nexthdr;
-		__be16 frag_off;
-
-		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
-					   &frag_off);
-		if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
-			return nf_conntrack_confirm(skb);
-		}
-		break;
-	default:
+	protoff = nf_ct_bridge_skb_protoff(skb);
+	if (protoff == -EOPNOTSUPP)
 		return NF_ACCEPT;
+	if (protoff < 0)
+		goto out;
+
+	/* adjust seqs for loopback traffic only in outgoing direction */
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return NF_DROP;
+		}
 	}
-	return nf_confirm(skb, protoff, ct, ctinfo);
+out:
+	/* We've seen it coming out the other side: confirm it */
+	return nf_conntrack_confirm(skb);
 }
 
 static unsigned int nf_ct_bridge_post(void *priv, struct sk_buff *skb,
@@ -395,6 +415,35 @@ static unsigned int nf_ct_bridge_post(void *priv, struct sk_buff *skb,
 	return nf_ct_bridge_refrag(skb, state, nf_ct_bridge_refrag_post);
 }
 
+static unsigned int nf_ct_bridge_helper(void *priv, struct sk_buff *skb,
+					const struct nf_hook_state *state)
+{
+	const struct nf_conntrack_helper *helper;
+	const struct nf_conn_help *help;
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+	int protoff;
+
+	/* This is where we call the helper: as the packet goes out. */
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
+		return NF_ACCEPT;
+
+	help = nfct_help(ct);
+	if (!help)
+		return NF_ACCEPT;
+	/* rcu_read_lock()ed by nf_hook_thresh */
+	helper = rcu_dereference(help->helper);
+	if (!helper)
+		return NF_ACCEPT;
+
+	protoff = nf_ct_bridge_skb_protoff(skb);
+	if (protoff < 0)
+		return NF_ACCEPT;
+
+	return helper->help(skb, protoff, ct, ctinfo);
+}
+
 static struct nf_hook_ops nf_ct_bridge_hook_ops[] __read_mostly = {
 	{
 		.hook		= nf_ct_bridge_pre,
@@ -402,6 +451,12 @@ static struct nf_hook_ops nf_ct_bridge_hook_ops[] __read_mostly = {
 		.hooknum	= NF_BR_PRE_ROUTING,
 		.priority	= NF_IP_PRI_CONNTRACK,
 	},
+	{
+		.hook		= nf_ct_bridge_helper,
+		.pf		= NFPROTO_BRIDGE,
+		.hooknum	= NF_BR_POST_ROUTING,
+		.priority	= NF_IP_PRI_CONNTRACK_HELPER,
+	},
 	{
 		.hook		= nf_ct_bridge_post,
 		.pf		= NFPROTO_BRIDGE,
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index a0560d175a7f..a000725bb248 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -121,54 +121,55 @@ const struct nf_conntrack_l4proto *nf_ct_l4proto_find(u8 l4proto)
 };
 EXPORT_SYMBOL_GPL(nf_ct_l4proto_find);
 
-unsigned int nf_confirm(struct sk_buff *skb, unsigned int protoff,
-			struct nf_conn *ct, enum ip_conntrack_info ctinfo)
+static unsigned int ipv4_helper(void *priv,
+				struct sk_buff *skb,
+				const struct nf_hook_state *state)
 {
+	struct nf_conn *ct;
+	enum ip_conntrack_info ctinfo;
 	const struct nf_conn_help *help;
+	const struct nf_conntrack_helper *helper;
+
+	/* This is where we call the helper: as the packet goes out. */
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
+		return NF_ACCEPT;
 
 	help = nfct_help(ct);
-	if (help) {
-		const struct nf_conntrack_helper *helper;
-		int ret;
-
-		/* rcu_read_lock()ed by nf_hook_thresh */
-		helper = rcu_dereference(help->helper);
-		if (helper) {
-			ret = helper->help(skb,
-					   protoff,
-					   ct, ctinfo);
-			if (ret != NF_ACCEPT)
-				return ret;
-		}
-	}
+	if (!help)
+		return NF_ACCEPT;
 
-	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
-	    !nf_is_loopback_packet(skb)) {
-		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
-			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
-			return NF_DROP;
-		}
-	}
+	/* rcu_read_lock()ed by nf_hook_thresh */
+	helper = rcu_dereference(help->helper);
+	if (!helper)
+		return NF_ACCEPT;
 
-	/* We've seen it coming out the other side: confirm it */
-	return nf_conntrack_confirm(skb);
+	return helper->help(skb, skb_network_offset(skb) + ip_hdrlen(skb),
+			    ct, ctinfo);
 }
-EXPORT_SYMBOL_GPL(nf_confirm);
 
 static unsigned int ipv4_confirm(void *priv,
 				 struct sk_buff *skb,
 				 const struct nf_hook_state *state)
 {
-	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct;
+	enum ip_conntrack_info ctinfo;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
-		return nf_conntrack_confirm(skb);
+		goto out;
 
-	return nf_confirm(skb,
-			  skb_network_offset(skb) + ip_hdrlen(skb),
-			  ct, ctinfo);
+	/* adjust seqs for loopback traffic only in outgoing direction */
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, ip_hdrlen(skb))) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return NF_DROP;
+		}
+	}
+out:
+	/* We've seen it coming out the other side: confirm it */
+	return nf_conntrack_confirm(skb);
 }
 
 static unsigned int ipv4_conntrack_in(void *priv,
@@ -216,12 +217,24 @@ static const struct nf_hook_ops ipv4_conntrack_ops[] = {
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP_PRI_CONNTRACK,
 	},
+	{
+		.hook		= ipv4_helper,
+		.pf		= NFPROTO_IPV4,
+		.hooknum	= NF_INET_POST_ROUTING,
+		.priority	= NF_IP_PRI_CONNTRACK_HELPER,
+	},
 	{
 		.hook		= ipv4_confirm,
 		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_POST_ROUTING,
 		.priority	= NF_IP_PRI_CONNTRACK_CONFIRM,
 	},
+	{
+		.hook		= ipv4_helper,
+		.pf		= NFPROTO_IPV4,
+		.hooknum	= NF_INET_LOCAL_IN,
+		.priority	= NF_IP_PRI_CONNTRACK_HELPER,
+	},
 	{
 		.hook		= ipv4_confirm,
 		.pf		= NFPROTO_IPV4,
@@ -367,21 +380,31 @@ static unsigned int ipv6_confirm(void *priv,
 	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
 	unsigned char pnum = ipv6_hdr(skb)->nexthdr;
-	__be16 frag_off;
 	int protoff;
+	__be16 frag_off;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
-		return nf_conntrack_confirm(skb);
+		goto out;
 
 	protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
 				   &frag_off);
 	if (protoff < 0 || (frag_off & htons(~0x7)) != 0) {
 		pr_debug("proto header not found\n");
-		return nf_conntrack_confirm(skb);
+		goto out;
 	}
 
-	return nf_confirm(skb, protoff, ct, ctinfo);
+	/* adjust seqs for loopback traffic only in outgoing direction */
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return NF_DROP;
+		}
+	}
+out:
+	/* We've seen it coming out the other side: confirm it */
+	return nf_conntrack_confirm(skb);
 }
 
 static unsigned int ipv6_conntrack_in(void *priv,
@@ -398,6 +421,42 @@ static unsigned int ipv6_conntrack_local(void *priv,
 	return nf_conntrack_in(skb, state);
 }
 
+static unsigned int ipv6_helper(void *priv,
+				struct sk_buff *skb,
+				const struct nf_hook_state *state)
+{
+	struct nf_conn *ct;
+	const struct nf_conn_help *help;
+	const struct nf_conntrack_helper *helper;
+	enum ip_conntrack_info ctinfo;
+	__be16 frag_off;
+	int protoff;
+	u8 nexthdr;
+
+	/* This is where we call the helper: as the packet goes out. */
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct || ctinfo == IP_CT_RELATED_REPLY)
+		return NF_ACCEPT;
+
+	help = nfct_help(ct);
+	if (!help)
+		return NF_ACCEPT;
+	/* rcu_read_lock()ed by nf_hook_thresh */
+	helper = rcu_dereference(help->helper);
+	if (!helper)
+		return NF_ACCEPT;
+
+	nexthdr = ipv6_hdr(skb)->nexthdr;
+	protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
+				   &frag_off);
+	if (protoff < 0 || (frag_off & htons(~0x7)) != 0) {
+		pr_debug("proto header not found\n");
+		return NF_ACCEPT;
+	}
+
+	return helper->help(skb, protoff, ct, ctinfo);
+}
+
 static const struct nf_hook_ops ipv6_conntrack_ops[] = {
 	{
 		.hook		= ipv6_conntrack_in,
@@ -411,12 +470,24 @@ static const struct nf_hook_ops ipv6_conntrack_ops[] = {
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP6_PRI_CONNTRACK,
 	},
+	{
+		.hook		= ipv6_helper,
+		.pf		= NFPROTO_IPV6,
+		.hooknum	= NF_INET_POST_ROUTING,
+		.priority	= NF_IP6_PRI_CONNTRACK_HELPER,
+	},
 	{
 		.hook		= ipv6_confirm,
 		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_POST_ROUTING,
 		.priority	= NF_IP6_PRI_LAST,
 	},
+	{
+		.hook		= ipv6_helper,
+		.pf		= NFPROTO_IPV6,
+		.hooknum	= NF_INET_LOCAL_IN,
+		.priority	= NF_IP6_PRI_CONNTRACK_HELPER,
+	},
 	{
 		.hook		= ipv6_confirm,
 		.pf		= NFPROTO_IPV6,
-- 
2.23.0

Re: userspace conntrack helper and confirming the master conntrack

[Pablo Neira Ayuso]

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

Hi Michal,

On Wed, Sep 04, 2019 at 02:16:51PM +0200, Michal Kubecek wrote:
> This seems to have fallen through the cracks. I tried to do the revert
> but it's not completely straightforward as bridge conntrack has been
> introduced in between and I'm not sure I got the bridge part right.
> Could someone more familiar with the code take a look?

I'm exploring a different path, see attached patch (still untested).

I'm trying to avoid this large revert from Florian. The idea with this
patch is to invoke the conntrack confirmation path from the
nf_reinject() path, which is what it is missing.

I'm at a conference right now, I'll try scratch time to sort out this
asap. Most likely we'll have to request a patch to be included in
-stable in the next release I'm afraid.

I'm very sorry this has taken a bit to be sorted out.

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

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 049aeb40fa35..e4047aae0da7 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -448,6 +448,7 @@ struct nf_ct_hook {
 	void (*destroy)(struct nf_conntrack *);
 	bool (*get_tuple_skb)(struct nf_conntrack_tuple *,
 			      const struct sk_buff *);
+	unsigned int (*confirm)(struct sk_buff *);
 };
 extern struct nf_ct_hook __rcu *nf_ct_hook;
 
diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index de10faf2ce91..42407086484f 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -66,6 +66,7 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
 
 unsigned int nf_confirm(struct sk_buff *skb, unsigned int protoff,
 			struct nf_conn *ct, enum ip_conntrack_info ctinfo);
+unsigned int nf_confirm_queue(struct sk_buff *skb);
 
 void print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
 		 const struct nf_conntrack_l4proto *proto);
diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index 3cb6dcf53a4e..46bbd2389ffc 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -6,12 +6,16 @@
 #include <linux/ipv6.h>
 #include <linux/jhash.h>
 
+/* extra verdict flags have mask 0x0000ff00, see uapi/linux/netfilter.h */
+#define __NF_VERDICT_FLAG_USERSPACE_CT_HELPER	0x00004000
+
 /* Each queued (to userspace) skbuff has one of these. */
 struct nf_queue_entry {
 	struct list_head	list;
 	struct sk_buff		*skb;
 	unsigned int		id;
 	unsigned int		hook_index;	/* index in hook_entries->hook[] */
+	unsigned int		queue_flags;
 
 	struct nf_hook_state	state;
 	u16			size; /* sizeof(entry) + saved route keys */
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 81a8ef42b88d..5238dc014156 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2487,6 +2487,7 @@ static struct nf_ct_hook nf_conntrack_hook = {
 	.update		= nf_conntrack_update,
 	.destroy	= destroy_conntrack,
 	.get_tuple_skb  = nf_conntrack_get_tuple_skb,
+	.confirm	= nf_confirm_queue,
 };
 
 void nf_conntrack_init_end(void)
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index a0560d175a7f..493c2e12434a 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -121,6 +121,53 @@ const struct nf_conntrack_l4proto *nf_ct_l4proto_find(u8 l4proto)
 };
 EXPORT_SYMBOL_GPL(nf_ct_l4proto_find);
 
+static unsigned int __nf_confirm(struct sk_buff *skb, unsigned int protoff,
+				 struct nf_conn *ct,
+				 enum ip_conntrack_info ctinfo)
+{
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return NF_DROP;
+		}
+	}
+
+	/* We've seen it coming out the other side: confirm it */
+	return nf_conntrack_confirm(skb);
+}
+
+unsigned int nf_confirm_queue(struct sk_buff *skb)
+{
+	enum ip_conntrack_info ctinfo;
+	unsigned int protoff;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct)
+		return NF_ACCEPT;
+
+	switch (skb->protocol) {
+	case htons(ETH_P_IP):
+		protoff = skb_network_offset(skb) + ip_hdrlen(skb);
+		break;
+	case htons(ETH_P_IPV6): {
+		 unsigned char pnum = ipv6_hdr(skb)->nexthdr;
+		__be16 frag_off;
+
+		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
+					   &frag_off);
+		if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
+			return nf_conntrack_confirm(skb);
+		}
+		break;
+	default:
+		return NF_ACCEPT;
+	}
+
+	return __nf_confirm(skb, protoff, ct, ctinfo);
+}
+
 unsigned int nf_confirm(struct sk_buff *skb, unsigned int protoff,
 			struct nf_conn *ct, enum ip_conntrack_info ctinfo)
 {
@@ -142,16 +189,7 @@ unsigned int nf_confirm(struct sk_buff *skb, unsigned int protoff,
 		}
 	}
 
-	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
-	    !nf_is_loopback_packet(skb)) {
-		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
-			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
-			return NF_DROP;
-		}
-	}
-
-	/* We've seen it coming out the other side: confirm it */
-	return nf_conntrack_confirm(skb);
+	return __nf_confirm(skb, protoff, ct, ctinfo);
 }
 EXPORT_SYMBOL_GPL(nf_confirm);
 
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index a2b58de82600..85ff51cadd2a 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -156,7 +156,8 @@ static void nf_ip6_saveroute(const struct sk_buff *skb,
 }
 
 static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
-		      unsigned int index, unsigned int queuenum)
+		      unsigned int index, unsigned int queuenum,
+		      unsigned int queue_flags)
 {
 	int status = -ENOENT;
 	struct nf_queue_entry *entry = NULL;
@@ -198,6 +199,7 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		.skb	= skb,
 		.state	= *state,
 		.hook_index = index,
+		.queue_flags = queue_flags,
 		.size	= sizeof(*entry) + route_key_size,
 	};
 
@@ -232,7 +234,8 @@ int nf_queue(struct sk_buff *skb, struct nf_hook_state *state,
 {
 	int ret;
 
-	ret = __nf_queue(skb, state, index, verdict >> NF_VERDICT_QBITS);
+	ret = __nf_queue(skb, state, index, verdict >> NF_VERDICT_QBITS,
+			 verdict & __NF_VERDICT_FLAG_USERSPACE_CT_HELPER);
 	if (ret < 0) {
 		if (ret == -ESRCH &&
 		    (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
@@ -324,6 +327,14 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 			verdict = NF_DROP;
 	}
 
+	if (verdict == NF_ACCEPT &&
+	    entry->queue_flags & __NF_VERDICT_FLAG_USERSPACE_CT_HELPER) {
+		struct nf_ct_hook *ct_hook = rcu_dereference(nf_ct_hook);
+
+		if (ct_hook)
+			verdict = nf_ct_hook->confirm(skb);
+	}
+
 	if (verdict == NF_ACCEPT) {
 next_hook:
 		++i;
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 7525063c25f5..2bf3b7c3ab9e 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -21,7 +21,7 @@
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_expect.h>
 #include <net/netfilter/nf_conntrack_ecache.h>
-
+#include <net/netfilter/nf_queue.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nfnetlink_conntrack.h>
 #include <linux/netfilter/nfnetlink_cthelper.h>
@@ -60,7 +60,8 @@ nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
 		return NF_ACCEPT;
 
 	/* If the user-space helper is not available, don't block traffic. */
-	return NF_QUEUE_NR(helper->queue_num) | NF_VERDICT_FLAG_QUEUE_BYPASS;
+	return NF_QUEUE_NR(helper->queue_num) | NF_VERDICT_FLAG_QUEUE_BYPASS |
+		__NF_VERDICT_FLAG_USERSPACE_CT_HELPER;
 }
 
 static const struct nla_policy nfnl_cthelper_tuple_pol[NFCTH_TUPLE_MAX+1] = {

Re: userspace conntrack helper and confirming the master conntrack

[Michal Kubecek]

On Wed, Sep 11, 2019 at 01:24:26AM +0200, Pablo Neira Ayuso wrote:
> Hi Michal,
> 
> On Wed, Sep 04, 2019 at 02:16:51PM +0200, Michal Kubecek wrote:
> > This seems to have fallen through the cracks. I tried to do the revert
> > but it's not completely straightforward as bridge conntrack has been
> > introduced in between and I'm not sure I got the bridge part right.
> > Could someone more familiar with the code take a look?
> 
> I'm exploring a different path, see attached patch (still untested).
> 
> I'm trying to avoid this large revert from Florian. The idea with this
> patch is to invoke the conntrack confirmation path from the
> nf_reinject() path, which is what it is missing.

Thank you for looking into it. I'll take a look at your patch.

> I'm at a conference right now, I'll try scratch time to sort out this
> asap. Most likely we'll have to request a patch to be included in
> -stable in the next release I'm afraid.

As the regression didn't happen in this cycle but in 5.1-rc1, there are 
already two releases affected so that it's IMHO more important to get it
right than to catch 5.3 at any cost.

Michal

Re: userspace conntrack helper and confirming the master conntrack

[Jacob Rasmussen]

From: Jacob Rasmussen <jacobraz@chromium.org>

Hello everyone,

I'm encountering a bug on Chrome OS that I believe to be caused by commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad and I was wondering if there was any update on a fix/revert landing for this. 
If a fix has already landed would anyone mind pointing me to the specific commit because I haven't been able to find it on my own.

Thanks,
Jacob

Re: userspace conntrack helper and confirming the master conntrack

[Florian Westphal]

Jacob Rasmussen <jacobraz@chromium.org> wrote:
> I'm encountering a bug on Chrome OS that I believe to be caused by commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad and I was wondering if there was any update on a fix/revert landing for this. 
> If a fix has already landed would anyone mind pointing me to the specific commit because I haven't been able to find it on my own.

Right, sorry about this.
Its most definitely caused by 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad.

Unfortunately I have no easy way to validate the fix.
If you could help speeding this up it would be great if you could test
the candidate fix which I will send in a minute.

I've checked that it doesn't break conntrack in obvious ways and that
nft_queue.sh self test still passes.

[PATCH nf] netfilter: make conntrack userspace helpers work again

[Florian Westphal]

conntrackd has support for userspace-based connection tracking helpers,
to move parsing of control packets to userspace.

It works by registering a userspace helper using 'nfct' tool with the kernel, e.g.

nfct add helper rpc inet udp

Then, you need to set up iptables' rules to assign the new userspace
helper:

iptables -I OUTPUT -t raw -p udp --dport 111 -j CT --helper rpc

or, use nft:
ct helper rpchelper {
    type "rpc" protocol udp;
    l3proto inet
}

udp dport 111 ct helper set "rpchelper"

After rules are in place to assign the helper to the connection,
conntrackd is configured:

Helper {
  Type rpc inet udp {
     QueueNum 1
     QueueLen 1024
     Policy rpc {
         ExpectMax 1
         ExpectTimeout 300
         }
    }
}

Conntrack then tells the kernel to queue packets from the generic
'userspace' conntrack helper to userspace using the given queue number.

Just like with normal 'nfqueue' use, skbs leave the netfilter hook
context.  When userspace (conntrackd) sends back the accept verdict,
the skb gets reinjected into the netfilter machinery.

Problem is that after the helper hook was merged back into the confirm
one, the queueing itself occurs from the confirm hook, i.e. we queue
from the last netfilter callback in the hook-list.

Therefore, on return, the packet bypasses the confirm action and the
connection is never committed to the main conntrack table.

To fix this there are several ways:
1. revert the 'Fixes' commit and have a extra helper hook again.
   Works, but has the drawback of adding another indirect call for
   everyone.

2. Special case this: split the hooks only when userspace helper
   gets added, so queueing occurs at a lower priority again,
   and normal nqueue reinject would eventually call the last hook.

3. Extend the existing nf_queue ct update hook to allow a forced
   confirmation (plus run the seqadj code).

This goes for 3).

Based on an earlier patch from Pablo Neira Ayuso.
Compile tested only, nft_queue.sh test still passes.

Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netfilter.h          |  3 +-
 include/net/netfilter/nf_queue.h   |  3 +-
 include/uapi/linux/netfilter.h     |  3 +-
 net/netfilter/nf_conntrack_core.c  | 67 +++++++++++++++++++++++++-----
 net/netfilter/nf_queue.c           |  7 +++-
 net/netfilter/nfnetlink_cthelper.c |  4 +-
 net/netfilter/nfnetlink_queue.c    | 26 ++++++++----
 7 files changed, 89 insertions(+), 24 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index eb312e7ca36e..727705f71828 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -465,7 +465,8 @@ struct nf_conn;
 enum ip_conntrack_info;
 
 struct nf_ct_hook {
-	int (*update)(struct net *net, struct sk_buff *skb);
+	int (*update)(struct net *net, struct sk_buff *skb,
+		      u16 queue_flags);
 	void (*destroy)(struct nf_conntrack *);
 	bool (*get_tuple_skb)(struct nf_conntrack_tuple *,
 			      const struct sk_buff *);
diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index e770bba00066..c8ddbdfa54d3 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -13,7 +13,8 @@ struct nf_queue_entry {
 	struct list_head	list;
 	struct sk_buff		*skb;
 	unsigned int		id;
-	unsigned int		hook_index;	/* index in hook_entries->hook[] */
+	u16			hook_index;	/* index in hook_entries->hook[] */
+	u16			queue_flags;	/* extra skb flags passed at NF_QUEUE time */
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	struct net_device	*physin;
 	struct net_device	*physout;
diff --git a/include/uapi/linux/netfilter.h b/include/uapi/linux/netfilter.h
index ca9e63d6e0e4..aaca2c2e1b87 100644
--- a/include/uapi/linux/netfilter.h
+++ b/include/uapi/linux/netfilter.h
@@ -22,7 +22,8 @@
 #define NF_VERDICT_MASK 0x000000ff
 
 /* extra verdict flags have mask 0x0000ff00 */
-#define NF_VERDICT_FLAG_QUEUE_BYPASS	0x00008000
+#define NF_VERDICT_FLAG_USERSPACE_CT_HELPER	0x00004000
+#define NF_VERDICT_FLAG_QUEUE_BYPASS		0x00008000
 
 /* queue number (NF_QUEUE) or errno (NF_DROP) */
 #define NF_VERDICT_QMASK 0xffff0000
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 1d57b95d3481..8480dc821f2e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2016,7 +2016,51 @@ static void nf_conntrack_attach(struct sk_buff *nskb, const struct sk_buff *skb)
 	nf_conntrack_get(skb_nfct(nskb));
 }
 
-static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
+static unsigned int nf_confirm_cthelper(struct sk_buff *skb, u16 flags)
+{
+	enum ip_conntrack_info ctinfo;
+	u8 pnum __maybe_unused;
+	unsigned int protoff;
+	struct nf_conn *ct;
+
+	if ((flags & NF_VERDICT_FLAG_USERSPACE_CT_HELPER) == 0)
+		return NF_ACCEPT;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct)
+		return NF_ACCEPT;
+
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return NF_DROP;
+		}
+	}
+
+	switch (nf_ct_l3num(ct)) {
+	case NFPROTO_IPV4:
+		protoff = skb_network_offset(skb) + ip_hdrlen(skb);
+		break;
+#if IS_ENABLED(CONFIG_IPV6)
+	case NFPROTO_IPV6:
+		pnum = ipv6_hdr(skb)->nexthdr;
+		__be16 frag_off;
+
+		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
+					   &frag_off);
+		if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
+			return NF_ACCEPT;
+		break;
+#endif
+	default:
+		return NF_ACCEPT;
+	}
+
+	return nf_confirm(skb, protoff, ct, ctinfo);
+}
+
+static int nf_conntrack_update(struct net *net, struct sk_buff *skb, u16 queue_flags)
 {
 	struct nf_conntrack_tuple_hash *h;
 	struct nf_conntrack_tuple tuple;
@@ -2029,18 +2073,21 @@ static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
 	u8 l4num;
 
 	ct = nf_ct_get(skb, &ctinfo);
-	if (!ct || nf_ct_is_confirmed(ct))
-		return 0;
+	if (!ct)
+		return NF_ACCEPT;
+
+	if (nf_ct_is_confirmed(ct))
+		return nf_confirm_cthelper(skb, queue_flags);
 
 	l3num = nf_ct_l3num(ct);
 
 	dataoff = get_l4proto(skb, skb_network_offset(skb), l3num, &l4num);
 	if (dataoff <= 0)
-		return -1;
+		return NF_DROP;
 
 	if (!nf_ct_get_tuple(skb, skb_network_offset(skb), dataoff, l3num,
 			     l4num, net, &tuple))
-		return -1;
+		return NF_DROP;
 
 	if (ct->status & IPS_SRC_NAT) {
 		memcpy(tuple.src.u3.all,
@@ -2060,7 +2107,7 @@ static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
 
 	h = nf_conntrack_find_get(net, nf_ct_zone(ct), &tuple);
 	if (!h)
-		return 0;
+		return nf_confirm_cthelper(skb, queue_flags);
 
 	/* Store status bits of the conntrack that is clashing to re-do NAT
 	 * mangling according to what it has been done already to this packet.
@@ -2073,19 +2120,19 @@ static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
 
 	nat_hook = rcu_dereference(nf_nat_hook);
 	if (!nat_hook)
-		return 0;
+		return nf_confirm_cthelper(skb, queue_flags);
 
 	if (status & IPS_SRC_NAT &&
 	    nat_hook->manip_pkt(skb, ct, NF_NAT_MANIP_SRC,
 				IP_CT_DIR_ORIGINAL) == NF_DROP)
-		return -1;
+		return NF_DROP;
 
 	if (status & IPS_DST_NAT &&
 	    nat_hook->manip_pkt(skb, ct, NF_NAT_MANIP_DST,
 				IP_CT_DIR_ORIGINAL) == NF_DROP)
-		return -1;
+		return NF_DROP;
 
-	return 0;
+	return nf_confirm_cthelper(skb, queue_flags);
 }
 
 static bool nf_conntrack_get_tuple_skb(struct nf_conntrack_tuple *dst_tuple,
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index bbd1209694b8..722846821e10 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -153,7 +153,7 @@ static void nf_ip6_saveroute(const struct sk_buff *skb,
 }
 
 static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
-		      unsigned int index, unsigned int queuenum)
+		      u16 index, u16 queue_flags, unsigned int queuenum)
 {
 	struct nf_queue_entry *entry = NULL;
 	const struct nf_queue_handler *qh;
@@ -191,6 +191,7 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		.skb	= skb,
 		.state	= *state,
 		.hook_index = index,
+		.queue_flags = queue_flags,
 		.size	= sizeof(*entry) + route_key_size,
 	};
 
@@ -222,7 +223,9 @@ int nf_queue(struct sk_buff *skb, struct nf_hook_state *state,
 {
 	int ret;
 
-	ret = __nf_queue(skb, state, index, verdict >> NF_VERDICT_QBITS);
+	ret = __nf_queue(skb, state, index,
+			 verdict & NF_VERDICT_FLAG_USERSPACE_CT_HELPER,
+			 verdict >> NF_VERDICT_QBITS);
 	if (ret < 0) {
 		if (ret == -ESRCH &&
 		    (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index a5f294aa8e4c..55aa5bacc603 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -60,7 +60,9 @@ nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
 		return NF_ACCEPT;
 
 	/* If the user-space helper is not available, don't block traffic. */
-	return NF_QUEUE_NR(helper->queue_num) | NF_VERDICT_FLAG_QUEUE_BYPASS;
+	return NF_QUEUE_NR(helper->queue_num) |
+	       NF_VERDICT_FLAG_QUEUE_BYPASS |
+	       NF_VERDICT_FLAG_USERSPACE_CT_HELPER;
 }
 
 static const struct nla_policy nfnl_cthelper_tuple_pol[NFCTH_TUPLE_MAX+1] = {
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 3243a31f6e82..ddb4a32f6ba5 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -223,22 +223,32 @@ find_dequeue_entry(struct nfqnl_instance *queue, unsigned int id)
 	return entry;
 }
 
+static bool nfqnl_skb_needs_ct_update(struct sk_buff *skb)
+{
+	enum ip_conntrack_info ctinfo;
+	const struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
+
+	return ct && !nf_ct_is_confirmed(ct);
+}
+
 static void nfqnl_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 {
 	struct nf_ct_hook *ct_hook;
-	int err;
 
 	if (verdict == NF_ACCEPT ||
 	    verdict == NF_REPEAT ||
 	    verdict == NF_STOP) {
-		rcu_read_lock();
+		bool nf_ct_helper = false;
+
 		ct_hook = rcu_dereference(nf_ct_hook);
-		if (ct_hook) {
-			err = ct_hook->update(entry->state.net, entry->skb);
-			if (err < 0)
-				verdict = NF_DROP;
-		}
-		rcu_read_unlock();
+		if (!ct_hook)
+			return nf_reinject(entry, verdict);
+
+		nf_ct_helper = entry->queue_flags & NF_VERDICT_FLAG_USERSPACE_CT_HELPER;
+
+		if (nfqnl_skb_needs_ct_update(entry->skb) || nf_ct_helper)
+			verdict = ct_hook->update(entry->state.net, entry->skb,
+						  entry->queue_flags);
 	}
 	nf_reinject(entry, verdict);
 }
-- 
2.26.2

Re: userspace conntrack helper and confirming the master conntrack

[Michal Kubecek]

On Fri, May 15, 2020 at 04:36:32PM +0200, Florian Westphal wrote:
> Jacob Rasmussen <jacobraz@chromium.org> wrote:
> > I'm encountering a bug on Chrome OS that I believe to be caused by commit 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad and I was wondering if there was any update on a fix/revert landing for this. 
> > If a fix has already landed would anyone mind pointing me to the specific commit because I haven't been able to find it on my own.
> 
> Right, sorry about this.
> Its most definitely caused by 827318feb69cb07ed58bb9b9dd6c2eaa81a116ad.
> 
> Unfortunately I have no easy way to validate the fix.
> If you could help speeding this up it would be great if you could test
> the candidate fix which I will send in a minute.
> 
> I've checked that it doesn't break conntrack in obvious ways and that
> nft_queue.sh self test still passes.

Ah, my fault, I completely forgot this. Hopefully I'll be able to test
the fix this weekend.

Michal