[RFC/PATCH 0/3] Fix ctnetlink regressions

[RFC/PATCH 0/3] Fix ctnetlink regressions

[Kevin Cernekee]

These patches address a problem I am seeing on Linux 4.4.  They do not
apply as-is to the master branch.  But I wanted to run them past the list
first to gather feedback on whether this is a reasonable approach.

I am using the user conntrack helpers from conntrackd on systems running
Linux 3.14, 3.18, and 4.4.  It was observed that conntrackd worked fine
on the 3.14/3.18 systems, but had no apparent effect on the 4.4 systems.
I tracked this down to a new check that was added in 4.4:

+       if (nfq_ct->parse(nfqa[NFQA_CT], ct) < 0)
+               return NULL;
+
+       if (nfqa[NFQA_EXP])
+               nfq_ct->attach_expect(nfqa[NFQA_EXP], ct,
+                                     NETLINK_CB(entry->skb).portid,
+                                     nlmsg_report(nlh));

Prior to 4.4, even if a netlink message failed the parse() checks, the
kernel would still run attach_expect() on it.  This masked a number of
failures.  With 4.4+, a sanity check failure on any attribute checked by
parse() will prevent the expectation from being created, which usually
breaks the conntrack helper.

In my testing I found that the sanity checks for CTA_TIMEOUT, CTA_STATUS,
and CTA_HELP were overly strict.  CTA_TIMEOUT may have been inadvertently
fixed in master (commit f330a7fdbe161), but I don't think the other two
are.  My proposal is to relax the checks so that existing user programs
do not break.

Another option is to simply ignore the parse() result, so that the
interface remains bug-compatible with old user code.


Kevin Cernekee (3):
  netfilter: ctnetlink: Fix regression in CTA_TIMEOUT processing
  netfilter: ctnetlink: Fix regression in CTA_STATUS processing
  netfilter: ctnetlink: Fix regression in CTA_HELP processing

 include/uapi/linux/netfilter/nf_conntrack_common.h |  4 +++
 net/netfilter/nf_conntrack_netlink.c               | 35 +++++++++++++---------
 2 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.7.4

[RFC/PATCH 1/3] netfilter: ctnetlink: Fix regression in CTA_TIMEOUT processing

[Kevin Cernekee]

