[PATCH 1/4] Fix expectaction mask dumping, take #3

[PATCH 1/4] Fix expectaction mask dumping, take #3

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1331 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>

-- 
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: 9952 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-09 17:34:01.000000000 +0100
+++ net-2.6.git/net/netfilter/nf_conntrack_netlink.c	2006-02-12 20:49:04.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,47 @@ 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);
+	u_int8_t l3num = (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);
+
 	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 +1162,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 +1194,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-04 14:33:53.000000000 +0100
+++ net-2.6.git/include/linux/netfilter/nfnetlink_conntrack.h	2006-02-12 20:48: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-02-09 17:34:00.000000000 +0100
+++ net-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-02-12 20:48:05.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 1/4] Fix expectaction mask dumping, take #3

[Harald Welte]

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

On Mon, Feb 13, 2006 at 03:39:50AM +0100, Pablo Neira Ayuso wrote:
> [CTNETLINK] Fix expectaction mask dumping

Thanks, Pablo.  This patch looks fine to me.  One cosmetic comment
though:

> Besides, this modification introduces the attribute CTA_IP_L3NUM.

Please call it CTA_L3NUM or something like that.  Nobody says that we
will always and only deal with protocols that are called IP[vX].

If nobody else has any other comments, I suggest you resubmit with this
requested change, and I'll apply and push to davem for net-2.6.17

Thanks!
-- 
- 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 --]

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> [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.

This part looks fine to me, apart from one minor nitpick :)

> 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.

I'm not sure I understand. The new attribute still contains the same
value as the netlink header, doesn't it? So userspace should currently
have at least two possibilities to get the correct value:

- use the value from the netlink header
- use the value from the tuple that comes with the mask, as the first
  part of your patch does. This seems most logically to me since the
  mask and the tuple belong together.

> @@ -77,33 +75,47 @@ 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);
> +	u_int8_t l3num = (u_int8_t)tuple->src.l3num;

The cast is unnecessary.

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Harald Welte]

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

On Thu, Feb 16, 2006 at 10:36:20AM +0100, Patrick McHardy wrote:

> I'm not sure I understand. The new attribute still contains the same
> value as the netlink header, doesn't it? So userspace should currently
> have at least two possibilities to get the correct value:
> 
> - use the value from the netlink header
> - use the value from the tuple that comes with the mask, as the first
>   part of your patch does. This seems most logically to me since the
>   mask and the tuple belong together.

I still dream about a connection with one end in ipv4 space and the
other in ipv6.  If we ever want to cover such a model, then the tuple
needs to include l3num.
-- 
- 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 --]

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Patrick McHardy]

Harald Welte wrote:
> On Thu, Feb 16, 2006 at 10:36:20AM +0100, Patrick McHardy wrote:
> 
> 
>>I'm not sure I understand. The new attribute still contains the same
>>value as the netlink header, doesn't it? So userspace should currently
>>have at least two possibilities to get the correct value:
>>
>>- use the value from the netlink header
>>- use the value from the tuple that comes with the mask, as the first
>>  part of your patch does. This seems most logically to me since the
>>  mask and the tuple belong together.
> 
> 
> I still dream about a connection with one end in ipv4 space and the
> other in ipv6.  If we ever want to cover such a model, then the tuple
> needs to include l3num.

Yes, but until then it looks totally redundant. Since the bandwidth
of netlink is limited, I think we shouldn't add new attributes without
really needing them.

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 16 Feb 2006 21:11:01 +0100

> Harald Welte wrote:
> > On Thu, Feb 16, 2006 at 10:36:20AM +0100, Patrick McHardy wrote:
> > 
> > 
> >>I'm not sure I understand. The new attribute still contains the same
> >>value as the netlink header, doesn't it? So userspace should currently
> >>have at least two possibilities to get the correct value:
> >>
> >>- use the value from the netlink header
> >>- use the value from the tuple that comes with the mask, as the first
> >>  part of your patch does. This seems most logically to me since the
> >>  mask and the tuple belong together.
> > 
> > 
> > I still dream about a connection with one end in ipv4 space and the
> > other in ipv6.  If we ever want to cover such a model, then the tuple
> > needs to include l3num.
>
> Yes, but until then it looks totally redundant. Since the bandwidth
> of netlink is limited, I think we shouldn't add new attributes without
> really needing them.

'l3num' field in expectation mask may be 0xff. Then the new field is
necessary so that kernel can pass the "exact value" in it to userspace.

But, I don't know whether userspace really wants to know the exact value
in it or not. I assumed, yes, but if it is not ture, I'll agree to Patrick.

-- Yasuyuki Kozakai

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Pablo Neira Ayuso]

Hi Patrick,

Patrick McHardy wrote:
>>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.
> 
> I'm not sure I understand. The new attribute still contains the same
> value as the netlink header, doesn't it? So userspace should currently
> have at least two possibilities to get the correct value:
> 
> - use the value from the netlink header
> - use the value from the tuple that comes with the mask, as the first
>   part of your patch does. This seems most logically to me since the
>   mask and the tuple belong together.

The problem is that currently the expectation mask is not dumped.
find_l3proto returns the generic protocol handler for 0xFF, and that
doesn't dump any layer 3 information. Moreover, the expectation mask has
l3num value that is different from the l3num in the nfnetlink header,
that's why I introduced this field.

I can send a patch to remove the expectation mask dumping but I'm not
sure if this information could be useful for userspace helpers. Harald?

-- 
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 1/4] Fix expectaction mask dumping, take #3

[Pablo Neira Ayuso]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>>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.
>>
>>I'm not sure I understand. The new attribute still contains the same
>>value as the netlink header, doesn't it? So userspace should currently
>>have at least two possibilities to get the correct value:
>>
>>- use the value from the netlink header
>>- use the value from the tuple that comes with the mask, as the first
>>  part of your patch does. This seems most logically to me since the
>>  mask and the tuple belong together.
> 
> The problem is that currently the expectation mask is not dumped.
> find_l3proto returns the generic protocol handler for 0xFF, and that
> doesn't dump any layer 3 information. Moreover, the expectation mask has
> l3num value that is different from the l3num in the nfnetlink header,
> that's why I introduced this field.
> 
> I can send a patch to remove the expectation mask dumping but I'm not
> sure if this information could be useful for userspace helpers. Harald?

I think I can answer myself after some thinking: in order to create an
expectation from userspace we will need to set the value of l3num of the
expectation mask. Such value will be different from the value in the
nfnetlink header, so I still think that we need that new CTA_L3NUM
attribute.

-- 
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 1/4] Fix expectaction mask dumping, take #3

[Yasuyuki KOZAKAI]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed, 22 Feb 2006 04:20:39 +0100

> > The problem is that currently the expectation mask is not dumped.
> > find_l3proto returns the generic protocol handler for 0xFF, and that
> > doesn't dump any layer 3 information.

Yes, I want this bug to go away before 2.6.16 is shipped.

> >                                        Moreover, the expectation mask has
> > l3num value that is different from the l3num in the nfnetlink header,
> > that's why I introduced this field.

Yes, but in the current code l3num in expectation mask is always 0xff.
And even if we port all helpers of ip_conntrack to nf_conntrack,
they will set 0xff to l3num.

Which situation kernel wants to set the value except of 0xff to l3num
in expectation mask ?

> > I can send a patch to remove the expectation mask dumping but I'm not
> > sure if this information could be useful for userspace helpers. Harald?
>
> I think I can answer myself after some thinking: in order to create an
> expectation from userspace we will need to set the value of l3num of the
> expectation mask. Such value will be different from the value in the
> nfnetlink header, so I still think that we need that new CTA_L3NUM
> attribute.

The similar question can arise. Which situation userspace wants to set
the value except of 0xff in l3num in expectation mask ?

-- Yasuyuki Kozakai

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Thu, 16 Feb 2006 21:11:01 +0100
> 
>>Yes, but until then it looks totally redundant. Since the bandwidth
>>of netlink is limited, I think we shouldn't add new attributes without
>>really needing them.
> 
> 
> 'l3num' field in expectation mask may be 0xff. Then the new field is
> necessary so that kernel can pass the "exact value" in it to userspace.
> 
> But, I don't know whether userspace really wants to know the exact value
> in it or not. I assumed, yes, but if it is not ture, I'll agree to Patrick.

My point was that a mask is pretty meaningless without the thing it
masks, which is the tuple (except maybe a mask of all-zeros). The tuple
itself already contains the correct protocol number.

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Wed, 22 Feb 2006 04:20:39 +0100
> 
> 
>>>                                       Moreover, the expectation mask has
>>>l3num value that is different from the l3num in the nfnetlink header,
>>>that's why I introduced this field.
> 
> 
> Yes, but in the current code l3num in expectation mask is always 0xff.
> And even if we port all helpers of ip_conntrack to nf_conntrack,
> they will set 0xff to l3num.
> 
> Which situation kernel wants to set the value except of 0xff to l3num
> in expectation mask ?

Harald and me were actually talking about simplifying the helper lookup
by making the dst part of an expectation fixed (for ip_conntrack). With
nf_conntrack l3num is in the src-part of the tuple, but thats just
for memory-efficiency, it can be treated equally to protonum and the
mask can be eliminated.

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 23 Feb 2006 10:43:32 +0100

> Yasuyuki KOZAKAI wrote:
> > From: Patrick McHardy <kaber@trash.net>
> > Date: Thu, 16 Feb 2006 21:11:01 +0100
> > 
> >>Yes, but until then it looks totally redundant. Since the bandwidth
> >>of netlink is limited, I think we shouldn't add new attributes without
> >>really needing them.
> > 
> > 
> > 'l3num' field in expectation mask may be 0xff. Then the new field is
> > necessary so that kernel can pass the "exact value" in it to userspace.
> > 
> > But, I don't know whether userspace really wants to know the exact value
> > in it or not. I assumed, yes, but if it is not ture, I'll agree to Patrick.
> 
> My point was that a mask is pretty meaningless without the thing it
> masks, which is the tuple (except maybe a mask of all-zeros). The tuple
> itself already contains the correct protocol number.

What I want to say in the just previous mail I sent is same. :)
I just wanted Pablo to confirm that.

-- Yasuyuki Kozakai

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 23 Feb 2006 10:48:57 +0100

> Harald and me were actually talking about simplifying the helper lookup
> by making the dst part of an expectation fixed (for ip_conntrack). With

I got time to read deeply the patch from him yesterday midnight.
(but that was for nf_conntrack). Simplifying the helper lookup is fine to
me.

> nf_conntrack l3num is in the src-part of the tuple, but thats just
> for memory-efficiency, it can be treated equally to protonum and the
> mask can be eliminated.

Yes, that patch actually eliminates it.

-- Yasuyuki Kozakai

Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Thu, 23 Feb 2006 10:48:57 +0100
> 
> 
>>Harald and me were actually talking about simplifying the helper lookup
>>by making the dst part of an expectation fixed (for ip_conntrack). With
> 
> 
> I got time to read deeply the patch from him yesterday midnight.
> (but that was for nf_conntrack). Simplifying the helper lookup is fine to
> me.

Oops, I actually meant the expectation lookup. But the idea is pretty
similar I think.