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
