[PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Pablo Neira Ayuso]

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

Hi Yasuyuki,

Currently we get an oops with nf_conntrack_netlink + nf_conntrack_ftp
because l3num is set to 0xFFFF for the expectation mask. At first sight,
this is correct because l3num is u_int16_t, but the size of the layer-3
array of protocol handlers is AF_MAX (32).

I could add some checking to verify that l3num is less than 32 in
nf_conntrack_find_l3proto, but such checking is only required for
nf_conntrack_ftp and further application helpers. AFAICS, this is the
cleanest way to fix this problem. Any other suggestion?

Cheers,
Pablo

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

[-- Attachment #2: x1 --]
[-- Type: text/plain, Size: 849 bytes --]

Set l3num to 0x1F (32) in expectation masks since the size of the layer 3 
procotol handler array is AF_MAX. This fixes a panic at expectation dumping
and event notification.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: netfilter-2.6.14.git/net/netfilter/nf_conntrack_ftp.c
===================================================================
--- netfilter-2.6.14.git.orig/net/netfilter/nf_conntrack_ftp.c	2006-01-08 21:24:44.000000000 +0100
+++ netfilter-2.6.14.git/net/netfilter/nf_conntrack_ftp.c	2006-01-08 21:33:52.000000000 +0100
@@ -574,7 +574,7 @@ static int help(struct sk_buff **pskb,
 	exp->tuple.dst.protonum = IPPROTO_TCP;
 
 	exp->mask = (struct nf_conntrack_tuple)
-		    { .src = { .l3num = 0xFFFF,
+		    { .src = { .l3num = 0x001F,
 			       .u = { .tcp = { 0 }},
 			     },
 		      .dst = { .protonum = 0xFF,

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Hi Yasuyuki,
> 
> Currently we get an oops with nf_conntrack_netlink + nf_conntrack_ftp
> because l3num is set to 0xFFFF for the expectation mask. At first sight,
> this is correct because l3num is u_int16_t, but the size of the layer-3
> array of protocol handlers is AF_MAX (32).
> 
> I could add some checking to verify that l3num is less than 32 in
> nf_conntrack_find_l3proto, but such checking is only required for
> nf_conntrack_ftp and further application helpers. AFAICS, this is the
> cleanest way to fix this problem. Any other suggestion?

What is the exact cause for the crash? I looked over the code, but
couldn't spot it.

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Yasuyuki KOZAKAI]

Hi,

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 13 Jan 2006 09:58:15 +0100

> Pablo Neira Ayuso wrote:
> > Hi Yasuyuki,
> > 
> > Currently we get an oops with nf_conntrack_netlink + nf_conntrack_ftp
> > because l3num is set to 0xFFFF for the expectation mask. At first sight,
> > this is correct because l3num is u_int16_t, but the size of the layer-3
> > array of protocol handlers is AF_MAX (32).
> > 
> > I could add some checking to verify that l3num is less than 32 in
> > nf_conntrack_find_l3proto, but such checking is only required for
> > nf_conntrack_ftp and further application helpers. AFAICS, this is the
> > cleanest way to fix this problem. Any other suggestion?
> 
> What is the exact cause for the crash? I looked over the code, but
> couldn't spot it.

I think that root caurse is missing check at __nf_ct_{l3}proto_find().
I'll write patch for it. It will free people from taking care about
expectation mask.

Regareds,

-- Yasuyuki Kozakai

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
>>>Currently we get an oops with nf_conntrack_netlink + nf_conntrack_ftp
>>>because l3num is set to 0xFFFF for the expectation mask. At first sight,
>>>this is correct because l3num is u_int16_t, but the size of the layer-3
>>>array of protocol handlers is AF_MAX (32).
>>>
>>>I could add some checking to verify that l3num is less than 32 in
>>>nf_conntrack_find_l3proto, but such checking is only required for
>>>nf_conntrack_ftp and further application helpers. AFAICS, this is the
>>>cleanest way to fix this problem. Any other suggestion?
>>
>>What is the exact cause for the crash? I looked over the code, but
>>couldn't spot it.

Ah I see now, see the reason is right there in Pablo's mail :)

> I think that root caurse is missing check at __nf_ct_{l3}proto_find().
> I'll write patch for it. It will free people from taking care about
> expectation mask.

I agree, that seems to be the best solution.

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Yasuyuki KOZAKAI]

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


From: Patrick McHardy <kaber@trash.net>
Date: Fri, 13 Jan 2006 10:04:34 +0100

> > I think that root caurse is missing check at __nf_ct_{l3}proto_find().
> > I'll write patch for it. It will free people from taking care about
> > expectation mask.
> 
> I agree, that seems to be the best solution.

Here you are. How about this ?

-- Yasuyuki Kozakai


[-- Attachment #2: check-l3num.patch --]
[-- Type: Text/Plain, Size: 2541 bytes --]

[NETFILTER] nf_conntrack: check address family when finding protocol module

__nf_conntrack_{l3}proto_find() doesn't check the passed protocol family,
then it's possible to touch out of the array which has only AF_MAX items.

spotted by Pablo Neira Ayuso.

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

---
commit 878d327ca0ca7983e3876cbdeb03679e801b637e
tree f559501a7202e5ec10106821f927460e5cc2bc11
parent 2e4e6a17af35be359cc8f1c924f8f198fbd478cc
author Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp> Fri, 13 Jan 2006 18:27:38 +0900
committer Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp> Fri, 13 Jan 2006 18:27:38 +0900

 include/net/netfilter/nf_conntrack_l3proto.h |   15 +++++++++------
 net/netfilter/nf_conntrack_core.c            |    2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
index 67856eb..417d6bd 100644
--- a/include/net/netfilter/nf_conntrack_l3proto.h
+++ b/include/net/netfilter/nf_conntrack_l3proto.h
@@ -88,12 +88,6 @@ extern struct nf_conntrack_l3proto *nf_c
 extern int nf_conntrack_l3proto_register(struct nf_conntrack_l3proto *proto);
 extern void nf_conntrack_l3proto_unregister(struct nf_conntrack_l3proto *proto);
 
-static inline struct nf_conntrack_l3proto *
-__nf_ct_l3proto_find(u_int16_t l3proto)
-{
-	return nf_ct_l3protos[l3proto];
-}
-
 extern struct nf_conntrack_l3proto *
 nf_ct_l3proto_find_get(u_int16_t l3proto);
 
@@ -103,4 +97,13 @@ extern void nf_ct_l3proto_put(struct nf_
 extern struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv4;
 extern struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6;
 extern struct nf_conntrack_l3proto nf_conntrack_generic_l3proto;
+
+static inline struct nf_conntrack_l3proto *
+__nf_ct_l3proto_find(u_int16_t l3proto)
+{
+	if (unlikely(l3proto > AF_MAX))
+		return &nf_conntrack_generic_l3proto;
+	return nf_ct_l3protos[l3proto];
+}
+
 #endif /*_NF_CONNTRACK_L3PROTO_H*/
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 62bb509..9a147dd 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -188,7 +188,7 @@ extern struct nf_conntrack_protocol nf_c
 struct nf_conntrack_protocol *
 __nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol)
 {
-	if (unlikely(nf_ct_protos[l3proto] == NULL))
+	if (unlikely(l3proto > AF_MAX || nf_ct_protos[l3proto] == NULL))
 		return &nf_conntrack_generic_protocol;
 
 	return nf_ct_protos[l3proto][protocol];

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Fri, 13 Jan 2006 10:04:34 +0100
> 
> 
>>>I think that root caurse is missing check at __nf_ct_{l3}proto_find().
>>>I'll write patch for it. It will free people from taking care about
>>>expectation mask.
>>
>>I agree, that seems to be the best solution.
> 
> 
> Here you are. How about this ?

Thanks, looks good. Applied.

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[YOSHIFUJI Hideaki / 吉藤英明]

In article <43C776A6.2010306@trash.net> (at Fri, 13 Jan 2006 10:45:10 +0100), Patrick McHardy <kaber@trash.net> says:

> Yasuyuki KOZAKAI wrote:
> > From: Patrick McHardy <kaber@trash.net>
> > Date: Fri, 13 Jan 2006 10:04:34 +0100
> > 
> > 
> >>>I think that root caurse is missing check at __nf_ct_{l3}proto_find().
> >>>I'll write patch for it. It will free people from taking care about
> >>>expectation mask.
> >>
> >>I agree, that seems to be the best solution.
> > 
> > 
> > Here you are. How about this ?
> 
> Thanks, looks good. Applied.

Really? ;-)

Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

diff -u b/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
--- b/include/net/netfilter/nf_conntrack_l3proto.h
+++ b/include/net/netfilter/nf_conntrack_l3proto.h
@@ -101,7 +101,7 @@
 static inline struct nf_conntrack_l3proto *
 __nf_ct_l3proto_find(u_int16_t l3proto)
 {
-	if (unlikely(l3proto > AF_MAX))
+	if (unlikely(l3proto >= AF_MAX))
 		return &nf_conntrack_generic_l3proto;
 	return nf_ct_l3protos[l3proto];
 }
diff -u b/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
--- b/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -188,7 +188,7 @@
 struct nf_conntrack_protocol *
 __nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol)
 {
-	if (unlikely(l3proto > AF_MAX || nf_ct_protos[l3proto] == NULL))
+	if (unlikely(l3proto >= AF_MAX || nf_ct_protos[l3proto] == NULL))
 		return &nf_conntrack_generic_protocol;
 
 	return nf_ct_protos[l3proto][protocol];

-- 
YOSHIFUJI Hideaki @ USAGI Project  <yoshfuji@linux-ipv6.org>
GPG-FP  : 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Patrick McHardy]

YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[ wrote:
>>Thanks, looks good. Applied.
> 
> 
> Really? ;-)

Thanks for noticing, I'm somehow blind this morning.

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Pablo Neira Ayuso]

Yasuyuki KOZAKAI wrote:
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 62bb509..9a147dd 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -188,7 +188,7 @@ extern struct nf_conntrack_protocol nf_c
>  struct nf_conntrack_protocol *
>  __nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol)
>  {
> -	if (unlikely(nf_ct_protos[l3proto] == NULL))
> +	if (unlikely(l3proto > AF_MAX || nf_ct_protos[l3proto] == NULL))

Could the unlikely statement be bad at expectation dumping?

cheers,
Pablo

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification

[Yasuyuki KOZAKAI]

Hi, Pablo,

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 13 Jan 2006 12:17:16 +0100

> Yasuyuki KOZAKAI wrote:
> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index 62bb509..9a147dd 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -188,7 +188,7 @@ extern struct nf_conntrack_protocol nf_c
> >  struct nf_conntrack_protocol *
> >  __nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol)
> >  {
> > -	if (unlikely(nf_ct_protos[l3proto] == NULL))
> > +	if (unlikely(l3proto > AF_MAX || nf_ct_protos[l3proto] == NULL))
> 
> Could the unlikely statement be bad at expectation dumping?

Indeed. But please let me think about this some more.
I feel strange to find l3proto with 0xffff as address family or to find proto
with 0xff as protocol number.

After all, ctnetlink_dump_tuples() gets nf_conntrack_l3proto_generic /
nf_conntrack_proto_generic and they doesn't fill any contents, right ?

If so, I think that it's better to make ctnetlink_exp_dump_expect() use
l3proto/proto for &exp->tuple to dump &exp->mask, and then
we would not need to remove above unlikely().

Regards,

-- Yasuyuki Kozaki

expectation mask handling in nfctnetlink (Was Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification)

[Yasuyuki KOZAKAI]

Hi, Pablo,

From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>
Date: Sun, 15 Jan 2006 22:07:39 +0900 (JST)

> I feel strange to find l3proto with 0xffff as address family or to find proto
> with 0xff as protocol number.
> 
> After all, ctnetlink_dump_tuples() gets nf_conntrack_l3proto_generic /
> nf_conntrack_proto_generic and they doesn't fill any contents, right ?
> 
> If so, I think that it's better to make ctnetlink_exp_dump_expect() use
> l3proto/proto for &exp->tuple to dump &exp->mask, and then
> we would not need to remove above unlikely().

I've re-visited this. For now, I'm not sure this is the best solution
because I don't see how format and contents of expectation mask
is required by libnetfilter_conntrack. It doesn't use dumped expectation
mask at all.

Then the other idea can be considerd, too. For example, dumping union of
l3part(mask->u3.all) and union of proto part(mask->u.all) without assumption
of specific address family/protocol. Do you have any idea on this ?

BTW, Harald, I have not read your userspace conntrack helper yet, but I think
that this issue might relate to it, too.

Regards,

-- Yasuyuki Kozakai

Re: expectation mask handling in nfctnetlink (Was Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification)

[Pablo Neira Ayuso]

Hi Yasuyuki,

I'd like to recover this thread.

Yasuyuki KOZAKAI wrote:
>>I feel strange to find l3proto with 0xffff as address family or to find proto
>>with 0xff as protocol number.
>>
>>After all, ctnetlink_dump_tuples() gets nf_conntrack_l3proto_generic /
>>nf_conntrack_proto_generic and they doesn't fill any contents, right ?
>>
>>If so, I think that it's better to make ctnetlink_exp_dump_expect() use
>>l3proto/proto for &exp->tuple to dump &exp->mask, and then
>>we would not need to remove above unlikely().
> 
> I've re-visited this. For now, I'm not sure this is the best solution
> because I don't see how format and contents of expectation mask
> is required by libnetfilter_conntrack. It doesn't use dumped expectation
> mask at all.

Maybe userspace helpers could need that information some day. I don't
want to add limitations to libnetfilter_conntrack just because we have
some assumptions.

> Then the other idea can be considerd, too. For example, dumping union of
> l3part(mask->u3.all) and union of proto part(mask->u.all) without assumption
> of specific address family/protocol. Do you have any idea on this ?

We shouldn't send the whole union via netlink. If the union is modified
further, it will break backward compatibility for userspace applications
since the size could change.

So, the possibilities I see up to now are:

a) just remove the unlikely() statement.
Drawback: Not so efficient, since that checking isn't always required.

b) back to my idea: set l3num to the maximum known value (0x1F), and add
a bug trap in nf_ct_expect_related to check that nobody uses a value >=
AF_MAX.
Drawback: People surely will hit bug while writing their own helpers.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: expectation mask handling in nfctnetlink (Was Re: [PATCH] fix nf_conntrack_netlink expectation dumping/event notification)

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> a) just remove the unlikely() statement.
> Drawback: Not so efficient, since that checking isn't always required.

