Mailing List Archive: iptables: Devel

[PATCH 1/3] libnl: add netfilter support

Index | Next | Previous | View Threaded

philipc at snapgear

Sep 2, 2007, 10:11 PM

Post #1 of 13 (1625 views)
Permalink

[PATCH 1/3] libnl: add netfilter support

Attachments:

nfnl.patch (13.7 KB)

kaber at trash

Sep 3, 2007, 2:50 AM

Post #2 of 13 (1571 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

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.

tgraf at suug

Sep 3, 2007, 3:01 AM

Post #3 of 13 (1575 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

* Patrick McHardy <kaber [at] trash> 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.

kaber at trash

Sep 3, 2007, 3:06 AM

Post #4 of 13 (1570 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

Thomas Graf wrote:
> * Patrick McHardy <kaber [at] trash> 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?

tgraf at suug

Sep 3, 2007, 3:09 AM

Post #5 of 13 (1576 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

* Philip Craig <philipc [at] snapgear> 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.

tgraf at suug

Sep 3, 2007, 3:15 AM

Post #6 of 13 (1574 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

* Patrick McHardy <kaber [at] trash> 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.

kaber at trash

Sep 3, 2007, 3:53 AM

Post #7 of 13 (1570 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

Thomas Graf wrote:
> * Patrick McHardy <kaber [at] trash> 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 :)

tgraf at suug

Sep 3, 2007, 4:03 AM

Post #8 of 13 (1569 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

* Patrick McHardy <kaber [at] trash> 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.

kaber at trash

Sep 3, 2007, 4:13 AM

Post #9 of 13 (1573 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

Thomas Graf wrote:
> * Patrick McHardy <kaber [at] trash> 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.

philipc at snapgear

Sep 3, 2007, 6:18 PM

Post #10 of 13 (1561 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

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.

philipc at snapgear

Sep 3, 2007, 7:12 PM

Post #11 of 13 (1561 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

Thomas Graf wrote:
> * Philip Craig <philipc [at] snapgear> 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?

tgraf at suug

Sep 4, 2007, 2:39 AM

Post #12 of 13 (1552 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

* Philip Craig <philipc [at] snapgear> 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.

tgraf at suug

Sep 4, 2007, 4:36 AM

Post #13 of 13 (1554 views)
Permalink

Re: [PATCH 1/3] libnl: add netfilter support [In reply to]

* Philip Craig <philipc [at] snapgear> 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!

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads