diff mbox

netfilter: conntrack: Fix ifdef checks for CONFIG_NF_CONNTRACK_MARK

Message ID 1481751357-22893-1-git-send-email-joseph.j.conley@gmail.com
State Not Applicable
Delegated to: Pablo Neira
Headers show

Commit Message

joseph.j.conley@gmail.com Dec. 14, 2016, 9:35 p.m. UTC 
From: Joe Conley <joe.conley@lairdtech.com>

Two missing ifdef checks for CONFIG_NF_CONNTRACK_MARK were causing
EOPNOTSUPP to be returned. Every single place that cda[CTA_MARK] or cda[CTA_MARK_MASK]
was checked was inside a #ifdef for CONFIG_NF_CONNTRACK_MARK except for these
two places. The reason for this change stems from this commit:
866476f323465a8afef10b14b48d5136bf5c51fe (netfilter: conntrack: Flush connections with a given mark)

This allows conntrack -L to be ran succesfully when CONFIG_NF_CONNTRACK_MARK
is not enabled.

Signed-off-by: Joe Conley <joe.conley@lairdtech.com>
---
 net/netfilter/nf_conntrack_netlink.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pablo Neira Ayuso Dec. 15, 2016, 8:55 p.m. UTC | #1 
On Wed, Dec 14, 2016 at 04:35:57PM -0500, joseph.j.conley@gmail.com wrote:
> From: Joe Conley <joe.conley@lairdtech.com>
> 
> Two missing ifdef checks for CONFIG_NF_CONNTRACK_MARK were causing
> EOPNOTSUPP to be returned. Every single place that cda[CTA_MARK] or cda[CTA_MARK_MASK]
> was checked was inside a #ifdef for CONFIG_NF_CONNTRACK_MARK except for these
> two places. The reason for this change stems from this commit:
> 866476f323465a8afef10b14b48d5136bf5c51fe (netfilter: conntrack: Flush connections with a given mark)
> 
> This allows conntrack -L to be ran succesfully when CONFIG_NF_CONNTRACK_MARK
> is not enabled.

I would like to understand how you're triggering this problem. If it
is a plain 'conntrack -L' command line invocation that triggers the
problem, then it's probably a userspace problem since we should not
send any mark attribute to the kernel if not set.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joseph.j.conley@gmail.com Dec. 19, 2016, 3:40 p.m. UTC | #2 
On Thu, Dec 15, 2016 at 3:55 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Wed, Dec 14, 2016 at 04:35:57PM -0500, joseph.j.conley@gmail.com wrote:
> > From: Joe Conley <joe.conley@lairdtech.com>
> >
> > Two missing ifdef checks for CONFIG_NF_CONNTRACK_MARK were causing
> > EOPNOTSUPP to be returned. Every single place that cda[CTA_MARK] or cda[CTA_MARK_MASK]
> > was checked was inside a #ifdef for CONFIG_NF_CONNTRACK_MARK except for these
> > two places. The reason for this change stems from this commit:
> > 866476f323465a8afef10b14b48d5136bf5c51fe (netfilter: conntrack: Flush connections with a given mark)
> >
> > This allows conntrack -L to be ran succesfully when CONFIG_NF_CONNTRACK_MARK
> > is not enabled.
>
> I would like to understand how you're triggering this problem. If it
> is a plain 'conntrack -L' command line invocation that triggers the
> problem, then it's probably a userspace problem since we should not
> send any mark attribute to the kernel if not set.

Building the kernel with CONFIG_NF_CONNTRACK_MARK disabled will cause
conntrack -L to return EOPNOTSUPP because of the missing ifdef checks.
Building the kernel with it enabled allows conntrack -L to run
successfully. At first, I thought this was a userspace bug as well but
it is not. Every single place CTA_MARK or CTA_MARK_MASK is used is
inside an ifdef check for CONFIG_NF_CONNTRACK_MARK except for these
two places. There is no clear reason as to why. There is no reason
that conntrack -L should return EOPNOTSUPP when
CONFIG_NF_CONNTRACK_MARK is disabled.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Dec. 23, 2016, 2:55 p.m. UTC | #3 
On Mon, Dec 19, 2016 at 10:40:50AM -0500, Joseph Conley wrote:
> On Thu, Dec 15, 2016 at 3:55 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Wed, Dec 14, 2016 at 04:35:57PM -0500, joseph.j.conley@gmail.com wrote:
> > > From: Joe Conley <joe.conley@lairdtech.com>
> > >
> > > Two missing ifdef checks for CONFIG_NF_CONNTRACK_MARK were causing
> > > EOPNOTSUPP to be returned. Every single place that cda[CTA_MARK] or cda[CTA_MARK_MASK]
> > > was checked was inside a #ifdef for CONFIG_NF_CONNTRACK_MARK except for these
> > > two places. The reason for this change stems from this commit:
> > > 866476f323465a8afef10b14b48d5136bf5c51fe (netfilter: conntrack: Flush connections with a given mark)
> > >
> > > This allows conntrack -L to be ran succesfully when CONFIG_NF_CONNTRACK_MARK
> > > is not enabled.
> >
> > I would like to understand how you're triggering this problem. If it
> > is a plain 'conntrack -L' command line invocation that triggers the
> > problem, then it's probably a userspace problem since we should not
> > send any mark attribute to the kernel if not set.
> 
> Building the kernel with CONFIG_NF_CONNTRACK_MARK disabled will cause
> conntrack -L to return EOPNOTSUPP because of the missing ifdef checks.
> Building the kernel with it enabled allows conntrack -L to run
> successfully. At first, I thought this was a userspace bug as well but
> it is not. Every single place CTA_MARK or CTA_MARK_MASK is used is
> inside an ifdef check for CONFIG_NF_CONNTRACK_MARK except for these
> two places. There is no clear reason as to why. There is no reason
> that conntrack -L should return EOPNOTSUPP when
> CONFIG_NF_CONNTRACK_MARK is disabled.

I think this a userspace bug, since we shouldn't send mark filter if
not set by the user.

Kernel rejects with EOPNOTSUPP to indicate userspace that the mark
filtering that requests is not supported. This check is good so user
knows that this mark-based filtering doesn't work.

Just sent a patch for userspace, thanks a lot for reporting.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 2754045..94146bd 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1107,13 +1107,13 @@  static int ctnetlink_flush_conntrack(struct net *net,
 				     u32 portid, int report)
 {
 	struct ctnetlink_filter *filter = NULL;
-
+#ifdef CONFIG_NF_CONNTRACK_MARK
 	if (cda[CTA_MARK] && cda[CTA_MARK_MASK]) {
 		filter = ctnetlink_alloc_filter(cda);
 		if (IS_ERR(filter))
 			return PTR_ERR(filter);
 	}
-
+#endif
 	nf_ct_iterate_cleanup(net, ctnetlink_filter_match, filter,
 			      portid, report);
 	kfree(filter);
@@ -1192,7 +1192,7 @@  static int ctnetlink_get_conntrack(struct net *net, struct sock *ctnl,
 			.dump = ctnetlink_dump_table,
 			.done = ctnetlink_done,
 		};
-
+#ifdef CONFIG_NF_CONNTRACK_MARK
 		if (cda[CTA_MARK] && cda[CTA_MARK_MASK]) {
 			struct ctnetlink_filter *filter;

@@ -1202,6 +1202,7 @@  static int ctnetlink_get_conntrack(struct net *net, struct sock *ctnl,

 			c.data = filter;
 		}
+#endif
 		return netlink_dump_start(ctnl, skb, nlh, &c);
 	}