[PATCH lnf-conntrack 1/3] qa: add api test for nfct_cmp and nfct_exp functions

[PATCH lnf-conntrack 1/3] qa: add api test for nfct_cmp and nfct_exp functions

[Florian Westphal]

Some of these checks will fail due to errors in nfct_cmp STRICT handling
and missing comparision of attributes in the nfexpect_cmp functions.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 The two patches following this one fix these test cases.

 I will push this in a couple of days if there are no
 objections.

 qa/test_api.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 87 insertions(+), 12 deletions(-)

diff --git a/qa/test_api.c b/qa/test_api.c
index 499d01f..e6ec240 100644
--- a/qa/test_api.c
+++ b/qa/test_api.c
@@ -82,6 +82,91 @@ static void test_nfct_bitmask(void)
 	printf("OK\n");
 }
 
+static void test_nfct_cmp_api(struct nf_conntrack *ct1, struct nf_conntrack *ct2)
+{
+	int i;
+
+	printf("== test cmp API ==\n");
+
+	assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL) == 1);
+	assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_STRICT) == 0);
+
+	nfct_copy(ct1, ct2, NFCT_CP_OVERRIDE);
+
+	assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL) == 1);
+	assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_STRICT) == 1);
+
+	for (i=0; i < ATTR_MAX ; i++) {
+		nfct_attr_unset(ct1, i);
+
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL) == 1);
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_STRICT) == 0);
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_MASK) == 1);
+	}
+	nfct_copy(ct1, ct2, NFCT_CP_OVERRIDE);
+	for (i=0; i < ATTR_MAX ; i++) {
+		nfct_attr_unset(ct2, i);
+
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL) == 1);
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_STRICT) == 0);
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_MASK) == 0);
+	}
+	nfct_copy(ct1, ct2, NFCT_CP_OVERRIDE);
+	for (i=0; i < ATTR_MAX ; i++) {
+		nfct_attr_unset(ct1, i);
+		nfct_attr_unset(ct2, i);
+
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL) == 1);
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_STRICT) == 1);
+		assert(nfct_cmp(ct1, ct2, NFCT_CMP_ALL|NFCT_CMP_MASK) == 1);
+	}
+	nfct_destroy(ct1);
+	nfct_destroy(ct2);
+}
+
+static void test_nfexp_cmp_api(struct nf_expect *ex1, struct nf_expect *ex2)
+{
+	int i;
+
+	printf("== test expect cmp API ==\n");
+
+	/* XXX: missing nfexp_copy API. */
+	memcpy(ex1, ex2, nfexp_maxsize());
+
+	assert(nfexp_cmp(ex1, ex2, 0) == 1);
+	assert(nfexp_cmp(ex1, ex2, NFCT_CMP_STRICT) == 1);
+
+	assert(nfexp_attr_is_set(ex1, 0) == 1);
+	nfexp_attr_unset(ex1, 0);
+	assert(nfexp_attr_is_set(ex1, 0) == 0);
+
+	memcpy(ex1, ex2, nfexp_maxsize());
+	for (i=0; i < ATTR_EXP_MAX; i++) {
+		nfexp_attr_unset(ex1, i);
+
+		assert(nfexp_cmp(ex1, ex2, 0) == 1);
+		assert(nfexp_cmp(ex1, ex2, NFCT_CMP_STRICT) == 0);
+		assert(nfexp_cmp(ex1, ex2, NFCT_CMP_MASK) == 1);
+	}
+	memcpy(ex1, ex2, nfexp_maxsize());
+	for (i=0; i < ATTR_EXP_MAX; i++) {
+		nfexp_attr_unset(ex2, i);
+
+		assert(nfexp_cmp(ex1, ex2, 0) == 1);
+		assert(nfexp_cmp(ex1, ex2, NFCT_CMP_MASK) == 0);
+	}
+	memcpy(ex1, ex2, nfexp_maxsize());
+	for (i=0; i < ATTR_EXP_MAX; i++) {
+		nfexp_attr_unset(ex1, i);
+		nfexp_attr_unset(ex2, i);
+
+		assert(nfexp_cmp(ex1, ex2, 0) == 1);
+		assert(nfexp_cmp(ex1, ex2, NFCT_CMP_STRICT) == 1);
+		assert(nfexp_cmp(ex1, ex2, NFCT_CMP_MASK) == 1);
+	}
+	nfexp_destroy(ex1);
+	nfexp_destroy(ex2);
+}
 
 int main(void)
 {
@@ -211,10 +296,9 @@ int main(void)
 		eval_sigterm(status);
 	}
 
