2.6.20 support for other 'features'

2.6.20 support for other 'features'

[Mohammad Mohsenzadeh]

Hello,

I have been developing extension for netfilter to allow stateful
firewall and filtering. I was looking at the new api in 2.6.20 and I
was wondering why the number of 'features' allowed is maxed to 4.

I was thinking of configuration number of features and maybe some way
of setting a default feature to use instead of NF_CT_F_BASIC. This way
the netfilter can easily be extended. I can provide a patch if you
think the above is a good idea.

Best regards

Mohammad

Re: 2.6.20 support for other 'features'

[Jan Engelhardt]

On Apr 11 2007 13:39, Mohammad Mohsenzadeh wrote:
>
> I have been developing extension for netfilter to allow stateful
> firewall and filtering.

Are not we stateful yet?
(To answer myself: Technically no, but we are stateful enough
for most users :-)

> I was looking at the new api in 2.6.20 and I
> was wondering why the number of 'features' allowed is maxed to 4.

Four?


Jan
-- 

Re: 2.6.20 support for other 'features'

[Mohammad Mohsenzadeh]

> Are not we stateful yet?
> (To answer myself: Technically no, but we are stateful enough
> for most users :-)

That's true, my extension will extend that by looking at the entire
packet payload similar but to layer7 filter and ipp2p projects.

> Four?

The features that can be registered using nf_conntrack_register_cache
is limited to NF_CT_F_NUM which is defined in nf_conntrack.h to 4.

Mohammad

Re: 2.6.20 support for other 'features'

[Patrick McHardy]

Mohammad Mohsenzadeh wrote:
> I have been developing extension for netfilter to allow stateful
> firewall and filtering. I was looking at the new api in 2.6.20 and I
> was wondering why the number of 'features' allowed is maxed to 4.
> 
> I was thinking of configuration number of features and maybe some way
> of setting a default feature to use instead of NF_CT_F_BASIC. This way
> the netfilter can easily be extended. I can provide a patch if you
> think the above is a good idea.


Lets see the patch to decide whether this is a good idea :)

But the feature stuff needs to be reworked in the not too distant
future, its impossible to properly determine what features might
be used in the future at allocation time, which is why we always
allocate for the worst case anyway and still can't properly deal
with the NAT module being loaded late.

Re: 2.6.20 support for other 'features'

[Patrick McHardy]

Mohammad Mohsenzadeh wrote:
> I have been developing extension for netfilter to allow stateful
> firewall and filtering. I was looking at the new api in 2.6.20 and I
> was wondering why the number of 'features' allowed is maxed to 4.
> 
> I was thinking of configuration number of features and maybe some way
> of setting a default feature to use instead of NF_CT_F_BASIC. This way
> the netfilter can easily be extended. I can provide a patch if you
> think the above is a good idea.


Actually on second thought, I don't think this can even work.
The features and the resulting memory layout of struct nf_conn
need to be coordinated for all different combinations, so
you can at most extend it by one, which would always come at
the end.

Re: 2.6.20 support for other 'features'

[Mohammad Mohsenzadeh]

On 4/11/07, Patrick McHardy <kaber@trash.net> wrote:
>
> But the feature stuff needs to be reworked in the not too distant
> future, its impossible to properly determine what features might
> be used in the future at allocation time, which is why we always
> allocate for the worst case anyway and still can't properly deal
> with the NAT module being loaded late.

We would still have this problem (even worse), because we would never
know what or how many features will be registered by other modules. At
least right now, we know the worse case is 4 when both nf_nat and
nf_helper are registered. But we could perhaps have a module parameter
to specify the worse case. Something like
      insmod nf_conntrack nf_conntrack_max_features=8

>
> Actually on second thought, I don't think this can even work.
> The features and the resulting memory layout of struct nf_conn
> need to be coordinated for all different combinations, so
> you can at most extend it by one, which would always come at
> the end.
>

That should be find. The layout will be like the following
feature #        layout
4                   nf_conn .. extra
5                   nf_conn .. helper .. extra
6                   nf_conn .. nat .. extra
7                   nf_conn .. nat .. helper .. extra

and so on. Therefore you can have as many features as you want. It
just extends the 'extra' part above.

I also try to post the patch here shortly.

Mohammad

Re: 2.6.20 support for other 'features'

[Patrick McHardy]

Mohammad Mohsenzadeh wrote:
> On 4/11/07, Patrick McHardy <kaber@trash.net> wrote:
> 
>>
>> But the feature stuff needs to be reworked in the not too distant
>> future, its impossible to properly determine what features might
>> be used in the future at allocation time, which is why we always
>> allocate for the worst case anyway and still can't properly deal
>> with the NAT module being loaded late.
> 
> 
> We would still have this problem (even worse), because we would never
> know what or how many features will be registered by other modules. At
> least right now, we know the worse case is 4 when both nf_nat and
> nf_helper are registered. But we could perhaps have a module parameter
> to specify the worse case. Something like
>      insmod nf_conntrack nf_conntrack_max_features=8


I'm not talking about the worst case for the number of features but
about the worst case used features for a specific conntrack entry.
We don't know if NAT will assign a helper later on, so we need
to allocate it in advance. We also don't know if they user will
load the NAT module later on, but we ignore this case and don't
handle it 100% correct.

