netfilter supporto for entry data

netfilter supporto for entry data

[Massimiliano Hofer]

Hi,
I wrote some time ago about the possibility of entry specific data for 
matches. There are several matches that could use a pointer to a structure 
allocated and initialized during checkentry() and disposed of during 
destroy().

Is there any reason the API doosn't provide a way to do this?

I'd like to change these structures (and all corresponding references and 
initialization code) as follows:

struct xt_match
{
...
        int (*match)(const struct sk_buff *skb,
                     const struct net_device *in,
                     const struct net_device *out,
                     const void *matchinfo,
                     int offset,
                     unsigned int protoff,
                     int *hotdrop,
                     void *instance_data);

        int (*checkentry)(const char *tablename,
                          const void *ip,
                          void *matchinfo,
                          unsigned int matchinfosize,
                          unsigned int hook_mask,
                          void **instance_data);

        void (*destroy)(void *matchinfo,
                        unsigned int matchinfosize,
                        void *instance_data);
...
};


struct xt_entry_match
{
...
        void *instance_data;
        unsigned char data[0];
};


This would solve the general problem of entry specific data (tracking, state, 
accounting, whatever).
It gives more expressiveness to the API in general and would have little 
impact on the parts of the code that do not use it (instance_data is 
initialized as NULL and never used).
The performance penalty would be negligible (4 or 8 more bytes in every 
descriptor and in every function call to the match function).

We could minimize impact on the existing code introducing a "normal" and an 
"extended" match funciton prototype, but this would crete complexity and its 
only purpose is if we don't want to upgrade everything.

The problem with my solution is:
- it's work (I could do it, so it's not your problem :));
- the kernel people may reject it or delay it for an extended period of time;
- while the patch is available and the kernel people does not accept it 
we'll be stuck with 2 sets of incompatible netfilter APIs with all kinds of 
inconveniences while everyone updates their function declarations (we already 
survived one such change with 2.6.16).

I could implement the "compatible" solution (2 sets of funtions) specifying 
that the old one is soon to become obsolete and will be removed in a few 
months, but I fear that we'll never really do the transition.

Is there anything that you wouldn't like about it?
Are there better ways to the same goal?
How best to survive the transition?
Please give me any feedback.

-- 
Bye,
   Massimiliano Hofer
        Nucleus

Re: netfilter supporto for entry data

[Patrick McHardy]

Massimiliano Hofer wrote:
> Hi,
> I wrote some time ago about the possibility of entry specific data for 
> matches. There are several matches that could use a pointer to a structure 
> allocated and initialized during checkentry() and disposed of during 
> destroy().

Yes, sorry for not responding earlier.

> 
> Is there any reason the API doosn't provide a way to do this?
> 
> I'd like to change these structures (and all corresponding references and 
> initialization code) as follows:
> 
> struct xt_match
> {
> ...
>         int (*match)(const struct sk_buff *skb,
>                      const struct net_device *in,
>                      const struct net_device *out,
>                      const void *matchinfo,
>                      int offset,
>                      unsigned int protoff,
>                      int *hotdrop,
>                      void *instance_data);
> 
>         int (*checkentry)(const char *tablename,
>                           const void *ip,
>                           void *matchinfo,
>                           unsigned int matchinfosize,
>                           unsigned int hook_mask,
>                           void **instance_data);
> 
>         void (*destroy)(void *matchinfo,
>                         unsigned int matchinfosize,
>                         void *instance_data);
> ...
> };
> 
> 
> struct xt_entry_match
> {
> ...
>         void *instance_data;
>         unsigned char data[0];
> };
> 
> 
> This would solve the general problem of entry specific data (tracking, state, 
> accounting, whatever).
> It gives more expressiveness to the API in general and would have little 
> impact on the parts of the code that do not use it (instance_data is 
> initialized as NULL and never used).
> The performance penalty would be negligible (4 or 8 more bytes in every 
> descriptor and in every function call to the match function).
> 
> We could minimize impact on the existing code introducing a "normal" and an 
> "extended" match funciton prototype, but this would crete complexity and its 
> only purpose is if we don't want to upgrade everything.


Your goals are definitely worthwhile, but if we find a working solution
we should just convert everything.

> The problem with my solution is:
> - it's work (I could do it, so it's not your problem :));
> - the kernel people may reject it or delay it for an extended period of time;
> - while the patch is available and the kernel people does not accept it 
> we'll be stuck with 2 sets of incompatible netfilter APIs with all kinds of 
> inconveniences while everyone updates their function declarations (we already 
> survived one such change with 2.6.16).


We try not to unreasonably reject good patches :) In this case however
the patch breaks userspace compatibility (xt_entry_match is also used
by userspace), so this isn't possible.

Mhh .. thinking about it some more, we might be able to squeeze it
into the u.kernel part, there should be enough room left in there.

Re: netfilter support for entry data

[Massimiliano Hofer]

On Friday 26 May 2006 3:42 pm, Patrick McHardy wrote:

> Yes, sorry for not responding earlier.

Don't worry, I've been busy anyway.

> > We could minimize impact on the existing code introducing a "normal" and
> > an "extended" match funciton prototype, but this would crete complexity
> > and its only purpose is if we don't want to upgrade everything.
>
> Your goals are definitely worthwhile, but if we find a working solution
> we should just convert everything.

OK. In a few days I'll post a patch that adds the new functionality to the 
core of netfilter and another patch that upgrades all mainline matches.

If you like it, it will be easy to adapt all existing out of mainline matches. 
If this causes an uproar I can create a compatibility patch, but I wouldn't 
like it to be long lived.

> We try not to unreasonably reject good patches :) In this case however
> the patch breaks userspace compatibility (xt_entry_match is also used
> by userspace), so this isn't possible.

Ops... I didn't read much of the userspace code and didn't realize it. I would 
have realized it soon after starting, but you saved me time and trouble.

> Mhh .. thinking about it some more, we might be able to squeeze it
> into the u.kernel part, there should be enough room left in there.

You are right. We should have at least 20 bytes (depending on architecture) 
there. Putting important pointers in a union just to share a struct with 
userspace feels a bit weird, but is definitely feasible.
I'll follow your advice.

-- 
Bye,
   Massimiliano Hofer