* [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
@ 2014-06-11 23:42 Ken-ichirou MATSUZAWA
2014-06-12 0:18 ` Florian Westphal
0 siblings, 1 reply; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-11 23:42 UTC (permalink / raw)
To: The netfilter developer mailinglist; +Cc: Pablo Neira Ayuso
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-11 23:42 [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone Ken-ichirou MATSUZAWA
@ 2014-06-12 0:18 ` Florian Westphal
2014-06-12 5:19 ` Ken-ichirou MATSUZAWA
0 siblings, 1 reply; 13+ messages in thread
From: Florian Westphal @ 2014-06-12 0:18 UTC (permalink / raw)
To: Ken-ichirou MATSUZAWA
Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 0:18 ` Florian Westphal
@ 2014-06-12 5:19 ` Ken-ichirou MATSUZAWA
2014-06-12 9:48 ` Florian Westphal
0 siblings, 1 reply; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-12 5:19 UTC (permalink / raw)
To: Florian Westphal; +Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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,
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 5:19 ` Ken-ichirou MATSUZAWA
@ 2014-06-12 9:48 ` Florian Westphal
2014-06-12 11:33 ` Ken-ichirou MATSUZAWA
2014-06-16 0:04 ` Ken-ichirou MATSUZAWA
0 siblings, 2 replies; 13+ messages in thread
From: Florian Westphal @ 2014-06-12 9:48 UTC (permalink / raw)
To: Ken-ichirou MATSUZAWA
Cc: Florian Westphal, The netfilter developer mailinglist, Pablo Neira Ayuso
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 9:48 ` Florian Westphal
@ 2014-06-12 11:33 ` Ken-ichirou MATSUZAWA
2014-06-12 11:59 ` Florian Westphal
2014-06-16 0:04 ` Ken-ichirou MATSUZAWA
1 sibling, 1 reply; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-12 11:33 UTC (permalink / raw)
To: Florian Westphal; +Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 11:33 ` Ken-ichirou MATSUZAWA
@ 2014-06-12 11:59 ` Florian Westphal
2014-06-12 13:43 ` Ken-ichirou MATSUZAWA
0 siblings, 1 reply; 13+ messages in thread
From: Florian Westphal @ 2014-06-12 11:59 UTC (permalink / raw)
To: Ken-ichirou MATSUZAWA
Cc: Florian Westphal, The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 11:59 ` Florian Westphal
@ 2014-06-12 13:43 ` Ken-ichirou MATSUZAWA
2014-06-12 14:05 ` Florian Westphal
0 siblings, 1 reply; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-12 13:43 UTC (permalink / raw)
To: Florian Westphal; +Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 13:43 ` Ken-ichirou MATSUZAWA
@ 2014-06-12 14:05 ` Florian Westphal
2014-06-12 23:12 ` Ken-ichirou MATSUZAWA
0 siblings, 1 reply; 13+ messages in thread
From: Florian Westphal @ 2014-06-12 14:05 UTC (permalink / raw)
To: Ken-ichirou MATSUZAWA
Cc: Florian Westphal, The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 14:05 ` Florian Westphal
@ 2014-06-12 23:12 ` Ken-ichirou MATSUZAWA
0 siblings, 0 replies; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-12 23:12 UTC (permalink / raw)
To: Florian Westphal; +Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-12 9:48 ` Florian Westphal
2014-06-12 11:33 ` Ken-ichirou MATSUZAWA
@ 2014-06-16 0:04 ` Ken-ichirou MATSUZAWA
2014-06-16 11:41 ` Florian Westphal
1 sibling, 1 reply; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-16 0:04 UTC (permalink / raw)
To: Florian Westphal; +Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-16 0:04 ` Ken-ichirou MATSUZAWA
@ 2014-06-16 11:41 ` Florian Westphal
2014-06-16 13:20 ` Ken-ichirou MATSUZAWA
0 siblings, 1 reply; 13+ messages in thread
From: Florian Westphal @ 2014-06-16 11:41 UTC (permalink / raw)
To: Ken-ichirou MATSUZAWA
Cc: Florian Westphal, The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-16 11:41 ` Florian Westphal
@ 2014-06-16 13:20 ` Ken-ichirou MATSUZAWA
2014-06-16 13:24 ` Florian Westphal
0 siblings, 1 reply; 13+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2014-06-16 13:20 UTC (permalink / raw)
To: Florian Westphal; +Cc: The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone
2014-06-16 13:20 ` Ken-ichirou MATSUZAWA
@ 2014-06-16 13:24 ` Florian Westphal
0 siblings, 0 replies; 13+ messages in thread
From: Florian Westphal @ 2014-06-16 13:24 UTC (permalink / raw)
To: Ken-ichirou MATSUZAWA
Cc: Florian Westphal, The netfilter developer mailinglist, Pablo Neira Ayuso
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-06-16 13:24 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-11 23:42 [RFC PATCH 1/1 libnetfilter_conntrack] zero value handling of mark and zone Ken-ichirou MATSUZAWA
2014-06-12 0:18 ` Florian Westphal
2014-06-12 5:19 ` Ken-ichirou MATSUZAWA
2014-06-12 9:48 ` Florian Westphal
2014-06-12 11:33 ` Ken-ichirou MATSUZAWA
2014-06-12 11:59 ` Florian Westphal
2014-06-12 13:43 ` Ken-ichirou MATSUZAWA
2014-06-12 14:05 ` Florian Westphal
2014-06-12 23:12 ` Ken-ichirou MATSUZAWA
2014-06-16 0:04 ` Ken-ichirou MATSUZAWA
2014-06-16 11:41 ` Florian Westphal
2014-06-16 13:20 ` Ken-ichirou MATSUZAWA
2014-06-16 13:24 ` Florian Westphal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.