[RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

This patch enables comparison of 0 value with mark and zone since
both CTA_MARK and CTA_ZONE are not set in case of its value is 0.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 src/conntrack/compare.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/conntrack/compare.c b/src/conntrack/compare.c
index f4a194a..06edd0d 100644
--- a/src/conntrack/compare.c
+++ b/src/conntrack/compare.c
@@ -24,8 +24,12 @@ static int __cmp(int attr,
        } else if (!a && !b) {
                return 1;
        } else if (flags & NFCT_CMP_MASK &&
-                  test_bit(attr, ct1->head.set)) {
-               return 0;
+                  !test_bit(attr, ct1->head.set)) {
+               return 1;
+       } else if (attr == ATTR_MARK) {
+               return ct1->mark == 0 && ct2->mark == 0;
+       } else if (attr == ATTR_ZONE) {
+               return ct1->zone == 0 && ct2->zone == 0;
        } else if (flags & NFCT_CMP_STRICT) {
                return 0;
        }
--
1.9.1

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Florian Westphal]

Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
> This patch enables comparison of 0 value with mark and zone since
> both CTA_MARK and CTA_ZONE are not set in case of its value is 0.

I think the general idea is right.

When nfct_cmp is invoked with 'full compare' then two conntracks
should be regarded the same if they only differ in 'zone unset'
and 'zone set and it is 0', as a unset zone does imply a 0.

Maybe it is better to alter cmp_meta() and invoke a different
comparator for MARK and ZONE that will give 'extra chance'
when we hit the NFCT_CMP_STRICT conditional, i.e.

>         } else if (flags & NFCT_CMP_STRICT) {

One way might be to add __cmp_harder() which is same as __cmp() but
will invoke cmp() again in the CMP_STRICT case.

Then, cmp_zone() could be altered to do

return nfct_attr_get_u16(ct1) == nfct_attr_get_u16(ct2)

Which should fix this problem (get_u16 of unset attr returns 0).

Alternatively of course we could change __cmp() to always
invoke the cmp() function again in STRICT case but it probably
requires a lot more changes/care since we'd have to fix all the
other attribute specific comparators.

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

 Hello, thank you for your reply.

2014-06-12 9:18 GMT+09:00 Florian Westphal <fw@strlen.de>:
> Maybe it is better to alter cmp_meta() and invoke a different
> comparator for MARK and ZONE that will give 'extra chance'

I see, thanks.

> when we hit the NFCT_CMP_STRICT conditional, i.e.

# I may not understand what you told me...

nf_conntrack which is created by --zone 0 options is the first param
of nfct_cmp() with NFCT_CMP_MASK flag in conntrack command,
I think it's better to handle NFCT_CMP_MASK flag too.

How about creating new function __cmp_none_as_zero() which
is called from cmp_meta() in case of ZONE attr and its signature
is the same as __cmp():

    return __cmp(attr, ct1, ct2, flags, cmp) ||
            (!test_bit(attr, ct1->head.set || nfct_get_attr_u16(ct1,
attr) == 0) &&
             (!test_bit(attr, ct2->head.set || nfct_get_attr_u16(ct2,
attr) == 0));

But this can work only for u16 attrs. To work with another size,
I think we need adding switch-case statement of attr length to
code snippet above or adding a new similer functions for it.
Would you tell me which one is better?

Thanks,

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Florian Westphal]

Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
> I see, thanks.
> 
> > when we hit the NFCT_CMP_STRICT conditional, i.e.
> 
> # I may not understand what you told me...

Understandable, I meant CMP_MASK.  But I think we can
get away with an even simpler change.

What about this:

static int cmp_zone(const struct nf_conntrack *ct1,
	const struct nf_conntrack *ct2, unsigned int flags)
{
 return nfct_get_attr_u16(ct1, ATTR_ZONE) == nfct_get_attr_u16(ct2, ATTR_ZONE);
}

Then it should be sufficient to not call __cmp at all, i.e.:

- if (!__cmp(ATTR_ZONE, ct1, ct2, flags, cmp_zone))
+ if (!cmp_zone(ct1, ct2, flags))

ct1 and ct2 zones would then always be equal except if
both have ATTR_ZONE set and the zones are different.

What do you think?

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

 Hello, thank you for taking your time.

2014-06-12 18:48 GMT+09:00 Florian Westphal <fw@strlen.de>:
> ct1 and ct2 zones would then always be equal except if
> both have ATTR_ZONE set and the zones are different.

I think it reasonable too, but attr holds old value if it was unset.
Too much doubt?

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Florian Westphal]

Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
>  Hello, thank you for taking your time.
> 
> 2014-06-12 18:48 GMT+09:00 Florian Westphal <fw@strlen.de>:
> > ct1 and ct2 zones would then always be equal except if
> > both have ATTR_ZONE set and the zones are different.
> 
> I think it reasonable too, but attr holds old value if it was unset.

Yes, but nfct_get_attr_u* return 0 if attribute bit is unset.

So I think this should work.

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

On Thu, Jun 12, 2014 at 01:59:25PM +0200, Florian Westphal wrote:
> > > ct1 and ct2 zones would then always be equal except if
> > > both have ATTR_ZONE set and the zones are different.
> > 
> > I think it reasonable too, but attr holds old value if it was unset.
> 
> Yes, but nfct_get_attr_u* return 0 if attribute bit is unset.