I'm not sure I understand the problem. Is it about the unlikely in
__nf_ct_proto_find? That one is most likely not making any difference
at all, since there is no code to reorder.

[RFC] [PATCH] Fix expectation mask dumping (Was Re: expectation mask handling in nfctnetlink)

[Pablo Neira Ayuso]

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

Hi,

This patch introduces the function ctnetlink_exp_dump_mask, that
correctly dumps the expectation mask. Such function uses the l3num value
from the expectation tuple that is a valid layer 3 protocol number.

Besides, this modification introduces the attribute CTA_IP_L3NUM.
Although the layer 3 protocol information is sent in the nfnetlink
header, if the message contains information about an expectation, it
will contain information about the master conntrack (just one of the
tuples), the expectation tuple and the expectation mask. In this case,
the value of l3num in the expectation mask is not the same that is set
in the nfnetlink message. That is why we need another field that contain
the value of l3num.

Now libnetfilter_conntrack can use the CTA_IP_L3NUM attribute, but if
this attribute is not present in the message, it can use the information
available in the nfnetlink header message.

comments?

cheers,
Pablo

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris


[-- Attachment #2: y --]
[-- Type: text/plain, Size: 5559 bytes --]

[RFC][CTNETLINK] Fix expectaction mask dumping

The expectation mask has some particularities that make handle in a different
way. The protocol number fields can be set to non-valid protocols, ie. l3num
is set to 0xFFFF. Since that protocol does not exist, the mask tuple will not
be dumped. Moreover, this results in a kernel panic when nf_conntrack accesses
the array of protocol handlers, that is PF_MAX (0x1F) long.

This patch introduces the function ctnetlink_exp_dump_mask, that correctly
dumps the expectation mask. Such function uses the l3num value from the
expectation tuple that is a valid layer 3 protocol number.

Besides, this modification introduces the attribute CTA_IP_L3NUM. Although
the layer 3 protocol information is sent in the nfnetlink header, if the
message contains information about an expectation, it will contain information
about the master conntrack (just one of the tuples), the expectation tuple and
the expectation mask. In this case, the value of l3num in the expectation mask
is not the same that is set in the nfnetlink message. That is why we need 
another field that contain the value of l3num.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2006-02-01 20:17:05.000000000 +0100
+++ net-2.6.git/net/netfilter/nf_conntrack_netlink.c	2006-02-02 01:28:09.000000000 +0100
@@ -55,20 +55,18 @@ static char __initdata version[] = "0.92
 
 static inline int
 ctnetlink_dump_tuples_proto(struct sk_buff *skb, 
-			    const struct nf_conntrack_tuple *tuple)
+			    const struct nf_conntrack_tuple *tuple,
+			    struct nf_conntrack_protocol *proto)
 {
-	struct nf_conntrack_protocol *proto;
 	int ret = 0;
+	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
 
 	NFA_PUT(skb, CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
 
-	/* If no protocol helper is found, this function will return the
-	 * generic protocol helper, so proto won't *ever* be NULL */
-	proto = nf_ct_proto_find_get(tuple->src.l3num, tuple->dst.protonum);
 	if (likely(proto->tuple_to_nfattr))
 		ret = proto->tuple_to_nfattr(skb, tuple);
 	
-	nf_ct_proto_put(proto);
+	NFA_NEST_END(skb, nest_parms);	
 
 	return ret;
 
@@ -77,33 +75,46 @@ nfattr_failure:
 }
 
 static inline int
-ctnetlink_dump_tuples(struct sk_buff *skb, 
-		      const struct nf_conntrack_tuple *tuple)
+ctnetlink_dump_tuples_ip(struct sk_buff *skb,
+			 const struct nf_conntrack_tuple *tuple,
+			 struct nf_conntrack_l3proto *l3proto)
 {
-	struct nfattr *nest_parms;
-	struct nf_conntrack_l3proto *l3proto;
 	int ret = 0;
-	
-	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
-	
-	nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
+	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
+
+	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int16_t), &tuple->src.l3num);
+
 	if (likely(l3proto->tuple_to_nfattr))
 		ret = l3proto->tuple_to_nfattr(skb, tuple);
+
 	NFA_NEST_END(skb, nest_parms);
 
+	return ret;
+
+nfattr_failure:
+	return -1;
+}
+
+static inline int
+ctnetlink_dump_tuples(struct sk_buff *skb, 
+		      const struct nf_conntrack_tuple *tuple)
+{
+	int ret = 0;
+	struct nf_conntrack_l3proto *l3proto;
+	struct nf_conntrack_protocol *proto;
+
+	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	ret = ctnetlink_dump_tuples_ip(skb, tuple, l3proto);
 	nf_ct_l3proto_put(l3proto);
 
 	if (unlikely(ret < 0))
 		return ret;
 
-	nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
-	ret = ctnetlink_dump_tuples_proto(skb, tuple);
-	NFA_NEST_END(skb, nest_parms);
+	proto = nf_ct_proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	ret = ctnetlink_dump_tuples_proto(skb, tuple, proto);
+	nf_ct_proto_put(proto);
 
 	return ret;
-
-nfattr_failure:
-	return -1;
 }
 
 static inline int
@@ -1150,6 +1161,29 @@ nfattr_failure:
 }			
 
 static inline int
+ctnetlink_exp_dump_mask(struct sk_buff *skb, 
+			const struct nf_conntrack_tuple *tuple,
+			const struct nf_conntrack_tuple *mask)
+{
+	int ret = 0;
+	struct nf_conntrack_l3proto *l3proto;
+	struct nf_conntrack_protocol *proto;
+
+	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	ret = ctnetlink_dump_tuples_ip(skb, mask, l3proto);
+	nf_ct_l3proto_put(l3proto);
+
+	if (unlikely(ret < 0))
+		return ret;
+
+	proto = nf_ct_proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	ret = ctnetlink_dump_tuples_proto(skb, mask, proto);
+	nf_ct_proto_put(proto);
+
+	return ret;
+}
+
+static inline int
 ctnetlink_exp_dump_expect(struct sk_buff *skb,
                           const struct nf_conntrack_expect *exp)
 {
@@ -1159,7 +1193,7 @@ ctnetlink_exp_dump_expect(struct sk_buff
 
 	if (ctnetlink_exp_dump_tuple(skb, &exp->tuple, CTA_EXPECT_TUPLE) < 0)
 		goto nfattr_failure;
-	if (ctnetlink_exp_dump_tuple(skb, &exp->mask, CTA_EXPECT_MASK) < 0)
+	if (ctnetlink_exp_dump_mask(skb, &exp->tuple, &exp->mask) < 0)
 		goto nfattr_failure;
 	if (ctnetlink_exp_dump_tuple(skb,
 				 &master->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
Index: net-2.6.git/include/linux/netfilter/nfnetlink_conntrack.h
===================================================================
--- net-2.6.git.orig/include/linux/netfilter/nfnetlink_conntrack.h	2006-02-01 20:01:44.000000000 +0100
+++ net-2.6.git/include/linux/netfilter/nfnetlink_conntrack.h	2006-02-01 20:52:05.000000000 +0100
@@ -52,6 +52,7 @@ enum ctattr_ip {
 	CTA_IP_V4_DST,
 	CTA_IP_V6_SRC,
 	CTA_IP_V6_DST,
+	CTA_IP_L3NUM,
 	__CTA_IP_MAX
 };
 #define CTA_IP_MAX (__CTA_IP_MAX - 1)

Re: [RFC] [PATCH] Fix expectation mask dumping

[Yasuyuki KOZAKAI]

Hi, Pablo,

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 02 Feb 2006 01:45:09 +0100

> Hi,
> 
> This patch introduces the function ctnetlink_exp_dump_mask, that
> correctly dumps the expectation mask. Such function uses the l3num value
> from the expectation tuple that is a valid layer 3 protocol number.
> 
> Besides, this modification introduces the attribute CTA_IP_L3NUM.
> Although the layer 3 protocol information is sent in the nfnetlink
> header, if the message contains information about an expectation, it
> will contain information about the master conntrack (just one of the
> tuples), the expectation tuple and the expectation mask. In this case,
> the value of l3num in the expectation mask is not the same that is set
> in the nfnetlink message. That is why we need another field that contain
> the value of l3num.
> 
> Now libnetfilter_conntrack can use the CTA_IP_L3NUM attribute, but if
> this attribute is not present in the message, it can use the information
> available in the nfnetlink header message.
> 
> comments?

sounds good idea.

>  static inline int
> -ctnetlink_dump_tuples(struct sk_buff *skb, 
> -		      const struct nf_conntrack_tuple *tuple)
> +ctnetlink_dump_tuples_ip(struct sk_buff *skb,
> +			 const struct nf_conntrack_tuple *tuple,
> +			 struct nf_conntrack_l3proto *l3proto)
>  {
> -	struct nfattr *nest_parms;
> -	struct nf_conntrack_l3proto *l3proto;
>  	int ret = 0;
> -	
> -	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
> -	
> -	nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
> +	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
> +
> +	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int16_t), &tuple->src.l3num);

Well, I think that 8bits are enough. It seems that it's time to change
'l3num' field in tuple to 8bits to avoid confusing.

If everyone has no objection, please just change this to 8bits, then I'll
write a patch to do that on top of your patch.

-- Yasuyuki Kozakai

[PATCH] Fix expectation mask dumping, take #2

[Pablo Neira Ayuso]

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

Hi,

@Patrick: this patch fixes the problem that `[NETFILTER 03/14]:
nf_conntrack: check address family when finding protocol module` works
around. So I think that such patch needs to be reverted :(

Description:

This patch introduces the function ctnetlink_exp_dump_mask, that
correctly dumps the expectation mask. Such function uses the l3num value
from the expectation tuple that is a valid layer 3 protocol number.

Besides, this modification introduces the attribute CTA_IP_L3NUM.
Although the layer 3 protocol information is sent in the nfnetlink
header, if the message contains information about an expectation, it
will contain information about the master conntrack (just one of the
tuples), the expectation tuple and the expectation mask. In this case,
the value of l3num in the expectation mask is not the same that is set
in the nfnetlink message. That is why we need another field that contain
the value of l3num.

Now libnetfilter_conntrack can use the CTA_IP_L3NUM attribute, but if
this attribute is not present in the message, it can use the information
available in the nfnetlink header message.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

[-- Attachment #2: y --]
[-- Type: text/plain, Size: 9916 bytes --]

[CTNETLINK] Fix expectaction mask dumping

The expectation mask has some particularities that make handle in a different
way. The protocol number fields can be set to non-valid protocols, ie. l3num
is set to 0xFFFF. Since that protocol does not exist, the mask tuple will not
be dumped. Moreover, this results in a kernel panic when nf_conntrack accesses
the array of protocol handlers, that is PF_MAX (0x1F) long.

This patch introduces the function ctnetlink_exp_dump_mask, that correctly
dumps the expectation mask. Such function uses the l3num value from the
expectation tuple that is a valid layer 3 protocol number.

Besides, this modification introduces the attribute CTA_IP_L3NUM. Although
the layer 3 protocol information is sent in the nfnetlink header, if the
message contains information about an expectation, it will contain information
about the master conntrack (just one of the tuples), the expectation tuple and
the expectation mask. In this case, the value of l3num in the expectation mask
is not the same that is set in the nfnetlink message. That is why we need 
another field that contain the value of l3num.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2006-02-01 20:17:05.000000000 +0100
+++ net-2.6.git/net/netfilter/nf_conntrack_netlink.c	2006-02-03 18:02:39.000000000 +0100
@@ -4,7 +4,7 @@
  * (C) 2001 by Jay Schulist <jschlst@samba.org>
  * (C) 2002-2005 by Harald Welte <laforge@gnumonks.org>
  * (C) 2003 by Patrick Mchardy <kaber@trash.net>
- * (C) 2005 by Pablo Neira Ayuso <pablo@eurodev.net>
+ * (C) 2005-2006 by Pablo Neira Ayuso <pablo@eurodev.net>
  *
  * I've reworked this stuff to use attributes instead of conntrack 
  * structures. 5.44 am. I need more tea. --pablo 05/07/11.
@@ -55,20 +55,18 @@ static char __initdata version[] = "0.92
 
 static inline int
 ctnetlink_dump_tuples_proto(struct sk_buff *skb, 
-			    const struct nf_conntrack_tuple *tuple)
+			    const struct nf_conntrack_tuple *tuple,
+			    struct nf_conntrack_protocol *proto)
 {
-	struct nf_conntrack_protocol *proto;
 	int ret = 0;
+	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
 
 	NFA_PUT(skb, CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
 
-	/* If no protocol helper is found, this function will return the
-	 * generic protocol helper, so proto won't *ever* be NULL */
-	proto = nf_ct_proto_find_get(tuple->src.l3num, tuple->dst.protonum);
 	if (likely(proto->tuple_to_nfattr))
 		ret = proto->tuple_to_nfattr(skb, tuple);
 	
-	nf_ct_proto_put(proto);
+	NFA_NEST_END(skb, nest_parms);	
 
 	return ret;
 
@@ -77,33 +75,46 @@ nfattr_failure:
 }
 
 static inline int
-ctnetlink_dump_tuples(struct sk_buff *skb, 
-		      const struct nf_conntrack_tuple *tuple)
+ctnetlink_dump_tuples_ip(struct sk_buff *skb,
+			 const struct nf_conntrack_tuple *tuple,
+			 struct nf_conntrack_l3proto *l3proto)
 {
-	struct nfattr *nest_parms;
-	struct nf_conntrack_l3proto *l3proto;
 	int ret = 0;
-	
-	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
-	
-	nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
+	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
+
+	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int8_t), &tuple->src.l3num);
+
 	if (likely(l3proto->tuple_to_nfattr))
 		ret = l3proto->tuple_to_nfattr(skb, tuple);
+
 	NFA_NEST_END(skb, nest_parms);
 
+	return ret;
+
+nfattr_failure:
+	return -1;
+}
+
+static inline int
+ctnetlink_dump_tuples(struct sk_buff *skb, 
+		      const struct nf_conntrack_tuple *tuple)
+{
+	int ret;
+	struct nf_conntrack_l3proto *l3proto;
+	struct nf_conntrack_protocol *proto;
+
+	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	ret = ctnetlink_dump_tuples_ip(skb, tuple, l3proto);
 	nf_ct_l3proto_put(l3proto);
 
 	if (unlikely(ret < 0))
 		return ret;
 
-	nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
-	ret = ctnetlink_dump_tuples_proto(skb, tuple);
-	NFA_NEST_END(skb, nest_parms);
+	proto = nf_ct_proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	ret = ctnetlink_dump_tuples_proto(skb, tuple, proto);
+	nf_ct_proto_put(proto);
 
 	return ret;
-
-nfattr_failure:
-	return -1;
 }
 
 static inline int
@@ -1150,6 +1161,29 @@ nfattr_failure:
 }			
 
 static inline int
+ctnetlink_exp_dump_mask(struct sk_buff *skb, 
+			const struct nf_conntrack_tuple *tuple,
+			const struct nf_conntrack_tuple *mask)
+{
+	int ret;
+	struct nf_conntrack_l3proto *l3proto;
+	struct nf_conntrack_protocol *proto;
+
+	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	ret = ctnetlink_dump_tuples_ip(skb, mask, l3proto);
+	nf_ct_l3proto_put(l3proto);
+
+	if (unlikely(ret < 0))
+		return ret;
+
+	proto = nf_ct_proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	ret = ctnetlink_dump_tuples_proto(skb, mask, proto);
+	nf_ct_proto_put(proto);
+
+	return ret;
+}
+
+static inline int
 ctnetlink_exp_dump_expect(struct sk_buff *skb,
                           const struct nf_conntrack_expect *exp)
 {
@@ -1159,7 +1193,7 @@ ctnetlink_exp_dump_expect(struct sk_buff
 
 	if (ctnetlink_exp_dump_tuple(skb, &exp->tuple, CTA_EXPECT_TUPLE) < 0)
 		goto nfattr_failure;
-	if (ctnetlink_exp_dump_tuple(skb, &exp->mask, CTA_EXPECT_MASK) < 0)
+	if (ctnetlink_exp_dump_mask(skb, &exp->tuple, &exp->mask) < 0)
 		goto nfattr_failure;
 	if (ctnetlink_exp_dump_tuple(skb,
 				 &master->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
Index: net-2.6.git/include/linux/netfilter/nfnetlink_conntrack.h
===================================================================
--- net-2.6.git.orig/include/linux/netfilter/nfnetlink_conntrack.h	2006-02-01 20:01:44.000000000 +0100
+++ net-2.6.git/include/linux/netfilter/nfnetlink_conntrack.h	2006-02-01 20:52:05.000000000 +0100
@@ -52,6 +52,7 @@ enum ctattr_ip {
 	CTA_IP_V4_DST,
 	CTA_IP_V6_SRC,
 	CTA_IP_V6_DST,
+	CTA_IP_L3NUM,
 	__CTA_IP_MAX
 };
 #define CTA_IP_MAX (__CTA_IP_MAX - 1)
Index: net-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
--- net-2.6.git.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-01-21 21:48:13.000000000 +0100
+++ net-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-02-04 13:02:31.000000000 +0100
@@ -4,7 +4,7 @@
  * (C) 2001 by Jay Schulist <jschlst@samba.org>
  * (C) 2002-2005 by Harald Welte <laforge@gnumonks.org>
  * (C) 2003 by Patrick Mchardy <kaber@trash.net>
- * (C) 2005 by Pablo Neira Ayuso <pablo@eurodev.net>
+ * (C) 2005-2006 by Pablo Neira Ayuso <pablo@eurodev.net>
  *
  * I've reworked this stuff to use attributes instead of conntrack 
  * structures. 5.44 am. I need more tea. --pablo 05/07/11.
@@ -53,20 +53,18 @@ static char __initdata version[] = "0.90
 
 static inline int
 ctnetlink_dump_tuples_proto(struct sk_buff *skb, 
-			    const struct ip_conntrack_tuple *tuple)
+			    const struct ip_conntrack_tuple *tuple,
+			    struct ip_conntrack_protocol *proto)
 {
-	struct ip_conntrack_protocol *proto;
 	int ret = 0;
+	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
 
 	NFA_PUT(skb, CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
 
-	/* If no protocol helper is found, this function will return the
-	 * generic protocol helper, so proto won't *ever* be NULL */
-	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
 	if (likely(proto->tuple_to_nfattr))
 		ret = proto->tuple_to_nfattr(skb, tuple);
 	
-	ip_conntrack_proto_put(proto);
+	NFA_NEST_END(skb, nest_parms);
 
 	return ret;
 
@@ -75,28 +73,41 @@ nfattr_failure:
 }
 
 static inline int
-ctnetlink_dump_tuples(struct sk_buff *skb, 
-		      const struct ip_conntrack_tuple *tuple)
+ctnetlink_dump_tuples_ip(struct sk_buff *skb, 
+			 const struct ip_conntrack_tuple *tuple)
 {
-	struct nfattr *nest_parms;
-	int ret;
+	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
 	
-	nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
 	NFA_PUT(skb, CTA_IP_V4_SRC, sizeof(u_int32_t), &tuple->src.ip);
 	NFA_PUT(skb, CTA_IP_V4_DST, sizeof(u_int32_t), &tuple->dst.ip);
-	NFA_NEST_END(skb, nest_parms);
 
-	nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
-	ret = ctnetlink_dump_tuples_proto(skb, tuple);
 	NFA_NEST_END(skb, nest_parms);
 
-	return ret;
+	return 0;
 
 nfattr_failure:
 	return -1;
 }
 
 static inline int
+ctnetlink_dump_tuples(struct sk_buff *skb,
+		      const struct ip_conntrack_tuple *tuple)
+{
+	int ret;
+	struct ip_conntrack_protocol *proto;
+
+	ret = ctnetlink_dump_tuples_ip(skb, tuple);
+	if (unlikely(ret < 0))
+		return ret;
+
+	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
+	ret = ctnetlink_dump_tuples_proto(skb, tuple, proto);
+	ip_conntrack_proto_put(proto);
+
+	return ret;
+}
+
+static inline int
 ctnetlink_dump_status(struct sk_buff *skb, const struct ip_conntrack *ct)
 {
 	u_int32_t status = htonl((u_int32_t) ct->status);
@@ -1134,6 +1145,25 @@ nfattr_failure:
 }			
 
 static inline int
+ctnetlink_exp_dump_mask(struct sk_buff *skb,
+			const struct ip_conntrack_tuple *tuple,
+			const struct ip_conntrack_tuple *mask)
+{
+	int ret;
+	struct ip_conntrack_protocol *proto;
+
+	ret = ctnetlink_dump_tuples_ip(skb, mask);
+	if (unlikely(ret < 0))
+		return ret;
+
+	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
+	ret = ctnetlink_dump_tuples_proto(skb, mask, proto);
+	ip_conntrack_proto_put(proto);
+
+	return ret;
+}
+
+static inline int
 ctnetlink_exp_dump_expect(struct sk_buff *skb,
                           const struct ip_conntrack_expect *exp)
 {
@@ -1143,7 +1173,7 @@ ctnetlink_exp_dump_expect(struct sk_buff
 
 	if (ctnetlink_exp_dump_tuple(skb, &exp->tuple, CTA_EXPECT_TUPLE) < 0)
 		goto nfattr_failure;
-	if (ctnetlink_exp_dump_tuple(skb, &exp->mask, CTA_EXPECT_MASK) < 0)
+	if (ctnetlink_exp_dump_mask(skb, &exp->tuple, &exp->mask) < 0)
 		goto nfattr_failure;
 	if (ctnetlink_exp_dump_tuple(skb,
 				 &master->tuplehash[IP_CT_DIR_ORIGINAL].tuple,

Re: [PATCH] Fix expectation mask dumping, take #2

[Yasuyuki KOZAKAI]

Hi, Pablo,

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 06 Feb 2006 00:54:54 +0100

> Hi,
> 
> @Patrick: this patch fixes the problem that `[NETFILTER 03/14]:
> nf_conntrack: check address family when finding protocol module` works
> around. So I think that such patch needs to be reverted :(

Please don't revert that. I've been thinking that both your and
his patch are necessary. Even if your patch fixes oops triggered by
nfctnetlink, other module might have same problem future.

I think he aim of 2 patches is different. His patch fixes the root
of oops. Your patch fixes dumping expectation mask.

BTW, ah, his patch includes unlikely(). But fortunately,
your patch looks that it makes unnecessary to remove it and
I expect that it will be applied to net-2.6 tree. Great!

-- Yasuyuki Kozakai

Re: [PATCH] Fix expectation mask dumping, take #2

[Yasuyuki KOZAKAI]

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


Hi, Pablo,

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 06 Feb 2006 00:54:54 +0100

> Description:
> 
> This patch introduces the function ctnetlink_exp_dump_mask, that
> correctly dumps the expectation mask. Such function uses the l3num value
> from the expectation tuple that is a valid layer 3 protocol number.
> 
> Besides, this modification introduces the attribute CTA_IP_L3NUM.
> Although the layer 3 protocol information is sent in the nfnetlink
> header, if the message contains information about an expectation, it
> will contain information about the master conntrack (just one of the
> tuples), the expectation tuple and the expectation mask. In this case,
> the value of l3num in the expectation mask is not the same that is set
> in the nfnetlink message. That is why we need another field that contain
> the value of l3num.
> 
> Now libnetfilter_conntrack can use the CTA_IP_L3NUM attribute, but if
> this attribute is not present in the message, it can use the information
> available in the nfnetlink header message.

Thanks for changing size of CTA_IP_L3NUM field. But it seems to have endian
problem. This patch can save your time ?

Other parts are fine to me.

Regards,

-- Yasuyuki Kozakai

[-- Attachment #2: nfctnetlink-8bits-l3num.patch --]
[-- Type: Text/Plain, Size: 602 bytes --]

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index ee814e1..150174f 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -81,8 +81,9 @@ ctnetlink_dump_tuples_ip(struct sk_buff 
 {
 	int ret = 0;
 	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
+	u_int8_t l3num = (u_int8_t)tuple->src.l3num;
 
-	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int8_t), &tuple->src.l3num);
+	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int8_t), &l3num);
 
 	if (likely(l3proto->tuple_to_nfattr))
 		ret = l3proto->tuple_to_nfattr(skb, tuple);

Re: [PATCH] Fix expectation mask dumping, take #2

[Pablo Neira Ayuso]

Yasuyuki KOZAKAI wrote:
> Thanks for changing size of CTA_IP_L3NUM field. But it seems to have endian
> problem. This patch can save your time ?
> 
> ------------------------------------------------------------------------
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index ee814e1..150174f 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -81,8 +81,9 @@ ctnetlink_dump_tuples_ip(struct sk_buff 
>  {
>  	int ret = 0;
>  	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
> +	u_int8_t l3num = (u_int8_t)tuple->src.l3num;
>  
> -	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int8_t), &tuple->src.l3num);
> +	NFA_PUT(skb, CTA_IP_L3NUM, sizeof(u_int8_t), &l3num);
>  
>  	if (likely(l3proto->tuple_to_nfattr))
>  		ret = l3proto->tuple_to_nfattr(skb, tuple);

Oops, I understood in your previous email that you were going to send a
patch that changes the type of l3num from u_int16_t to u_int8_t. In that
case I'll resend a patch with this change. Thanks

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: expectation mask handling in nfctnetlink

[Yasuyuki KOZAKAI]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed, 01 Feb 2006 03:09:21 +0100

> > I've re-visited this. For now, I'm not sure this is the best solution
> > because I don't see how format and contents of expectation mask
> > is required by libnetfilter_conntrack. It doesn't use dumped expectation
> > mask at all.
> 
> Maybe userspace helpers could need that information some day. I don't
> want to add limitations to libnetfilter_conntrack just because we have
> some assumptions.
> 
> > Then the other idea can be considerd, too. For example, dumping union of
> > l3part(mask->u3.all) and union of proto part(mask->u.all) without assumption
> > of specific address family/protocol. Do you have any idea on this ?
> 
> We shouldn't send the whole union via netlink. If the union is modified
> further, it will break backward compatibility for userspace applications
> since the size could change.

I don't think so because we can use 'len' field for that.
But anyway,

>
> So, the possibilities I see up to now are:
> 
> a) just remove the unlikely() statement.
> Drawback: Not so efficient, since that checking isn't always required.
> 
> b) back to my idea: set l3num to the maximum known value (0x1F), and add
> a bug trap in nf_ct_expect_related to check that nobody uses a value >=
> AF_MAX.
> Drawback: People surely will hit bug while writing their own helpers.

I'm fine to do a) for current nfctnetlink. Let's re-visit this
when it becomes clear about format to dump expectation mask.

-- Yasuyuki Kozakai

Re: expectation mask handling in nfctnetlink

[Yasuyuki KOZAKAI]

Hi,

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 01 Feb 2006 12:04:02 +0100

> Pablo Neira Ayuso wrote:
> > a) just remove the unlikely() statement.
> > Drawback: Not so efficient, since that checking isn't always required.
> 
> I'm not sure I understand the problem. Is it about the unlikely in
> __nf_ct_proto_find?

Yes.

>                     That one is most likely not making any difference
> at all, since there is no code to reorder.

Pablo pointed out ...

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 13 Jan 2006 12:17:16 +0100

> Yasuyuki KOZAKAI wrote:
> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index 62bb509..9a147dd 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -188,7 +188,7 @@ extern struct nf_conntrack_protocol nf_c
> >  struct nf_conntrack_protocol *
> >  __nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol)
> >  {
> > -	if (unlikely(nf_ct_protos[l3proto] == NULL))
> > +	if (unlikely(l3proto > AF_MAX || nf_ct_protos[l3proto] == NULL))
> 
> Could the unlikely statement be bad at expectation dumping?

I thought unlikely() was ok but current ctnetlink_exp_dump_expect() was
strange because it trys to find proto with 0xffff as address family and 0xff
as protocol number.

But I understood from Pablo's mail, that we have not decided what contents
should be included in netlink message for expectation mask yet, then I agreed
to remove unlikely() for now.

-- Yasuyuki Kozakai