nf_conntrack/ip_conntrack disrepencies

nf_conntrack/ip_conntrack disrepencies

[Amin Azez]

in nf_conntrack_tuple.h we have:
static inline int nf_ct_tuple_src_equal(const struct nf_conntrack_tuple *t1,
                                        const struct nf_conntrack_tuple *t2)
{
        return (t1->src.u3.all[0] == t2->src.u3.all[0] &&
                t1->src.u3.all[1] == t2->src.u3.all[1] &&
                t1->src.u3.all[2] == t2->src.u3.all[2] &&
                t1->src.u3.all[3] == t2->src.u3.all[3] &&
                t1->src.u.all == t2->src.u.all &&
                t1->src.l3num == t2->src.l3num &&
                t1->dst.protonum == t2->dst.protonum);
}

In ip_conntrack_tuple.h we have:
static inline int ip_ct_tuple_src_equal(const struct ip_conntrack_tuple *t1,
                                        const struct ip_conntrack_tuple *t2)
{
        return t1->src.ip == t2->src.ip
                && t1->src.u.all == t2->src.u.all;
}


nf_conntrack compares the dst.protonum (makes sense) but the
ip_conntrack doesn't. Is there the danger of ip_conntrack selecting
wrong matching tuples sometimes? Perhaps only in the rare case where the
hashes also match (unlikely) but I thought I would point this out.

Does protonum really belong in the dst member? Correspondingly, for
nf_conntrack, does l3num really belong in src?

The final thing that troubles me is that the dst member seems to have
things unrelated, like "dir" and "protonum", why are these part of
nf_conntrack_tuple.dst and not the top level nf_conntrack_tuple struct?

Sam

Re: nf_conntrack/ip_conntrack disrepencies

[Patrick McHardy]

Amin Azez wrote:
> in nf_conntrack_tuple.h we have:
> static inline int nf_ct_tuple_src_equal(const struct nf_conntrack_tuple *t1,
>                                         const struct nf_conntrack_tuple *t2)
> {
>         return (t1->src.u3.all[0] == t2->src.u3.all[0] &&
>                 t1->src.u3.all[1] == t2->src.u3.all[1] &&
>                 t1->src.u3.all[2] == t2->src.u3.all[2] &&
>                 t1->src.u3.all[3] == t2->src.u3.all[3] &&
>                 t1->src.u.all == t2->src.u.all &&
>                 t1->src.l3num == t2->src.l3num &&
>                 t1->dst.protonum == t2->dst.protonum);
> }
> 
> In ip_conntrack_tuple.h we have:
> static inline int ip_ct_tuple_src_equal(const struct ip_conntrack_tuple *t1,
>                                         const struct ip_conntrack_tuple *t2)
> {
>         return t1->src.ip == t2->src.ip
>                 && t1->src.u.all == t2->src.u.all;
> }
> 
> 
> nf_conntrack compares the dst.protonum (makes sense) but the
> ip_conntrack doesn't. Is there the danger of ip_conntrack selecting
> wrong matching tuples sometimes? Perhaps only in the rare case where the
> hashes also match (unlikely) but I thought I would point this out.

The only place where it is used in ip_conntrack is ip_ct_tuple_equal,
which also compares the dst tuple. I guess its the same in nf_conntrack,
so it could probably changed.

> Does protonum really belong in the dst member? Correspondingly, for
> nf_conntrack, does l3num really belong in src?

Not really, but it saves room. The compiler pads src and dst to
a multiple of 32 bit, so there are two bytes room.

> The final thing that troubles me is that the dst member seems to have
> things unrelated, like "dir" and "protonum", why are these part of
> nf_conntrack_tuple.dst and not the top level nf_conntrack_tuple struct?

See above.

more nf_conntrack/ip_conntrack questions

[Amin Azez]

2 questions, part style, relating to nf_conntrack and ip_conntrack

In nf_conntrack_tuple.h in

#define NF_CT_TUPLE_U_BLANK(tuple) \
do { \
(tuple)->src.u.all = 0; \
(tuple)->dst.u.all = 0; \
memset((tuple)->src.u3.all, 0, \
sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE); \
memset((tuple)->dst.u3.all, 0, \
sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE); \
} while (0)

why do we have:
sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE

as the size, instead of just:
sizeof((tuple)->dst.u3)

it seems to presume that:
1) .all will always be the biggest member
2) .all is always an array of NF_CT_TUPLE_L3SIZE of u_int32_t

I wonder why we need to duplicate this knowledge when a small definition
appears to suffice.

Also; and I asked about somehting similar before, why is
ip_conntrack_tuple.src almost exactly the same as
ip_conntrack_tuple.dst, but .src is defined in terms of pre-declared
structs and unions but dst is declared in terms of (nearly) identical
structs and unions. With nf_conntrack as far as I can tell they are the
same. The inline explanation of "manipulatable" doesn't seem to cover it.

Azez

Re: more nf_conntrack/ip_conntrack questions

[Patrick McHardy]

Amin Azez wrote:
> 2 questions, part style, relating to nf_conntrack and ip_conntrack
> 
> In nf_conntrack_tuple.h in
> 
> #define NF_CT_TUPLE_U_BLANK(tuple) \
> do { \
> (tuple)->src.u.all = 0; \
> (tuple)->dst.u.all = 0; \
> memset((tuple)->src.u3.all, 0, \
> sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE); \
> memset((tuple)->dst.u3.all, 0, \
> sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE); \
> } while (0)
> 
> why do we have:
> sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE
> 
> as the size, instead of just:
> sizeof((tuple)->dst.u3)
> 
> it seems to presume that:
> 1) .all will always be the biggest member
> 2) .all is always an array of NF_CT_TUPLE_L3SIZE of u_int32_t
> 
> I wonder why we need to duplicate this knowledge when a small definition
> appears to suffice.

I have no idea, but I agree that it looks confusing. You should ask
Yasuyuki, he wrote this code.

> Also; and I asked about somehting similar before, why is
> ip_conntrack_tuple.src almost exactly the same as
> ip_conntrack_tuple.dst, but .src is defined in terms of pre-declared
> structs and unions but dst is declared in terms of (nearly) identical
> structs and unions. With nf_conntrack as far as I can tell they are the
> same. The inline explanation of "manipulatable" doesn't seem to cover it.

Well, for one dst is only nearly identical. The "maniputable" part is
also used on its own for function arguments in the NAT code, the
non-manipulable part isn't, so there's no need to put it in a seperate
structure.

Re: more nf_conntrack/ip_conntrack questions

[Yasuyuki KOZAKAI]

Hi,

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 12 Sep 2005 12:42:51 +0200

> Amin Azez wrote:
> > 2 questions, part style, relating to nf_conntrack and ip_conntrack
> > 
> > In nf_conntrack_tuple.h in
> > 
> > #define NF_CT_TUPLE_U_BLANK(tuple) \
> > do { \
> > (tuple)->src.u.all = 0; \
> > (tuple)->dst.u.all = 0; \
> > memset((tuple)->src.u3.all, 0, \
> > sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE); \
> > memset((tuple)->dst.u3.all, 0, \
> > sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE); \
> > } while (0)
> > 
> > why do we have:
> > sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE
> > 
> > as the size, instead of just:
> > sizeof((tuple)->dst.u3)
> > 
> > it seems to presume that:
> > 1) .all will always be the biggest member
> > 2) .all is always an array of NF_CT_TUPLE_L3SIZE of u_int32_t

Yes,

> > I wonder why we need to duplicate this knowledge when a small definition
> > appears to suffice.
> 
> I have no idea, but I agree that it looks confusing. You should ask
> Yasuyuki, he wrote this code.

This is for nf_ct_tuple_mask_cmp(), which compares 2 tuples with mask.
I didn't want to use 'switch - case' for each layer 3 protocol, then I
tried to generalize it like ip_ct_tuple_mask_cmp() like layer 4 protocol
part. Then I introduced the member 'all'.

 And for efficiency, it applies mask to tuple per 32bits instead of 8bits.
NF_CT_TUPLE_L3SIZE is used for checking loop limitation. I think it's easier
to read than sizeof(tuple->dst.u3)/sizeof(u_int32_t). And explicit definition
of array 'all' assures that sizeof(tuple->dst.u3) is divisible with
sizeof(u_int32_t) independent on contents of 'u3'.

Of cause, smarter idea than this way is welcome.

-----------------------------------------------------------------
Yasuyuki Kozakai @ USAGI Project <yasuyuki.kozakai@toshiba.co.jp>

Re: more nf_conntrack/ip_conntrack questions

[Yasuyuki KOZAKAI]

Sorry,

From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>
Date: Mon, 12 Sep 2005 20:29:07 +0900 (JST)

>  then I
> tried to generalize it like ip_ct_tuple_mask_cmp() like layer 4 protocol
> part.

What I want to say is ...

I tried to generalize it like the handling of layer 4 protocol part
in ip_ct_tuple_mask_cmp().

-----------------------------------------------------------------
Yasuyuki Kozakai @ USAGI Project <yasuyuki.kozakai@toshiba.co.jp>

Re: more nf_conntrack/ip_conntrack questions

[Amin Azez]

Yasuyuki KOZAKAI wrote:

>Sorry,
>
>From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>
>Date: Mon, 12 Sep 2005 20:29:07 +0900 (JST)
>
>  
>
>> then I
>>tried to generalize it like ip_ct_tuple_mask_cmp() like layer 4 protocol
>>part.
>>    
>>
>
>What I want to say is ...
>
>I tried to generalize it like the handling of layer 4 protocol part
>in ip_ct_tuple_mask_cmp().
>
>  
>

Sure, I don't have any fault with the structures, just the way the
sizeof is measured for initial zero-ing.

Sam

Re: more nf_conntrack/ip_conntrack questions

[Yasuyuki KOZAKAI]

[-- Attachment #1: Type: Text/Plain, Size: 468 bytes --]

From: Amin Azez <azez@ufomechanic.net>
Date: Mon, 12 Sep 2005 13:04:40 +0100

> Sure, I don't have any fault with the structures, just the way the
> sizeof is measured for initial zero-ing.

Oh, it's reasonable. Thank you for notice.

Harald, please apply this patch.

Signed-off-by: Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp>

-----------------------------------------------------------------
Yasuyuki Kozakai @ USAGI Project <yasuyuki.kozakai@toshiba.co.jp>


[-- Attachment #2: tuple-l3size.patch --]
[-- Type: Text/Plain, Size: 2444 bytes --]

[NETFILTER] simplifies calculation of size of l3 part in tuple

Signed-off-by: Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp>

---
commit 0a330af3412fbfaecd694acd572000f29f2a407c
tree 76e0dc454fa6723e98b1fc423fbb222985612960
parent b24ffb6730679e783e77eb3bc721ddd50fdf9ec7
author Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp> Mon, 12 Sep 2005 21:49:32 +0900
committer Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp> Mon, 12 Sep 2005 21:49:32 +0900

 include/linux/netfilter/nf_conntrack_tuple.h |    6 ++----
 net/netfilter/nf_conntrack_l3proto_generic.c |    8 ++++----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/linux/netfilter/nf_conntrack_tuple.h b/include/linux/netfilter/nf_conntrack_tuple.h
--- a/include/linux/netfilter/nf_conntrack_tuple.h
+++ b/include/linux/netfilter/nf_conntrack_tuple.h
@@ -104,10 +104,8 @@ struct nf_conntrack_tuple
         do {                                                    	\
                 (tuple)->src.u.all = 0;                         	\
                 (tuple)->dst.u.all = 0;                         	\
-		memset((tuple)->src.u3.all, 0,				\
-		       sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE);		\
-		memset((tuple)->dst.u3.all, 0,				\
-		       sizeof(u_int32_t)*NF_CT_TUPLE_L3SIZE);		\
+		memset(&(tuple)->src.u3, 0, sizeof((tuple)->src.u3));	\
+		memset(&(tuple)->dst.u3, 0, sizeof((tuple)->dst.u3));	\
         } while (0)
 
 #ifdef __KERNEL__
diff --git a/net/netfilter/nf_conntrack_l3proto_generic.c b/net/netfilter/nf_conntrack_l3proto_generic.c
--- a/net/netfilter/nf_conntrack_l3proto_generic.c
+++ b/net/netfilter/nf_conntrack_l3proto_generic.c
@@ -43,8 +43,8 @@ DECLARE_PER_CPU(struct nf_conntrack_stat
 static int generic_pkt_to_tuple(const struct sk_buff *skb, unsigned int nhoff,
 				struct nf_conntrack_tuple *tuple)
 {
-	memset(tuple->src.u3.all, 0, NF_CT_TUPLE_L3SIZE);
-	memset(tuple->dst.u3.all, 0, NF_CT_TUPLE_L3SIZE);
+	memset(&tuple->src.u3, 0, sizeof(tuple->src.u3));
+	memset(&tuple->dst.u3, 0, sizeof(tuple->dst.u3));
 
 	return 1;
 }
@@ -52,8 +52,8 @@ static int generic_pkt_to_tuple(const st
 static int generic_invert_tuple(struct nf_conntrack_tuple *tuple,
 			   const struct nf_conntrack_tuple *orig)
 {
-	memset(tuple->src.u3.all, 0, NF_CT_TUPLE_L3SIZE);
-	memset(tuple->dst.u3.all, 0, NF_CT_TUPLE_L3SIZE);
+	memset(&tuple->src.u3, 0, sizeof(tuple->src.u3));
+	memset(&tuple->dst.u3, 0, sizeof(tuple->dst.u3));
 
 	return 1;
 }

Re: more nf_conntrack/ip_conntrack questions

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On Mon, Sep 12, 2005 at 09:56:16PM +0900, Yasuyuki KOZAKAI wrote:
> From: Amin Azez <azez@ufomechanic.net>
> Date: Mon, 12 Sep 2005 13:04:40 +0100
> 
> > Sure, I don't have any fault with the structures, just the way the
> > sizeof is measured for initial zero-ing.
> 
> Oh, it's reasonable. Thank you for notice.
> 
> Harald, please apply this patch.

thanks, applied to netfilter-2.6.14#nf_conntrack

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]