That's why you do not use the raw value. I should have understood
it earlyer, sorry.

> So I think this should work.

I agree with you, thank you.
Should we clear errno in that cmp_zone() before return?

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Florian Westphal]

Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
> Should we clear errno in that cmp_zone() before return?

Good question, I don't know.

I *think* the best approach would be to not clear it
at this time.

Its not yet clear to me what problem the zeroing
would solve.  I don't think callers/users should rely on errno
having any particular value after nfct_cmp() returns, regardless
of the return value.

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

 Hello, thank you for your reply.

2014-06-12 23:05 GMT+09:00 Florian Westphal <fw@strlen.de>:
> Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
>> Should we clear errno in that cmp_zone() before return?
>
> I *think* the best approach would be to not clear it
> at this time.
>
> would solve.  I don't think callers/users should rely on errno
> having any particular value after nfct_cmp() returns, regardless
> of the return value.

I'd read same thing in err30-c and I'll follow your best thought.

Sorry for offtopic:
I think go lang people made an unlikable decision. Its function
can return multiple value, and its c wrapper suite (cgo) normally
returns result and errno as error. They have an ideom like

    if retval, err := function(); err != nil {
        error_processing()
    }

Things you told me is very helpful for wrapping by golang, thanks.

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

 Hello,

2014-06-12 18:48 GMT+09:00 Florian Westphal <fw@strlen.de>:
> What about this:
>
> static int cmp_zone(const struct nf_conntrack *ct1,
>         const struct nf_conntrack *ct2, unsigned int flags)
> {
>  return nfct_get_attr_u16(ct1, ATTR_ZONE) == nfct_get_attr_u16(ct2, ATTR_ZONE);
> }
>
> Then it should be sufficient to not call __cmp at all, i.e.:
>
> - if (!__cmp(ATTR_ZONE, ct1, ct2, flags, cmp_zone))
> + if (!cmp_zone(ct1, ct2, flags))
>
> ct1 and ct2 zones would then always be equal except if
> both have ATTR_ZONE set and the zones are different.

Sorry, it did not work with NFCT_CMP_MASK in case of
only ct2 has attr. We need to think of NCFT_CMP_MASK
as you told.

  static int cmp_zone(const struct nf_conntrack *ct1,
                                  const struct nf_conntrack *ct2,
                                  unsigned int flags)
  {
      return (flags & NFCT_CMP_MASK &&
                 !test_bit(ATTR_ZONE, ct1->head.set)) ||
                 nfct_get_attr_u16(ct1, ATTR_ZONE)
                 == nfct_get_attr_u16(ct2, ATTR_ZONE);
  }

What about this?
Could I resend the patch if it is acceptable?

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Florian Westphal]

Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
> 2014-06-12 18:48 GMT+09:00 Florian Westphal <fw@strlen.de>:
> > What about this:
> >
> > static int cmp_zone(const struct nf_conntrack *ct1,
> >         const struct nf_conntrack *ct2, unsigned int flags)
> > {
> >  return nfct_get_attr_u16(ct1, ATTR_ZONE) == nfct_get_attr_u16(ct2, ATTR_ZONE);
> > }
> >
> > Then it should be sufficient to not call __cmp at all, i.e.:
> >
> > - if (!__cmp(ATTR_ZONE, ct1, ct2, flags, cmp_zone))
> > + if (!cmp_zone(ct1, ct2, flags))
> >
> > ct1 and ct2 zones would then always be equal except if
> > both have ATTR_ZONE set and the zones are different.
> 
> Sorry, it did not work with NFCT_CMP_MASK in case of
> only ct2 has attr. We need to think of NCFT_CMP_MASK
> as you told.
i
Why?  cmp_zone() does not evaluate the 'flags' paramter.

So, if only ct2 has attr:
nfct_get_attr_u16(ct1, ATTR_ZONE) -> returns 0
nfct_get_attr_u16(ct2, ATTR_ZONE) -> returns the zone id

ct1 and ct2 would be equal if ct2 zone is 0.

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Ken-ichirou MATSUZAWA]

Sorry for lack of explanation.

On Mon, Jun 16, 2014 at 01:41:54PM +0200, Florian Westphal wrote:
> So, if only ct2 has attr:
> nfct_get_attr_u16(ct1, ATTR_ZONE) -> returns 0
> nfct_get_attr_u16(ct2, ATTR_ZONE) -> returns the zone id
> 
> ct1 and ct2 would be equal if ct2 zone is 0.

And nfct_cmp(ct1, ct2, NFCT_CMP_MASK) is expected to return
true if ct2 zone is not 0.

Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone

[Florian Westphal]

Ken-ichirou MATSUZAWA <chamaken@gmail.com> wrote:
> Sorry for lack of explanation.
> 
> On Mon, Jun 16, 2014 at 01:41:54PM +0200, Florian Westphal wrote:
> > So, if only ct2 has attr:
> > nfct_get_attr_u16(ct1, ATTR_ZONE) -> returns 0
> > nfct_get_attr_u16(ct2, ATTR_ZONE) -> returns the zone id
> > 
> > ct1 and ct2 would be equal if ct2 zone is 0.
> 
> And nfct_cmp(ct1, ct2, NFCT_CMP_MASK) is expected to return
> true if ct2 zone is not 0.

You're right.  I think your patch is the right thing to do.