-	printf("== test cmp API ==\n");
 	ret = fork();
 	if (ret == 0) {
-		nfct_cmp(tmp, ct, NFCT_CMP_ALL);
+		test_nfct_cmp_api(tmp, ct);
 		exit(0);
 	} else {
 		wait(&status);
@@ -276,24 +360,15 @@ int main(void)
 		eval_sigterm(status);
 	}
 
-	/* XXX: missing nfexp_copy API. */
-	memcpy(tmp_exp, exp, nfexp_maxsize());
-
-	printf("== test expect cmp API ==\n");
 	ret = fork();
 	if (ret == 0) {
-		nfexp_cmp(tmp_exp, exp, 0);
+		test_nfexp_cmp_api(tmp_exp, exp);
 		exit(0);
 	} else {
 		wait(&status);
 		eval_sigterm(status);
 	}
 
-	ct2 = nfct_clone(ct);
-	assert(ct2);
-	assert(nfct_cmp(ct, ct2, NFCT_CMP_ALL) == 1);
-	nfct_destroy(ct2);
-
 	ct2 = nfct_new();
 	if (!ct2) {
 		perror("nfct_new");
-- 
1.8.1.5

[PATCH 2/3] conntrack, expect: fix _cmp api with STRICT checking

[Florian Westphal]

Normal comparision succeeds when the _common_ attribute subset
have same values.

When STRICT matching is specified, the comparision should succeed only when
both objects have same attribute subset and attribute values match.

However, STRICT comparision often fails as an attribute missing in both
objects is erronously considered an error.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/conntrack/compare.c | 6 +++++-
 src/expect/compare.c    | 7 ++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/conntrack/compare.c b/src/conntrack/compare.c
index e282a24..97c25cb 100644
--- a/src/conntrack/compare.c
+++ b/src/conntrack/compare.c
@@ -17,8 +17,12 @@ static int __cmp(int attr,
 		 	    const struct nf_conntrack *ct2,
 			    unsigned int flags))
 {
-	if (test_bit(attr, ct1->head.set) && test_bit(attr, ct2->head.set)) {
+	int a = test_bit(attr, ct1->head.set);
+	int b = test_bit(attr, ct2->head.set);
+	if (a && b) {
 		return cmp(ct1, ct2, flags);
+	} else if (!a && !b) {
+		return 1;
 	} else if (flags & NFCT_CMP_MASK &&
 		   test_bit(attr, ct1->head.set)) {
 		return 0;
diff --git a/src/expect/compare.c b/src/expect/compare.c
index a524f4c..484d7b1 100644
--- a/src/expect/compare.c
+++ b/src/expect/compare.c
@@ -18,8 +18,13 @@ static int exp_cmp(int attr,
 			      const struct nf_expect *exp2,
 			      unsigned int flags))
 {
-	if (test_bit(attr, exp1->set) && test_bit(attr, exp2->set)) {
+	int a = test_bit(attr, exp1->set);
+	int b = test_bit(attr, exp2->set);
+
+	if (a && b) {
 		return cmp(exp1, exp2, flags);
+	} else if (!a && !b) {
+		return 1;
 	} else if (flags & NFCT_CMP_MASK &&
 		   test_bit(attr, exp1->set)) {
 		return 0;
-- 
1.8.1.5

[PATCH 3/3] expect: consider all expect attributes when comparing

[Florian Westphal]

The expect cmp function ignored most of the attributes.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/expect/compare.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 10 deletions(-)

diff --git a/src/expect/compare.c b/src/expect/compare.c
index 484d7b1..6326706 100644
--- a/src/expect/compare.c
+++ b/src/expect/compare.c
@@ -35,6 +35,45 @@ static int exp_cmp(int attr,
 }
 
 static int
+cmp_exp_master(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	       unsigned int flags)
+{
+	return __cmp_orig((struct nf_conntrack *)&exp1->master,
+			  (struct nf_conntrack *)&exp2->master, flags);
+}
+
+static int
+cmp_exp_expected(const struct nf_expect *exp1, const struct nf_expect *exp2,
+		 unsigned int flags)
+{
+	return __cmp_orig((struct nf_conntrack *)&exp1->expected,
+			  (struct nf_conntrack *)&exp2->expected, flags);
+}
+
+static int
+cmp_exp_mask(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	     unsigned int flags)
+{
+	return __cmp_orig((struct nf_conntrack *)&exp1->mask,
+			  (struct nf_conntrack *)&exp2->mask, flags);
+
+}
+
+static int
+cmp_exp_timeout(const struct nf_expect *exp1, const struct nf_expect *exp2,
+		unsigned int flags)
+{
+	return exp1->timeout == exp2->timeout;
+}
+
+static int
+cmp_exp_zone(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	     unsigned int flags)
+{
+	return exp1->zone == exp2->zone;
+}
+
+static int
 cmp_exp_flags(const struct nf_expect *exp1, const struct nf_expect *exp2,
 	      unsigned int flags)
 {
@@ -42,32 +81,68 @@ cmp_exp_flags(const struct nf_expect *exp1, const struct nf_expect *exp2,
 }
 
 static int
+cmp_exp_hname(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	      unsigned int flags)
+{
+	return strcmp(exp1->helper_name, exp2->helper_name) == 0;
+}
+
+static int
 cmp_exp_class(const struct nf_expect *exp1, const struct nf_expect *exp2,
 	      unsigned int flags)
 {
 	return (exp1->class == exp2->class);
 }
 
+static int
+cmp_exp_natt(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	     unsigned int flags)
+{
+	return __cmp_orig((struct nf_conntrack *)&exp1->nat,
+			  (struct nf_conntrack *)&exp2->nat, flags);
+
+}
+
+static int
+cmp_exp_natdir(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	       unsigned int flags)
+{
+	return exp1->nat_dir == exp2->nat_dir;
+}
+
+static int
+cmp_exp_expfn(const struct nf_expect *exp1, const struct nf_expect *exp2,
+	      unsigned int flags)
+{
+	return strcmp(exp1->expectfn, exp2->expectfn) == 0;
+}
+
+
 int __cmp_expect(const struct nf_expect *exp1,
 		 const struct nf_expect *exp2,
 		 unsigned int flags)
 {
-	if (!__cmp_orig((struct nf_conntrack *)&exp1->master,
-			(struct nf_conntrack *)&exp2->master, flags)) {
+	if (!exp_cmp(ATTR_EXP_MASTER, exp1, exp2, flags, cmp_exp_master))
 		return 0;
-	}
-	if (!__cmp_orig((struct nf_conntrack *)&exp1->expected,
-			(struct nf_conntrack *)&exp2->expected, flags)) {
+	if (!exp_cmp(ATTR_EXP_EXPECTED, exp1, exp2, flags, cmp_exp_expected))
 		return 0;
-	}
-	if (!__cmp_orig((struct nf_conntrack *)&exp1->mask,
-			(struct nf_conntrack *)&exp2->mask, flags)) {
+	if (!exp_cmp(ATTR_EXP_MASK, exp1, exp2, flags, cmp_exp_mask))
+		return 0;
+	if (!exp_cmp(ATTR_EXP_TIMEOUT, exp1, exp2, flags, cmp_exp_timeout))
+		return 0;
+	if (!exp_cmp(ATTR_EXP_ZONE, exp1, exp2, flags, cmp_exp_zone))
 		return 0;
-	}
 	if (!exp_cmp(ATTR_EXP_FLAGS, exp1, exp2, flags, cmp_exp_flags))
 		return 0;
+	if (!exp_cmp(ATTR_EXP_HELPER_NAME, exp1, exp2, flags, cmp_exp_hname))
+		return 0;
 	if (!exp_cmp(ATTR_EXP_CLASS, exp1, exp2, flags, cmp_exp_class))
 		return 0;
-
+	if (!exp_cmp(ATTR_EXP_NAT_TUPLE, exp1, exp2, flags, cmp_exp_natt))
+		return 0;
+	if (!exp_cmp(ATTR_EXP_NAT_DIR, exp1, exp2, flags, cmp_exp_natdir))
+		return 0;
+	if (!exp_cmp(ATTR_EXP_FN, exp1, exp2, flags, cmp_exp_expfn))
+		return 0;
 	return 1;
 }
-- 
1.8.1.5

Re: [PATCH 3/3] expect: consider all expect attributes when comparing

[Pablo Neira Ayuso]

On Sat, Jun 01, 2013 at 02:59:31PM +0200, Florian Westphal wrote:
> The expect cmp function ignored most of the attributes.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/expect/compare.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/src/expect/compare.c b/src/expect/compare.c
> index 484d7b1..6326706 100644
> --- a/src/expect/compare.c
> +++ b/src/expect/compare.c
> @@ -35,6 +35,45 @@ static int exp_cmp(int attr,
>  }
>  
>  static int
> +cmp_exp_master(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	       unsigned int flags)
> +{
> +	return __cmp_orig((struct nf_conntrack *)&exp1->master,
> +			  (struct nf_conntrack *)&exp2->master, flags);
> +}
> +
> +static int
> +cmp_exp_expected(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +		 unsigned int flags)
> +{
> +	return __cmp_orig((struct nf_conntrack *)&exp1->expected,
> +			  (struct nf_conntrack *)&exp2->expected, flags);
> +}
> +
> +static int
> +cmp_exp_mask(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	     unsigned int flags)
> +{
> +	return __cmp_orig((struct nf_conntrack *)&exp1->mask,
> +			  (struct nf_conntrack *)&exp2->mask, flags);
> +
> +}
> +
> +static int
> +cmp_exp_timeout(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +		unsigned int flags)
> +{
> +	return exp1->timeout == exp2->timeout;
> +}

The timeout comparison needs to implement the __NFCT_CMP_TIMEOUT
logic, similar to nfct_cmp. Otherwise nfexp_cmp will break in
conntrackd with expect sync mode.

> +
> +static int
> +cmp_exp_zone(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	     unsigned int flags)
> +{
> +	return exp1->zone == exp2->zone;
> +}
> +
> +static int
>  cmp_exp_flags(const struct nf_expect *exp1, const struct nf_expect *exp2,
>  	      unsigned int flags)
>  {
> @@ -42,32 +81,68 @@ cmp_exp_flags(const struct nf_expect *exp1, const struct nf_expect *exp2,
>  }
>  
>  static int
> +cmp_exp_hname(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	      unsigned int flags)
> +{
> +	return strcmp(exp1->helper_name, exp2->helper_name) == 0;
> +}
> +
> +static int
>  cmp_exp_class(const struct nf_expect *exp1, const struct nf_expect *exp2,
>  	      unsigned int flags)
>  {
>  	return (exp1->class == exp2->class);
>  }
>  
> +static int
> +cmp_exp_natt(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	     unsigned int flags)
> +{
> +	return __cmp_orig((struct nf_conntrack *)&exp1->nat,
> +			  (struct nf_conntrack *)&exp2->nat, flags);
> +
> +}
> +
> +static int
> +cmp_exp_natdir(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	       unsigned int flags)
> +{
> +	return exp1->nat_dir == exp2->nat_dir;
> +}
> +
> +static int
> +cmp_exp_expfn(const struct nf_expect *exp1, const struct nf_expect *exp2,
> +	      unsigned int flags)
> +{
> +	return strcmp(exp1->expectfn, exp2->expectfn) == 0;
> +}
> +
> +
>  int __cmp_expect(const struct nf_expect *exp1,
>  		 const struct nf_expect *exp2,
>  		 unsigned int flags)
>  {
> -	if (!__cmp_orig((struct nf_conntrack *)&exp1->master,
> -			(struct nf_conntrack *)&exp2->master, flags)) {
> +	if (!exp_cmp(ATTR_EXP_MASTER, exp1, exp2, flags, cmp_exp_master))
>  		return 0;
> -	}
> -	if (!__cmp_orig((struct nf_conntrack *)&exp1->expected,
> -			(struct nf_conntrack *)&exp2->expected, flags)) {
> +	if (!exp_cmp(ATTR_EXP_EXPECTED, exp1, exp2, flags, cmp_exp_expected))
>  		return 0;
> -	}
> -	if (!__cmp_orig((struct nf_conntrack *)&exp1->mask,
> -			(struct nf_conntrack *)&exp2->mask, flags)) {
> +	if (!exp_cmp(ATTR_EXP_MASK, exp1, exp2, flags, cmp_exp_mask))
> +		return 0;
> +	if (!exp_cmp(ATTR_EXP_TIMEOUT, exp1, exp2, flags, cmp_exp_timeout))
> +		return 0;
> +	if (!exp_cmp(ATTR_EXP_ZONE, exp1, exp2, flags, cmp_exp_zone))
>  		return 0;
> -	}
>  	if (!exp_cmp(ATTR_EXP_FLAGS, exp1, exp2, flags, cmp_exp_flags))
>  		return 0;
> +	if (!exp_cmp(ATTR_EXP_HELPER_NAME, exp1, exp2, flags, cmp_exp_hname))
> +		return 0;
>  	if (!exp_cmp(ATTR_EXP_CLASS, exp1, exp2, flags, cmp_exp_class))
>  		return 0;
> -
> +	if (!exp_cmp(ATTR_EXP_NAT_TUPLE, exp1, exp2, flags, cmp_exp_natt))
> +		return 0;
> +	if (!exp_cmp(ATTR_EXP_NAT_DIR, exp1, exp2, flags, cmp_exp_natdir))
> +		return 0;
> +	if (!exp_cmp(ATTR_EXP_FN, exp1, exp2, flags, cmp_exp_expfn))
> +		return 0;
>  	return 1;
>  }
> -- 
> 1.8.1.5
> 
> --
> 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

Re: [PATCH 3/3] expect: consider all expect attributes when comparing

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +static int
> > +cmp_exp_timeout(const struct nf_expect *exp1, const struct nf_expect *exp2,
> > +		unsigned int flags)
> > +{
> > +	return exp1->timeout == exp2->timeout;
> > +}
> 
> The timeout comparison needs to implement the __NFCT_CMP_TIMEOUT
> logic, similar to nfct_cmp. Otherwise nfexp_cmp will break in
> conntrackd with expect sync mode.

You're right of course.  I'll implement this and send a v2 of this
patch.

Thanks for catching this!

Re: [PATCH 3/3] expect: consider all expect attributes when comparing

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > +static int
> > > +cmp_exp_timeout(const struct nf_expect *exp1, const struct nf_expect *exp2,
> > > +		unsigned int flags)
> > > +{
> > > +	return exp1->timeout == exp2->timeout;
> > > +}
> > 
> > The timeout comparison needs to implement the __NFCT_CMP_TIMEOUT
> > logic, similar to nfct_cmp. Otherwise nfexp_cmp will break in
> > conntrackd with expect sync mode.
> 
> You're right of course.  I'll implement this and send a v2 of this
> patch.

Hrm, I think a better option is to not compare the expectation timeout
in the first place.  I think the timeout is an irrelevant meta detail;
if the actual expectations are identical, nfexp_cmp should say so even
if they happen to have different timeouts.

In case users want a timeout compare, they could simply
nfexp_get_attr_u32(e1, ATTR_EXP_TIMEOUT) == ..._u32(e2, ATTR_EXP_TIMEOUT)?

Re: [PATCH 3/3] expect: consider all expect attributes when comparing

[Pablo Neira Ayuso]

On Wed, Jun 05, 2013 at 05:41:01PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > +static int
> > > > +cmp_exp_timeout(const struct nf_expect *exp1, const struct nf_expect *exp2,
> > > > +		unsigned int flags)
> > > > +{
> > > > +	return exp1->timeout == exp2->timeout;
> > > > +}
> > > 
> > > The timeout comparison needs to implement the __NFCT_CMP_TIMEOUT
> > > logic, similar to nfct_cmp. Otherwise nfexp_cmp will break in
> > > conntrackd with expect sync mode.
> > 
> > You're right of course.  I'll implement this and send a v2 of this
> > patch.
> 
> Hrm, I think a better option is to not compare the expectation timeout
> in the first place.  I think the timeout is an irrelevant meta detail;
> if the actual expectations are identical, nfexp_cmp should say so even
> if they happen to have different timeouts.

That's fine with me, we don't have any requirement for such feature at
this moment. So just skip it. Probably adding a short comment in the
code would be a good idea as placeholder.

Re: [PATCH 3/3] expect: consider all expect attributes when comparing

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jun 05, 2013 at 05:41:01PM +0200, Florian Westphal wrote:
> > Hrm, I think a better option is to not compare the expectation timeout
> > in the first place.  I think the timeout is an irrelevant meta detail;
> > if the actual expectations are identical, nfexp_cmp should say so even
> > if they happen to have different timeouts.
> 
> That's fine with me, we don't have any requirement for such feature at
> this moment. So just skip it. Probably adding a short comment in the
> code would be a good idea as placeholder.

Perfect!
I've pushed all three patches, with ATTR_TIMEOUT compare replaced by a
comment.