Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* userspace conntrack helper and confirming the master conntrack
@ 2019-07-18  8:49 Michal Kubecek
  2019-07-18  9:21 ` Florian Westphal
  0 siblings, 1 reply; 7+ messages in thread
From: Michal Kubecek @ 2019-07-18  8:49 UTC (permalink / raw)
  To: netfilter-devel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: userspace conntrack helper and confirming the master conntrack
  2019-07-18  8:49 userspace conntrack helper and confirming the master conntrack Michal Kubecek
@ 2019-07-18  9:21 ` Florian Westphal
  2019-07-18 10:18   ` Michal Kubecek
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2019-07-18  9:21 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: userspace conntrack helper and confirming the master conntrack
  2019-07-18  9:21 ` Florian Westphal
@ 2019-07-18 10:18   ` Michal Kubecek
  2019-07-19 16:47     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Michal Kubecek @ 2019-07-18 10:18 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

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 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: userspace conntrack helper and confirming the master conntrack
  2019-07-18 10:18   ` Michal Kubecek
@ 2019-07-19 16:47     ` Pablo Neira Ayuso
  2019-09-04 12:16       ` Michal Kubecek
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-07-19 16:47 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: Florian Westphal, netfilter-devel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: userspace conntrack helper and confirming the master conntrack
  2019-07-19 16:47     ` Pablo Neira Ayuso
@ 2019-09-04 12:16       ` Michal Kubecek
  2019-09-10 23:24         ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Michal Kubecek @ 2019-09-04 12:16 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso, Florian Westphal

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: userspace conntrack helper and confirming the master conntrack
  2019-09-04 12:16       ` Michal Kubecek
@ 2019-09-10 23:24         ` Pablo Neira Ayuso
  2019-09-11  8:17           ` Michal Kubecek
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-10 23:24 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: netfilter-devel, Florian Westphal

[-- 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] = {

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: userspace conntrack helper and confirming the master conntrack
  2019-09-10 23:24         ` Pablo Neira Ayuso
@ 2019-09-11  8:17           ` Michal Kubecek
  0 siblings, 0 replies; 7+ messages in thread
From: Michal Kubecek @ 2019-09-11  8:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso, Florian Westphal

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-18  8:49 userspace conntrack helper and confirming the master conntrack Michal Kubecek
2019-07-18  9:21 ` Florian Westphal
2019-07-18 10:18   ` Michal Kubecek
2019-07-19 16:47     ` Pablo Neira Ayuso
2019-09-04 12:16       ` Michal Kubecek
2019-09-10 23:24         ` Pablo Neira Ayuso
2019-09-11  8:17           ` Michal Kubecek

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org netfilter-devel@archiver.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox