NFNL_NFA_NEST

NFNL_NFA_NEST

[Patrick McHardy]

One of the worst mistakes in nfnetlink in my opinion was the
introduction of the NFNL_NFA_NEST bit. It prevents us from
using a large part of the generic netlink stuff, since that
just interprets it as a really huge attribute type. Since
its not used even for anything, this is really annoying.

Unfortunately there is no easy way to get rid of it, current
userspace code sends it to the kernel, so even if we stop
including it in the kernel, we need to deal with it for
compatibilty.

What I want to propose is to stop sending it on both kernel
and userspace side immediately, but continue to accept it
for another year or two. After this time, we switch to use
the generic netlink stuff, which would break using old
libnfnetlink users. Since we just introduced a new API and
are fading out the old one anyway, this doesn't seem too
bad.

Any comments?

Re: NFNL_NFA_NEST

[Jozsef Kadlecsik]

Hi Patrick,

On Wed, 21 Mar 2007, Patrick McHardy wrote:

> One of the worst mistakes in nfnetlink in my opinion was the
> introduction of the NFNL_NFA_NEST bit. It prevents us from
> using a large part of the generic netlink stuff, since that
> just interprets it as a really huge attribute type. Since
> its not used even for anything, this is really annoying.

Pablo helped me to work on porting ipset from sockopt to nfnetlink (which 
is still not finished yet :-() and I nagged Pablo a lot to use nesting, 
primarily to hide sub-module details at netlink message level from the 
ipset core. For example when adding/deleting/testing a set, the netlink 
message looks like this:

<set name>
<set type>
<nested: set type specific data>

so that the core is not burdened by module-dependent details.

The other place where I wanted to use nesting is to send a bunch of the 
same type data in one netlink message instead of sending every one of them 
in separated messages: I shudder to send ~370 netlink messages instead of 
a single one in order to pass that number of IP addresses.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
           H-1525 Budapest 114, POB. 49, Hungary

Re: NFNL_NFA_NEST

[Patrick McHardy]

Jozsef Kadlecsik wrote:
> On Wed, 21 Mar 2007, Patrick McHardy wrote:
> 
>> One of the worst mistakes in nfnetlink in my opinion was the
>> introduction of the NFNL_NFA_NEST bit. It prevents us from
>> using a large part of the generic netlink stuff, since that
>> just interprets it as a really huge attribute type. Since
>> its not used even for anything, this is really annoying.
> 
> 
> Pablo helped me to work on porting ipset from sockopt to nfnetlink
> (which is still not finished yet :-() and I nagged Pablo a lot to use
> nesting, primarily to hide sub-module details at netlink message level
> from the ipset core. For example when adding/deleting/testing a set, the
> netlink message looks like this:
> 
> <set name>
> <set type>
> <nested: set type specific data>
> 
> so that the core is not burdened by module-dependent details.
> 
> The other place where I wanted to use nesting is to send a bunch of the
> same type data in one netlink message instead of sending every one of
> them in separated messages: I shudder to send ~370 netlink messages
> instead of a single one in order to pass that number of IP addresses.


I don't want to remove the ability to nest attributes, just the
NFNL_NFA_NEST bit on nested attributes (ORed in nfa_type):

#define NFA_NEST(skb, type) \


({      struct nfattr *__start = (struct nfattr *) (skb)->tail; \
        NFA_PUT(skb, (NFNL_NFA_NEST | type), 0, NULL); \
        __start;  })

Or did I misunderstand you and you actually use this for something?

Re: NFNL_NFA_NEST

[Jozsef Kadlecsik]

On Wed, 21 Mar 2007, Patrick McHardy wrote:

> I don't want to remove the ability to nest attributes, just the
> NFNL_NFA_NEST bit on nested attributes (ORed in nfa_type):
>
> #define NFA_NEST(skb, type) \
>
>
> ({      struct nfattr *__start = (struct nfattr *) (skb)->tail; \
>        NFA_PUT(skb, (NFNL_NFA_NEST | type), 0, NULL); \
>        __start;  })
>
> Or did I misunderstand you and you actually use this for something?

No, the other way around, I misunderstood you :-). I don't use/plan to use 
the NFNL_NFA_NEST bit for anything.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
           H-1525 Budapest 114, POB. 49, Hungary

Re: NFNL_NFA_NEST

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> One of the worst mistakes in nfnetlink in my opinion was the
> introduction of the NFNL_NFA_NEST bit. It prevents us from
> using a large part of the generic netlink stuff, since that
> just interprets it as a really huge attribute type. Since
> its not used even for anything, this is really annoying.

I'm using this bit to convert attribute headers from host byte order to
network byte order in conntrackd. I'm unsure about how I would do the
conversion if we remove such bit.

> Unfortunately there is no easy way to get rid of it, current
> userspace code sends it to the kernel, so even if we stop
> including it in the kernel, we need to deal with it for
> compatibilty.

We have a field in the nfnetlink that contains the protocol version that
we can use for this kind of changes without breaking backward.

-- 
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: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>One of the worst mistakes in nfnetlink in my opinion was the
>>introduction of the NFNL_NFA_NEST bit. It prevents us from
>>using a large part of the generic netlink stuff, since that
>>just interprets it as a really huge attribute type. Since
>>its not used even for anything, this is really annoying.
> 
> 
> I'm using this bit to convert attribute headers from host byte order to
> network byte order in conntrackd. I'm unsure about how I would do the
> conversion if we remove such bit.


That was the original idea behind it. But where do we use host byte
order?

>>Unfortunately there is no easy way to get rid of it, current
>>userspace code sends it to the kernel, so even if we stop
>>including it in the kernel, we need to deal with it for
>>compatibilty.
> 
> 
> We have a field in the nfnetlink that contains the protocol version that
> we can use for this kind of changes without breaking backward.


If we still support the old stuff we can't get rid of it.
I would really like to remove all the duplicated code and
use the much nicer netlink infrastructure we have today.

Re: NFNL_NFA_NEST

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>
>>> One of the worst mistakes in nfnetlink in my opinion was the
>>> introduction of the NFNL_NFA_NEST bit. It prevents us from
>>> using a large part of the generic netlink stuff, since that
>>> just interprets it as a really huge attribute type. Since
>>> its not used even for anything, this is really annoying.
>>
>> I'm using this bit to convert attribute headers from host byte order to
>> network byte order in conntrackd. I'm unsure about how I would do the
>> conversion if we remove such bit.
> 
> That was the original idea behind it. But where do we use host byte
> order?

Currently, some parts of the nfnetlink message are in host byte order
and some in network byte order. For example, the netlink header and the
attribute header (TL) are un host byte order, but the attribute values
(V) are in network byte order.

Having a look at the code, if we decide to remove the nested bit,
conntrackd will have to parse the message coming from the kernel, put in
a nf_conntrack object and generate a new message un network byte order.
Moreover, I'll have to extend libnfnetlink and libnetfilter_conntrack to
indicate the byte order of a certain nfnetlink message, since without
the nested bit, I can't do proxying anymore.

>>> Unfortunately there is no easy way to get rid of it, current
>>> userspace code sends it to the kernel, so even if we stop
>>> including it in the kernel, we need to deal with it for
>>> compatibilty.
>>
>> We have a field in the nfnetlink that contains the protocol version that
>> we can use for this kind of changes without breaking backward.
> 
> If we still support the old stuff we can't get rid of it.
> I would really like to remove all the duplicated code and
> use the much nicer netlink infrastructure we have today.

I'm fine with using the new netlink infrastructure. We could remove the
nested bit, release new libraries that don't send it to kernel anymore
and keep the old nfnetlink code that understand the nest bit thing for
quite some time. Thus, doing the port to the new infrastructure later.

Thoughts?

-- 
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: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
>>>I'm using this bit to convert attribute headers from host byte order to
>>>network byte order in conntrackd. I'm unsure about how I would do the
>>>conversion if we remove such bit.
>>
>>That was the original idea behind it. But where do we use host byte
>>order?
> 
> 
> Currently, some parts of the nfnetlink message are in host byte order
> and some in network byte order. For example, the netlink header and the
> attribute header (TL) are un host byte order, but the attribute values
> (V) are in network byte order.


Right.

> Having a look at the code, if we decide to remove the nested bit,
> conntrackd will have to parse the message coming from the kernel, put in
> a nf_conntrack object and generate a new message un network byte order.
> Moreover, I'll have to extend libnfnetlink and libnetfilter_conntrack to
> indicate the byte order of a certain nfnetlink message, since without
> the nested bit, I can't do proxying anymore.


Not really. The NEST bit allows to walk nested structures without
being aware of the structure. So all you need to do is make it aware
of which attributes are nested - something you need anyway for
parsing I suppose.

The byteorder is already clear, all netlink header values are in
host byte order, all attributes in network byte order.

> I'm fine with using the new netlink infrastructure. We could remove the
> nested bit, release new libraries that don't send it to kernel anymore
> and keep the old nfnetlink code that understand the nest bit thing for
> quite some time. Thus, doing the port to the new infrastructure later.


Yes, we should probably wait for at least one year.

Re: NFNL_NFA_NEST

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Not really. The NEST bit allows to walk nested structures without
> being aware of the structure. So all you need to do is make it aware
> of which attributes are nested - something you need anyway for
> parsing I suppose.

OK, then I have two choices here, define a proxy function that is
structure aware to convert TL to network byte order that will be almost
a copy and paste of the parsing part, or alternatively integrate the
byte order conversion as an option for the build/parse functions, as for
now the latter seems more natural to me.

> The byteorder is already clear, all netlink header values are in
> host byte order, all attributes in network byte order.
> 
>> I'm fine with using the new netlink infrastructure. We could remove the
>> nested bit, release new libraries that don't send it to kernel anymore
>> and keep the old nfnetlink code that understand the nest bit thing for
>> quite some time. Thus, doing the port to the new infrastructure later.
> 
> Yes, we should probably wait for at least one year.

Fine with it. I'm about to release a new version of libnfnetlink, should
we stop sending the nested bit thing to kernel since now?

-- 
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: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>Not really. The NEST bit allows to walk nested structures without
>>being aware of the structure. So all you need to do is make it aware
>>of which attributes are nested - something you need anyway for
>>parsing I suppose.
> 
> 
> OK, then I have two choices here, define a proxy function that is
> structure aware to convert TL to network byte order that will be almost
> a copy and paste of the parsing part, or alternatively integrate the
> byte order conversion as an option for the build/parse functions, as for
> now the latter seems more natural to me.


I guess it should be possible to define a couple of structures that
hold the information about which attributes are nested and build a
simple conversion function based on that.

>>Yes, we should probably wait for at least one year.
> 
> 
> Fine with it. I'm about to release a new version of libnfnetlink, should
> we stop sending the nested bit thing to kernel since now?


Yes, the kernel doesn't need them in any case. Which gives me an
idea, we could just stop sending them in userspace and still
include them in the kernel, if that makes life easier for you.

Re: NFNL_NFA_NEST

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>
>>> Not really. The NEST bit allows to walk nested structures without
>>> being aware of the structure. So all you need to do is make it aware
>>> of which attributes are nested - something you need anyway for
>>> parsing I suppose.
>>
>> OK, then I have two choices here, define a proxy function that is
>> structure aware to convert TL to network byte order that will be almost
>> a copy and paste of the parsing part, or alternatively integrate the
>> byte order conversion as an option for the build/parse functions, as for
>> now the latter seems more natural to me.
> 
> 
> I guess it should be possible to define a couple of structures that
> hold the information about which attributes are nested and build a
> simple conversion function based on that.

I implemented a simple conversion function yesterday, so we can get rid 
of it for this library release. Anyhow, I'm still concerned about one 
scenario, since netlink over network messages could be sent between two 
systems A and B with different endianess and different library versions, 
consider that A sent a message that contains one attribute that B 
doesn't know. Thus, the message will not be completely converted by the 
structure aware proxy function in B. Then, the message will probably be 
passed to B's netlink subsystem that will complain about a malformed 
attribute, returning EINVAL. For the conntrackd example, both replicas 
should have the same configuration, so this shouldn't be a problem for 
me, but perhaps other future applications could have problems. Another 
point is that we'll have to cook a structure aware conversion function 
for every netlink messages sent on the wire.

>>> Yes, we should probably wait for at least one year.
>>
>> Fine with it. I'm about to release a new version of libnfnetlink, should
>> we stop sending the nested bit thing to kernel since now?
> 
> Yes, the kernel doesn't need them in any case. Which gives me an
> idea, we could just stop sending them in userspace and still
> include them in the kernel, if that makes life easier for you.

Yes, but we'll have to remove it sooner or later, so I understand this 
as a temporary solution, isn't it?

-- 
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: NFNL_NFA_NEST

[Pablo Neira Ayuso]

Pablo Neira Ayuso wrote:
> I implemented a simple conversion function yesterday, so we can get rid 
> of it for this library release. Anyhow, I'm still concerned about one 
> scenario, since netlink over network messages could be sent between two 
> systems A and B with different endianess and different library versions, 
> consider that A sent a message that contains one attribute that B 
> doesn't know. Thus, the message will not be completely converted by the 
> structure aware proxy function in B. Then, the message will probably be 
> passed to B's netlink subsystem that will complain about a malformed 
> attribute, returning EINVAL. For the conntrackd example, both replicas 
> should have the same configuration, so this shouldn't be a problem for 
> me, but perhaps other future applications could have problems. Another 
> point is that we'll have to cook a structure aware conversion function 
> for every netlink messages sent on the wire.

Perhaps the example above probably looks pretty strange. Still, think of 
old libraries and new kernels with one new nested attribute, the 
conversion function will miss the conversion for it. This bit thing gets 
me stressed me a bit :)

-- 
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: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
> I implemented a simple conversion function yesterday, so we can get rid
> of it for this library release. Anyhow, I'm still concerned about one
> scenario, since netlink over network messages could be sent between two
> systems A and B with different endianess and different library versions,
> consider that A sent a message that contains one attribute that B
> doesn't know. Thus, the message will not be completely converted by the
> structure aware proxy function in B. Then, the message will probably be
> passed to B's netlink subsystem that will complain about a malformed
> attribute, returning EINVAL. For the conntrackd example, both replicas
> should have the same configuration, so this shouldn't be a problem for
> me, but perhaps other future applications could have problems. Another
> point is that we'll have to cook a structure aware conversion function
> for every netlink messages sent on the wire.


We just need one conversion function, but a structure per nested
attribute, no?

>> Yes, the kernel doesn't need them in any case. Which gives me an
>> idea, we could just stop sending them in userspace and still
>> include them in the kernel, if that makes life easier for you.
> 
> 
> Yes, but we'll have to remove it sooner or later, so I understand this
> as a temporary solution, isn't it?


Not necessarily. The problem with the NEST bit is the receive
side of the kernel code, the generic stuff can't deal with it.
On the send-side we can simply manually OR it into the type value.

Re: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Pablo Neira Ayuso wrote:
> 
>> I implemented a simple conversion function yesterday, so we can get
>> rid of it for this library release. Anyhow, I'm still concerned about
>> one scenario, since netlink over network messages could be sent
>> between two systems A and B with different endianess and different
>> library versions, consider that A sent a message that contains one
>> attribute that B doesn't know. Thus, the message will not be
>> completely converted by the structure aware proxy function in B. Then,
>> the message will probably be passed to B's netlink subsystem that will
>> complain about a malformed attribute, returning EINVAL. For the
>> conntrackd example, both replicas should have the same configuration,
>> so this shouldn't be a problem for me, but perhaps other future
>> applications could have problems. Another point is that we'll have to
>> cook a structure aware conversion function for every netlink messages
>> sent on the wire.
> 
> 
> Perhaps the example above probably looks pretty strange. Still, think of
> old libraries and new kernels with one new nested attribute, the
> conversion function will miss the conversion for it. This bit thing gets
> me stressed me a bit :)


Yes, thats why we need to keep it long enough so it won't hurt anyone.
IIRC you had planned to fade out the old nfnetlink API anyway, at
that point I guess it won't matter.

Re: NFNL_NFA_NEST

[Pablo Neira Ayuso]

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

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>
>> I implemented a simple conversion function yesterday, so we can get rid
>> of it for this library release. Anyhow, I'm still concerned about one
>> scenario, since netlink over network messages could be sent between two
>> systems A and B with different endianess and different library versions,
>> consider that A sent a message that contains one attribute that B
>> doesn't know. Thus, the message will not be completely converted by the
>> structure aware proxy function in B. Then, the message will probably be
>> passed to B's netlink subsystem that will complain about a malformed
>> attribute, returning EINVAL. For the conntrackd example, both replicas
>> should have the same configuration, so this shouldn't be a problem for
>> me, but perhaps other future applications could have problems. Another
>> point is that we'll have to cook a structure aware conversion function
>> for every netlink messages sent on the wire.
> 
> We just need one conversion function, but a structure per nested
> attribute, no?

Right, just one conversion function, sorry, the previous statement was
not accurate. What I meant is that we'll have to maintain the nested
attribute structure per subsystem and live with the possible
synchronization problems: new kernels with new nested attributes and old
libraries will not do an appropiate conversion.

Attached a patch with the conversion function and how the structure
would look like for ctnetlink, one candidate conversion function is
defined in conntrackd, I'll move it to the libraries soon.

>>> Yes, the kernel doesn't need them in any case. Which gives me an
>>> idea, we could just stop sending them in userspace and still
>>> include them in the kernel, if that makes life easier for you.
>>
>> Yes, but we'll have to remove it sooner or later, so I understand this
>> as a temporary solution, isn't it?
> 
> 
> Not necessarily. The problem with the NEST bit is the receive
> side of the kernel code, the generic stuff can't deal with it.
> On the send-side we can simply manually OR it into the type value.

I just started thinking that probably the generic infrastructure should
support the nest bit. How crazy is this idea?

-- 
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: 3344 bytes --]

Index: src/proxy.c
===================================================================
--- src/proxy.c	(revisión: 48)
+++ src/proxy.c	(copia de trabajo)
@@ -25,7 +25,42 @@
 #define dprintf
 #endif
 
-int nlh_payload_host2network(struct nfattr *nfa, int len)
+struct nested_attr {
+	struct nested_attr *next;
+};
+
+static struct nested_attr is_nested_tuple_ip[CTA_IP_MAX] = {};
+static struct nested_attr is_nested_tuple_proto[CTA_PROTO_MAX] = {};
+static struct nested_attr is_nested_protoinfo_tcp[CTA_PROTOINFO_TCP_MAX] = {};
+static struct nested_attr is_nested_nat_proto[CTA_PROTONAT_MAX] = {};
+
+static struct nested_attr is_nested_tuple[CTA_TUPLE_MAX] = {
+	[CTA_TUPLE_IP-1]        = { .next = is_nested_tuple_ip },
+	[CTA_TUPLE_PROTO-1]     = { .next = is_nested_tuple_proto },
+};
+static struct nested_attr is_nested_protoinfo[CTA_PROTOINFO_MAX] = {
+	[CTA_PROTOINFO_TCP-1]   = { .next = is_nested_protoinfo_tcp },
+};
+static struct nested_attr is_nested_counters[CTA_COUNTERS_MAX] = {};
+static struct nested_attr is_nested_nat[CTA_NAT_MAX] = {
+	[CTA_NAT_PROTO-1]       = { .next = is_nested_nat_proto },
+};
+static struct nested_attr is_nested_help[CTA_HELP_MAX] = {};
+
+struct nested_attr is_nested[CTA_MAX] = {
+	[CTA_TUPLE_ORIG-1]      = { .next = is_nested_tuple },
+	[CTA_TUPLE_REPLY-1]     = { .next = is_nested_tuple },
+	[CTA_PROTOINFO-1]       = { .next = is_nested_protoinfo },
+	[CTA_HELP-1]            = { .next = is_nested_help },
+	[CTA_NAT_SRC-1]         = { .next = is_nested_nat },
+	[CTA_COUNTERS_ORIG-1]   = { .next = is_nested_counters },
+	[CTA_COUNTERS_REPLY-1]  = { .next = is_nested_counters },
+	[CTA_NAT_DST-1]         = { .next = is_nested_nat },
+};
+
+static int payload_hton(struct nfattr *nfa, 
+			int len, 
+			struct nested_attr *is_nested)
 {
 	struct nfattr *__nfa;
 
@@ -36,12 +71,13 @@
 			nfa->nfa_len, len,
 			nfa->nfa_type & NFNL_NFA_NEST ? "NEST":"");
 
-		if (nfa->nfa_type & NFNL_NFA_NEST) {
+		if (is_nested[NFA_TYPE(nfa)-1].next) {
 			if (NFA_PAYLOAD(nfa) > len)
 				return -1;
 
-			if (nlh_payload_host2network(NFA_DATA(nfa), 
-						     NFA_PAYLOAD(nfa)) == -1)
+			if (payload_hton(NFA_DATA(nfa), 
+					 NFA_PAYLOAD(nfa),
+					 is_nested[NFA_TYPE(nfa)-1].next) == -1)
 				return -1;
 		}
 
@@ -70,10 +106,10 @@
 
 	nfhdr->res_id    = htons(nfhdr->res_id);
 
-	return nlh_payload_host2network(NFM_NFA(NLMSG_DATA(nlh)), len);
+	return payload_hton(NFM_NFA(NLMSG_DATA(nlh)), len, is_nested);
 }
 
-int nlh_payload_network2host(struct nfattr *nfa, int len)
+int payload_ntoh(struct nfattr *nfa, int len, struct nested_attr *is_nested)
 {
 	nfa->nfa_type = ntohs(nfa->nfa_type);
 	nfa->nfa_len  = ntohs(nfa->nfa_len);
@@ -85,12 +121,13 @@
 		        nfa->nfa_len, len, 
 		        nfa->nfa_type & NFNL_NFA_NEST ? "NEST":"");
 
-		if (nfa->nfa_type & NFNL_NFA_NEST) {
+		if (is_nested[NFA_TYPE(nfa)-1].next) {
 			if (NFA_PAYLOAD(nfa) > len)
 				return -1;
 
-			if (nlh_payload_network2host(NFA_DATA(nfa),
-						     NFA_PAYLOAD(nfa)) == -1)
+			if (payload_ntoh(NFA_DATA(nfa),
+					 NFA_PAYLOAD(nfa),
+					 is_nested[NFA_TYPE(nfa)-1].next) == -1)
 				return -1;
 		}
 
@@ -120,5 +157,5 @@
 
 	nfhdr->res_id    = ntohs(nfhdr->res_id);
 
-	return nlh_payload_network2host(NFM_NFA(NLMSG_DATA(nlh)), len);
+	return payload_ntoh(NFM_NFA(NLMSG_DATA(nlh)), len, is_nested);
 }

Re: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>
>>>Yes, but we'll have to remove it sooner or later, so I understand this
>>>as a temporary solution, isn't it?
>>
>>
>>Not necessarily. The problem with the NEST bit is the receive
>>side of the kernel code, the generic stuff can't deal with it.
>>On the send-side we can simply manually OR it into the type value.
> 
> 
> I just started thinking that probably the generic infrastructure should
> support the nest bit. How crazy is this idea?


I don't know. It could accept them on the receive side without
further problems since probably no netlink user will define
more than 2^15 attributes, but could not send it since that
would break compatibility. Since the kernel doesn't need it
at all not sending it from userspace seems like a better
solution to me.

Re: NFNL_NFA_NEST

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> I just started thinking that probably the generic infrastructure should
>> support the nest bit. How crazy is this idea?
> 
> I don't know. It could accept them on the receive side without
> further problems since probably no netlink user will define
> more than 2^15 attributes, but could not send it since that
> would break compatibility. Since the kernel doesn't need it
> at all not sending it from userspace seems like a better
> solution to me.

Hm, then we'll have different message formats depending on the source.
If there is no choice I think that I prefer to keep it homogeneous, use
the conversion function and live with the library issues.

Alternatively, we could add support for the nest bit in the receive side
of genetlink and OR the nest bit in nfnetlink for messages sent to
userspace.

Thomas, do you have any suggestion on where to go?

-- 
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: NFNL_NFA_NEST

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Alternatively, we could add support for the nest bit in the receive side
> of genetlink and OR the nest bit in nfnetlink for messages sent to
> userspace.


Well, fine if Thomas doesn't mind.

> Thomas, do you have any suggestion on where to go?


Some context: removal of all the duplicated nfnetlink code is currently
hindered by the NFNL_NFA_NEST bit, which is the highest bit of nfa_type
and is set on nested attributes to allow walking nested attributes
without knowledge of their structure (for endian-conversion). To use
the generic code we need to either stop sending it from userspace (and
wait quite a while because of old libnfnetlink releases) or make the
generic code ignore it.

Your choice :)