>> Actually on second thought, I don't think this can even work.
>> The features and the resulting memory layout of struct nf_conn
>> need to be coordinated for all different combinations, so
>> you can at most extend it by one, which would always come at
>> the end.
>>
> 
> That should be find. The layout will be like the following
> feature #        layout
> 4                   nf_conn .. extra
> 5                   nf_conn .. helper .. extra
> 6                   nf_conn .. nat .. extra
> 7                   nf_conn .. nat .. helper .. extra
> 
> and so on. Therefore you can have as many features as you want. It
> just extends the 'extra' part above.

And how would it do that for two external modules, that don't know
how large the last part is?


> I also try to post the patch here shortly.


OK.

Re: 2.6.20 support for other 'features'

[Mohammad Mohsenzadeh]

On 4/11/07, Patrick McHardy <kaber@trash.net> wrote:
> Mohammad Mohsenzadeh wrote:
> > On 4/11/07, Patrick McHardy <kaber@trash.net> wrote:
>
> >> Actually on second thought, I don't think this can even work.
> >> The features and the resulting memory layout of struct nf_conn
> >> need to be coordinated for all different combinations, so
> >> you can at most extend it by one, which would always come at
> >> the end.
> >>
> >
> > That should be find. The layout will be like the following
> > feature #        layout
> > 4                   nf_conn .. extra
> > 5                   nf_conn .. helper .. extra
> > 6                   nf_conn .. nat .. extra
> > 7                   nf_conn .. nat .. helper .. extra
> >
> > and so on. Therefore you can have as many features as you want. It
> > just extends the 'extra' part above.
>
> And how would it do that for two external modules, that don't know
> how large the last part is?
>

That's absolutely correct. How about having only one slab cache and a
dynamic list of features. Whenever a new feature is registered or an
old one is removed, we can reallocate slab cache with a new size. I'm
not sure if that would work for sure, just throwing some ideas out
there. Something like

struct nf_conntrack_feature {
    struct list_head list;

    /* name of feature */
    unsigned char name[NF_CT_FEATURES_NAMELEN];

    /* size of feature */
    size_t size;

    /* feature offset for private data */
    size_t offset;

    /* modules which use this feature */
    int use;
};

static struct {
    /* dynamic list of features */
    struct list_head features;

	/* size of slab cache */
	size_t size;

	/* slab cache pointer */
	struct kmem_cache *cachep;

} nf_ct_cache;


One problem that I can see with this is that we have flush all our
conntracks so we can call 	kmem_cache_destroy and then recreate the
slab with new size. Would this be alright since it would only happen
on registering and unregistering features since it doesn't happen
often.

Mohammad

Re: 2.6.20 support for other 'features'

[Patrick McHardy]

Mohammad Mohsenzadeh wrote:
> On 4/11/07, Patrick McHardy <kaber@trash.net> wrote:
> 
>> And how would it do that for two external modules, that don't know
>> how large the last part is?
>>
> 
> That's absolutely correct. How about having only one slab cache and a
> dynamic list of features. Whenever a new feature is registered or an
> old one is removed, we can reallocate slab cache with a new size. I'm
> not sure if that would work for sure, just throwing some ideas out
> there. Something like
> 
> [...]
> One problem that I can see with this is that we have flush all our
> conntracks so we can call     kmem_cache_destroy and then recreate the
> slab with new size. Would this be alright since it would only happen
> on registering and unregistering features since it doesn't happen
> often.


No, I don't like that, besides a single slab cache would force us to
waste space by always allocating the maximum size even if not needed
for a specific conntrack (helpers for example are rare). Rusty had a
better idea, search the archives for ct_extend.

Re: 2.6.20 support for other 'features'

[Yasuyuki KOZAKAI]

Hi,

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 13 Apr 2007 06:22:29 +0200

> > One problem that I can see with this is that we have flush all our
> > conntracks so we can call     kmem_cache_destroy and then recreate the
> > slab with new size. Would this be alright since it would only happen
> > on registering and unregistering features since it doesn't happen
> > often.
> 
> 
> No, I don't like that, besides a single slab cache would force us to
> waste space by always allocating the maximum size even if not needed
> for a specific conntrack (helpers for example are rare).

And that is too complicated to reallocate conntrack and restructure
relationship among them. That's why the current nf_connrack doesn't
reallocate conntrack even if NAT and all helper modules are unloaded.


>                                                          Rusty had a
> better idea, search the archives for ct_extend.

In the current, I think that ct_extend is the best way to manage memory
space for conntrack.

IIRC he said that more benchmarks are necessary, but anyway, I'll look his
code again. I think we can improve his code more.

-- Yasuyuki Kozakai

Re: 2.6.20 support for other 'features'

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Fri, 13 Apr 2007 06:22:29 +0200
> 
>>Rusty had a better idea, search the archives for ct_extend.
> 
> 
> In the current, I think that ct_extend is the best way to manage memory
> space for conntrack.
> 
> IIRC he said that more benchmarks are necessary, but anyway, I'll look his
> code again. I think we can improve his code more.


Great, I'm glad someone is looking into this.