Commit b7bd1809e078 ("netfilter: nfnetlink_queue: get rid of
nfnetlink_queue_ct.c") introduced a new check on the return value
from the NFQA_CT parser (currently ctnetlink_glue_parse_ct()).
Prior to Linux 4.4, nfqnl_ct_parse() would process the NFQA_EXP
attribute even if there were errors in the NFQA_CT attribute.
After Linux 4.4, this is no longer true, so any error in the NFQA_CT
attribute will cause the kernel to silently fail to create an
expectation.

The new check is causing user conntrack helpers to fail.  If a user
program sends an NFQA_CT attribute containing a CTA_TIMEOUT attribute
before the connection is confirmed (i.e. before the initial
ACCEPT/DROP decision has been made), del_timer() in
ctnetlink_change_timeout() will fail, and all further processing will be
aborted.  The (simplified) calling sequence looks like:

    nfnetlink_rcv_msg
      nfqnl_recv_verdict
        nfqnl_ct_parse
          ctnetlink_glue_parse_ct
            ctnetlink_change_timeout
              del_timer [ERROR]
        nf_reinject
          __nf_conntrack_confirm

Fix this by adding a case to ctnetlink_change_timeout() to handle
unconfirmed connections.

Also, if a timeout of 0 is set for an unconfirmed connection, restore the
old behavior of ignoring it (rather than setting up a connection that
expires immediately).

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 net/netfilter/nf_conntrack_netlink.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 9f5272968abb..43beb950df16 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1531,11 +1531,15 @@ ctnetlink_change_timeout(struct nf_conn *ct, const struct nlattr * const cda[])
 {
 	u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 
-	if (!del_timer(&ct->timeout))
-		return -ETIME;
+	if (nf_ct_is_confirmed(ct)) {
+		if (!del_timer(&ct->timeout))
+			return -ETIME;
 
-	ct->timeout.expires = jiffies + timeout * HZ;
-	add_timer(&ct->timeout);
+		ct->timeout.expires = jiffies + timeout * HZ;
+		add_timer(&ct->timeout);
+	} else if (timeout != 0) {
+		ct->timeout.expires = timeout * HZ;
+	}
 
 	return 0;
 }
-- 
2.7.4

Re: [RFC/PATCH 1/3] netfilter: ctnetlink: Fix regression in CTA_TIMEOUT processing

[Doug Anderson]

Hi,

On Mon, Jan 16, 2017 at 9:14 PM, Kevin Cernekee <cernekee@chromium.org> wrote:
> Commit b7bd1809e078 ("netfilter: nfnetlink_queue: get rid of
> nfnetlink_queue_ct.c") introduced a new check on the return value
> from the NFQA_CT parser (currently ctnetlink_glue_parse_ct()).
> Prior to Linux 4.4, nfqnl_ct_parse() would process the NFQA_EXP
> attribute even if there were errors in the NFQA_CT attribute.
> After Linux 4.4, this is no longer true, so any error in the NFQA_CT
> attribute will cause the kernel to silently fail to create an
> expectation.
>
> The new check is causing user conntrack helpers to fail.  If a user
> program sends an NFQA_CT attribute containing a CTA_TIMEOUT attribute
> before the connection is confirmed (i.e. before the initial
> ACCEPT/DROP decision has been made), del_timer() in
> ctnetlink_change_timeout() will fail, and all further processing will be
> aborted.  The (simplified) calling sequence looks like:
>
>     nfnetlink_rcv_msg
>       nfqnl_recv_verdict
>         nfqnl_ct_parse
>           ctnetlink_glue_parse_ct
>             ctnetlink_change_timeout
>               del_timer [ERROR]
>         nf_reinject
>           __nf_conntrack_confirm
>
> Fix this by adding a case to ctnetlink_change_timeout() to handle
> unconfirmed connections.
>
> Also, if a timeout of 0 is set for an unconfirmed connection, restore the
> old behavior of ignoring it (rather than setting up a connection that
> expires immediately).
>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

I'm not an expert (I'd never looked at netlink code before yesterday),
but Kevin's explanation and proposal seem sane to me.  In case it's
helpful:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[RFC/PATCH 2/3] netfilter: ctnetlink: Fix regression in CTA_STATUS processing

[Kevin Cernekee]

The libnetfilter_conntrack userland library always sets IPS_CONFIRMED
when building a CTA_STATUS attribute.  If this toggles the bit from
0->1, Linux 4.4+ will reject it and this will cause any NFQA_EXP
attribute in the packet to be ignored.  This breaks conntrackd's
userland helpers because they operate on unconfirmed connections.

Instead of returning -EBUSY if the user program asks to modify an
unchangeable bit, simply ignore the change.

Also, fix the logic so that user programs are allowed to clear
the bits that they are allowed to change.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 include/uapi/linux/netfilter/nf_conntrack_common.h |  4 ++++
 net/netfilter/nf_conntrack_netlink.c               | 10 ++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
index 319f47128db8..846722d8e2a4 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -76,6 +76,10 @@ enum ip_conntrack_status {
 	IPS_DYING_BIT = 9,
 	IPS_DYING = (1 << IPS_DYING_BIT),
 
+	/* Bits that cannot be altered from userland. */
+	IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
+				 IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
+
 	/* Connection has fixed timeout. */
 	IPS_FIXED_TIMEOUT_BIT = 10,
 	IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT),
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 43beb950df16..cc59f388928e 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1422,10 +1422,6 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
 	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
 	d = ct->status ^ status;
 
-	if (d & (IPS_EXPECTED|IPS_CONFIRMED|IPS_DYING))
-		/* unchangeable */
-		return -EBUSY;
-
 	if (d & IPS_SEEN_REPLY && !(status & IPS_SEEN_REPLY))
 		/* SEEN_REPLY bit can only be set */
 		return -EBUSY;
@@ -1436,8 +1432,10 @@ ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
 
 	/* Be careful here, modifying NAT bits can screw up things,
 	 * so don't let users modify them directly if they don't pass
-	 * nf_nat_range. */
-	ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+	 * nf_nat_range.  Also, disallow changing bits that indicate
+	 * which hash table owns the connection. */
+	ct->status = (status & ~IPS_UNCHANGEABLE_MASK) |
+		     (ct->status & IPS_UNCHANGEABLE_MASK);
 	return 0;
 }
 
-- 
2.7.4

Re: [RFC/PATCH 2/3] netfilter: ctnetlink: Fix regression in CTA_STATUS processing

[Doug Anderson]

Hi,

On Mon, Jan 16, 2017 at 9:14 PM, Kevin Cernekee <cernekee@chromium.org> wrote:
> The libnetfilter_conntrack userland library always sets IPS_CONFIRMED
> when building a CTA_STATUS attribute.  If this toggles the bit from
> 0->1, Linux 4.4+ will reject it and this will cause any NFQA_EXP
> attribute in the packet to be ignored.  This breaks conntrackd's
> userland helpers because they operate on unconfirmed connections.

I prefer the wording that you changed it to between this patch and the
next one on the Chromium OS review system:

https://chromium-review.googlesource.com/c/428493/1..2//COMMIT_MSG

It makes it more obvious that the function has always considered some
of these things to be "errors" but that the errors simply affect
things so much in the past (before commit b7bd1809e078 ("netfilter:
nfnetlink_queue: get rid of nfnetlink_queue_ct.c")).

> Instead of returning -EBUSY if the user program asks to modify an
> unchangeable bit, simply ignore the change.
>
> Also, fix the logic so that user programs are allowed to clear
> the bits that they are allowed to change.

This is a general bugfix to the code.  If folks are using netfilter
then it seems like this ought to be backported even before 4.4.


> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>  include/uapi/linux/netfilter/nf_conntrack_common.h |  4 ++++
>  net/netfilter/nf_conntrack_netlink.c               | 10 ++++------
>  2 files changed, 8 insertions(+), 6 deletions(-)

This bugfix is fairly easy to understand.  Assuming you agree that
it's OK to silently ignore these errors (and behave more like things
worked before kernel 4.4), then it's a clear win.

Unless someone has a better suggestion, I'd say:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[RFC/PATCH 3/3] netfilter: ctnetlink: Fix regression in CTA_HELP processing

[Kevin Cernekee]

If a user program specifies CTA_HELP but the argument matches the
current conntrack helper name, ignore it instead of generating an error.

Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---
 net/netfilter/nf_conntrack_netlink.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index cc59f388928e..2912f582da65 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1472,14 +1472,19 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
 	struct nlattr *helpinfo = NULL;
 	int err;
 
-	/* don't change helper of sibling connections */
-	if (ct->master)
-		return -EBUSY;
-
 	err = ctnetlink_parse_help(cda[CTA_HELP], &helpname, &helpinfo);
 	if (err < 0)
 		return err;
 
+	/* don't change helper of sibling connections */
+	if (ct->master) {
+		if (help && help->helper &&
+		    !strcmp(help->helper->name, helpname))
+			return 0;
+		else
+			return -EBUSY;
+	}
+
 	if (!strcmp(helpname, "")) {
 		if (help && help->helper) {
 			/* we had a helper before ... */
-- 
2.7.4

Re: [RFC/PATCH 3/3] netfilter: ctnetlink: Fix regression in CTA_HELP processing

[Doug Anderson]

Hi,

On Mon, Jan 16, 2017 at 9:14 PM, Kevin Cernekee <cernekee@chromium.org> wrote:
> If a user program specifies CTA_HELP but the argument matches the
> current conntrack helper name, ignore it instead of generating an error.

The "subject" of this patch says that it fixes a regression, but that
regression isn't explained anywhere.  The description of this patch
should make it obvious that the regression is the same as described in
your previous patch in this series: "netfilter: ctnetlink: Fix
regression in CTA_TIMEOUT processing".

Specifically: calling this twice with the same value has always
returned an error for the 2nd call, but that error didn't affect
things in quite the same way before commit b7bd1809e078 ("netfilter:
nfnetlink_queue: get rid of nfnetlink_queue_ct.c").


> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index cc59f388928e..2912f582da65 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1472,14 +1472,19 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
>         struct nlattr *helpinfo = NULL;
>         int err;
>
> -       /* don't change helper of sibling connections */
> -       if (ct->master)
> -               return -EBUSY;
> -
>         err = ctnetlink_parse_help(cda[CTA_HELP], &helpname, &helpinfo);
>         if (err < 0)
>                 return err;
>
> +       /* don't change helper of sibling connections */
> +       if (ct->master) {

As discussed in the Chromium OS review system:

https://chromium-review.googlesource.com/c/428494/1..2/net/netfilter/nf_conntrack_netlink.c

It feels like we need a comment here.  There you added:

* If we try to change the helper to the same thing twice,
* treat the second attempt as a no-op instead of returning
* an error.

That seems like a good comment.

> +               if (help && help->helper &&
> +                   !strcmp(help->helper->name, helpname))

I was also curious: do you need to also compare helpinfo to
helper->to_nlattr() ?

...effectively right now if you call this function twice with the same
value you want to avoid returning an error for the 2nd call, right?
...you just want to treat it as a no-op.  It seems like checking
"helpinfo" would be safer to really detect two calls with the same
value, but maybe I'm being paranoid...

> +                       return 0;
> +               else
> +                       return -EBUSY;
> +       }
> +
>         if (!strcmp(helpname, "")) {
>                 if (help && help->helper) {
>                         /* we had a helper before ... */
> --
> 2.7.4
>

Re: [RFC/PATCH 0/3] Fix ctnetlink regressions

[Pablo Neira Ayuso]

On Mon, Jan 16, 2017 at 09:14:05PM -0800, Kevin Cernekee wrote:
> These patches address a problem I am seeing on Linux 4.4.  They do not
> apply as-is to the master branch.  But I wanted to run them past the list
> first to gather feedback on whether this is a reasonable approach.

1/3 and 3/3 look fine.

Regarding 2/3, I'd suggest a new ctnetlink_update_status() and you use
it from ctnetlink_glue_parse_ct(). This new function allows us to set
status bits leaving what is already set intact, as you proposed.

So we leave ctnetlink_change_status() in place, but not being called
from the userspace helper path.

Thanks for fixing up this mess Kevin.