Mailing List Archive

Philip Craig wrote:
> +#define NFA_NEST(skb, type) \
> +({ struct nfattr *__start = (struct nfattr *)skb_tail_pointer(skb); \
> + NFA_PUT(skb, (NFNL_NFA_NEST | type), 0, NULL); \
> + __start; })


The latest libnetfilter_conntrack version doesn't send the NFA_NEST
bit to the kernel anymore since we intend to get rid of it, at
least on the receive side. Please change this to not send it here
as well.

* Patrick McHardy <kaber@trash.net> 2007-09-03 11:50
> Philip Craig wrote:
> > +#define NFA_NEST(skb, type) \
> > +({ struct nfattr *__start = (struct nfattr *)skb_tail_pointer(skb); \
> > + NFA_PUT(skb, (NFNL_NFA_NEST | type), 0, NULL); \
> > + __start; })
>
>
> The latest libnetfilter_conntrack version doesn't send the NFA_NEST
> bit to the kernel anymore since we intend to get rid of it, at
> least on the receive side. Please change this to not send it here
> as well.

Great, so basically the attribute interfaces could be merged on the
sending side.

I think I can live with checking for the bit on the receiving side,
the only problem it could cause is when the attribute type is used
to encode information such as a port number. So if we are willing
to take that risk the receiving path could be merged as well.

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2007-09-03 11:50
>
>>Philip Craig wrote:
>>
>>>+#define NFA_NEST(skb, type) \
>>>+({ struct nfattr *__start = (struct nfattr *)skb_tail_pointer(skb); \
>>>+ NFA_PUT(skb, (NFNL_NFA_NEST | type), 0, NULL); \
>>>+ __start; })
>>
>>
>>The latest libnetfilter_conntrack version doesn't send the NFA_NEST
>>bit to the kernel anymore since we intend to get rid of it, at
>>least on the receive side. Please change this to not send it here
>>as well.
>
>
> Great, so basically the attribute interfaces could be merged on the
> sending side.


Yes, hopefully. We're using big endian for numeric values, but that
shouldn't be a problem I think.

> I think I can live with checking for the bit on the receiving side,
> the only problem it could cause is when the attribute type is used
> to encode information such as a port number. So if we are willing
> to take that risk the receiving path could be merged as well.


That kind of information should be stored as attribute value, no?

* Philip Craig <philipc@snapgear.com> 2007-09-03 15:11
> Index: libnl/lib/attr.c
> ===================================================================
> --- libnl.orig/lib/attr.c 2007-09-03 14:24:29.000000000 +1000
> +++ libnl/lib/attr.c 2007-09-03 14:24:45.000000000 +1000
> @@ -261,7 +261,8 @@ int nla_parse(struct nlattr *tb[], int m
> memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
>
> nla_for_each_attr(nla, head, len, rem) {
> - uint16_t type = nla->nla_type;
> + /* Ignore NFNL_NFA_NEST bit, hope nothing else uses it */
> + uint16_t type = nla->nla_type & 0x7fff;

I wonder if it is useful to make this behaviour conditional so that
the netfilter subsystem could enable this for backwards compatibility
while other subsystems won't be affected.

It will offend a few people but it's still pre 1.0 and we can break
the API. I'd rather get this right now than having to life with
side effects for a long time.

* Patrick McHardy <kaber@trash.net> 2007-09-03 12:06
> Thomas Graf wrote:
> > I think I can live with checking for the bit on the receiving side,
> > the only problem it could cause is when the attribute type is used
> > to encode information such as a port number. So if we are willing
> > to take that risk the receiving path could be merged as well.
>
>
> That kind of information should be stored as attribute value, no?

Yes it should but we've already got code like this:
for (i = 0; i < cnt; i++)
nla_put(skb, i, ....);

It's only a matter of time until someone comes up with
something like this:
nla_put(skb, dstport, sizeof(port_info), &port_info);

Maybe we could put a WARN_ON(bit_set) in nla_put() on kernel side
to make sure such behaviour is not being introduced by accident.

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2007-09-03 12:06
>
>>Thomas Graf wrote:
>>
>>>I think I can live with checking for the bit on the receiving side,
>>>the only problem it could cause is when the attribute type is used
>>>to encode information such as a port number. So if we are willing
>>>to take that risk the receiving path could be merged as well.
>>
>>
>>That kind of information should be stored as attribute value, no?
>
>
> Yes it should but we've already got code like this:
> for (i = 0; i < cnt; i++)
> nla_put(skb, i, ....);


That looks pretty much like a list of attributes, but
using different attribute types. Just out of interest:
do you have a pointer to code doing this?


> It's only a matter of time until someone comes up with
> something like this:
> nla_put(skb, dstport, sizeof(port_info), &port_info);
>
> Maybe we could put a WARN_ON(bit_set) in nla_put() on kernel side
> to make sure such behaviour is not being introduced by accident.


That sounds like a good idea. Anyone doing this is looking
for trouble since array-based attribute parsing would require
at least 2^31 elements :)

* Patrick McHardy <kaber@trash.net> 2007-09-03 12:53
> That looks pretty much like a list of attributes, but
> using different attribute types. Just out of interest:
> do you have a pointer to code doing this?

Yes, see tcf_action_dump().

OTOH, netlabel is making heavy use of attribute lists but
the type is constant to allow for automatic policy validation.
I think this is the proper way of generating lists.

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2007-09-03 12:53
>
>>That looks pretty much like a list of attributes, but
>>using different attribute types. Just out of interest:
>>do you have a pointer to code doing this?
>
>
> Yes, see tcf_action_dump().


Right, I remeber, the entire "action order" thing. Thats a
horrible hack IMO, probably best to convert it to real lists
if possible.

> OTOH, netlabel is making heavy use of attribute lists but
> the type is constant to allow for automatic policy validation.
> I think this is the proper way of generating lists.


Fully agreed, thats also what I did in the 8021q netlink
interface.

Patrick McHardy wrote:
> Philip Craig wrote:
>> +#define NFA_NEST(skb, type) \
>> +({ struct nfattr *__start = (struct nfattr *)skb_tail_pointer(skb); \
>> + NFA_PUT(skb, (NFNL_NFA_NEST | type), 0, NULL); \
>> + __start; })
>
>
> The latest libnetfilter_conntrack version doesn't send the NFA_NEST
> bit to the kernel anymore since we intend to get rid of it, at
> least on the receive side. Please change this to not send it here
> as well.

This is just a copy of the kernel header (maybe an old one though).
I haven't added send support to libnl yet, but when I do I'll be
sure not to use this.

Thomas Graf wrote:
> * Philip Craig <philipc@snapgear.com> 2007-09-03 15:11
>> Index: libnl/lib/attr.c
>> ===================================================================
>> --- libnl.orig/lib/attr.c 2007-09-03 14:24:29.000000000 +1000
>> +++ libnl/lib/attr.c 2007-09-03 14:24:45.000000000 +1000
>> @@ -261,7 +261,8 @@ int nla_parse(struct nlattr *tb[], int m
>> memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
>>
>> nla_for_each_attr(nla, head, len, rem) {
>> - uint16_t type = nla->nla_type;
>> + /* Ignore NFNL_NFA_NEST bit, hope nothing else uses it */
>> + uint16_t type = nla->nla_type & 0x7fff;
>
> I wonder if it is useful to make this behaviour conditional so that
> the netfilter subsystem could enable this for backwards compatibility
> while other subsystems won't be affected.
>
> It will offend a few people but it's still pre 1.0 and we can break
> the API. I'd rather get this right now than having to life with
> side effects for a long time.

Yes it would be nice to only do it for subsystems that need it.
The only place I can see in the existing arguments for nla_parse()
is to overload the meaning of the unused policy[0]. The other options
are to change nla_parse() or add nla_parse_masked(). Any preference?

* Philip Craig <philipc@snapgear.com> 2007-09-04 12:12
> Yes it would be nice to only do it for subsystems that need it.
> The only place I can see in the existing arguments for nla_parse()
> is to overload the meaning of the unused policy[0]. The other options
> are to change nla_parse() or add nla_parse_masked(). Any preference?

Actually after discussing this with Patrick a bit I think we can safely
assume that this bit is never set in any other subsystem so just leave
it as-is.

* Philip Craig <philipc@snapgear.com> 2007-09-03 15:11
>

> ---
> include/linux/netfilter/nfnetlink.h | 106 +++++++++++++++
> include/netlink/netfilter/nfnl.h | 44 ++++++
> include/netlink/netlink.h | 1
> lib/Makefile | 2
> lib/attr.c | 3
> lib/netfilter/nfnl.c | 246 ++++++++++++++++++++++++++++++++++++
> 6 files changed, 401 insertions(+), 1 deletion(-)

I've applied this patch with some minor modification which is the
removal of most of the macros from include/linux/netfilter/nfnetlink.h
for the purpose of making sure people use the generic alternatives.

Thanks a lot Philip!