[PATCH 2.6 0/12]: netfilter update

[PATCH 2.6 0/12]: netfilter update

[Patrick McHardy]

Hi Dave,

following are 12 mostly random netfilter patches for 2.6.
You can also pull all changes from bk://212.42.230.204/2.6-netfilter

Regards
Patrick


ChangeSet@1.1935.1.12, 2004-09-20 11:55:28+02:00, kaber@coreworks.de
  [NETFILTER]: add comment match
 
  2.4 version by Brad Fisher <brad@info-link.net>
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.11, 2004-09-20 11:54:00+02:00, kaber@coreworks.de
  [NETFILTER]: Fix invalid return values in sctp_new
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.10, 2004-09-20 11:52:16+02:00, kaber@coreworks.de
  [NETFILTER]: Fix two broken assertions
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.9, 2004-09-19 18:18:43+02:00, gandalf@wlug.westbo.se
  [NETFILTER]: Cleanup ctstat
 
  This patch simply adds a macro to increase the statistics.
  And it changes icmp_error to error in struct ip_conntrack_stat in order
  to adopt to the tcp-windowtracking changes.
 
  Based on patch by Pablo Neira.
 
  Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.8, 2004-09-19 18:08:05+02:00, kaber@coreworks.de
  [NETFILTER]: lookup sockets for incoming packets in ipt_owner
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.7, 2004-09-19 16:28:21+02:00, kaber@coreworks.de
  [NETFILTER]: Keep conntrack/nat protocols in array instead of linked list
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.6, 2004-09-19 15:33:35+02:00, kaber@coreworks.de
  [NETFILTER]: Use u_int16_t for initialized/num_manips in struct 
ip_nat_info
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.5, 2004-09-19 15:29:24+02:00, kaber@coreworks.de
  [NETFILTER]: kill struct nf_ct_info, saves five pointers per conntrack
 
  The relationship of the skb to the conntrack is stored in a new field
  in the skb.
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.4, 2004-09-19 00:05:29+02:00, kaber@coreworks.de
  [NETFILTER]: kill struct ip_nat_hash, saves two pointers per conntrack
 
  The back-pointer is not needed when using list.h macros.
 
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.3, 2004-09-18 23:44:40+02:00, rusty@rustcorp.com.au
  [NETFILTER]: Shuffle conntrack structure for better cacheline behavior
 
  Every time we walk the conntrack hashtable list, we hit the same
  cacheline that is dirtied by the use of the conntrack
  entry. Shuffling these entries to the end should help this
  (sizeof(struct ip_conntrack)) > cacheline size).
 
  Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.2, 2004-09-18 23:27:31+02:00, laforge@netfilter.org
  [NETFILTER]: add sysctl to read out the number of current connections
 
  Apparently a lot of scripts use a construct like
          "cat /proc/net/ip_conntrack | wc -l"
  which has a negative impact on system performance due to all the locking
  required.
 
  Signed-off-by: Harald Welte <laforge@netfilter.org>
  Signed-off-by: Patrick McHardy <kaber@trash.net>

ChangeSet@1.1935.1.1, 2004-09-18 23:18:23+02:00, rusty@rustcorp.com.au
  [NETFILTER]: Don't try to do any random dropping since we now use 
jenkins hash
 
  Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
  Signed-off-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH 2.6 0/12]: netfilter update

[David S. Miller]

On Tue, 21 Sep 2004 05:20:28 +0200
Patrick McHardy <kaber@trash.net> wrote:

> following are 12 mostly random netfilter patches for 2.6.
> You can also pull all changes from bk://212.42.230.204/2.6-netfilter

There are big conflicts with Rusty's recent
seq file fixes to ip_conntrack_standalone.c

Please resync with Linus's current tree and
use "bk resolve" or whatever means to fix this
up.

Re: [PATCH 2.6 0/12]: netfilter update

[Patrick McHardy]

David S. Miller wrote:

>On Tue, 21 Sep 2004 05:20:28 +0200
>Patrick McHardy <kaber@trash.net> wrote:
>
>  
>
>>following are 12 mostly random netfilter patches for 2.6.
>>You can also pull all changes from bk://212.42.230.204/2.6-netfilter
>>    
>>
>
>There are big conflicts with Rusty's recent
>seq file fixes to ip_conntrack_standalone.c
>
>Please resync with Linus's current tree and
>use "bk resolve" or whatever means to fix this
>up.
>  
>

Done. The updated tree is at the same location.

Regards
Patrick

Re: [PATCH 2.6 0/12]: netfilter update

[David S. Miller]

On Wed, 22 Sep 2004 01:38:10 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Done. The updated tree is at the same location.

Thanks, two problems:

1) Please layout the new skbuff.h structure changes more
   intelligently.  Currently the netfilter stuff is:

	pointer
	u32
	pointer
	u32

   and this wastes a lot of space on 64-bitters.

2) There is no way I'm amicable to exporting __tcp_v4_lookup()
   and friends to modules.

   Are you absolutely sure that the existing scheme cannot
   work properly?  I'm talking about relying upon skb->sk
   being sane.

So what I think I'll do is merge in all the changes other
than the ipt_owner.c change by hand into a tree I'll push
to Linus, I'll also fix the layout ordering problem which
I mentioned above in #1

But something has to be done about the ipt_owner.c change, it
isn't going in as-is.

Re: [PATCH 2.6 0/12]: netfilter update

[Patrick McHardy]

David S. Miller wrote:

>Thanks, two problems:
>
>1) Please layout the new skbuff.h structure changes more
>   intelligently.  Currently the netfilter stuff is:
>
>	pointer
>	u32
>	pointer
>	u32
>
>   and this wastes a lot of space on 64-bitters.
>
Oops. That wasn't very smart of me.

>2) There is no way I'm amicable to exporting __tcp_v4_lookup()
>   and friends to modules.
>
>   Are you absolutely sure that the existing scheme cannot
>   work properly?  I'm talking about relying upon skb->sk
>   being sane.
>  
>
The existing scheme has one problem, it takes files->file_lock
in a BH, but the patch doesn't mean to fix this. Besides
this, it works fine. The patch extends the owner match so it
can also be used in the input path, where skb->sk isn't set.
This is quite useful for filtering input traffic based on user
etc., unfortunately it can't be done without exporting
{tcp,udp}_v4_lookup. Do you see an alternative to dropping the
patch ?

>So what I think I'll do is merge in all the changes other
>than the ipt_owner.c change by hand into a tree I'll push
>to Linus, I'll also fix the layout ordering problem which
>I mentioned above in #1
>  
>
Thanks.

Regards
Patrick

Re: [PATCH 2.6 0/12]: netfilter update

[David S. Miller]

On Wed, 22 Sep 2004 03:42:24 +0200
Patrick McHardy <kaber@trash.net> wrote:

> The existing scheme has one problem, it takes files->file_lock
> in a BH, but the patch doesn't mean to fix this. Besides
> this, it works fine. The patch extends the owner match so it
> can also be used in the input path, where skb->sk isn't set.
> This is quite useful for filtering input traffic based on user
> etc., unfortunately it can't be done without exporting
> {tcp,udp}_v4_lookup. Do you see an alternative to dropping the
> patch ?

I think the scheme is a bit illogical.

Until the actual TCP socket lookup call, you cannot say for sure
what that lookup would result in.

For example, let's say that between where the ipt_owner match
is invoked on input, and the actual call to tcp_v4_rcv(), some
filtering or other packet mangling scheme changes the IP address
or ports.  You're going to get different results in ipt_owner()
than TCP was going to achieve.

It sounds like we want some kind of socket resolution policy check.
It's starting to get rediculious, as we have two filters _already_
in that code path, one for the per-socket BPF style filtering, and
the other for IPSEC.

The IPSEC check is available on output too.

I don't see how it's avoidable to add another condition there.
So what if we had something like:

	NF_SK_HOOK(PF_INET, NF_TCP_IN, sk, skb, skb->dev, NULL,
		   tcp_v4_rcv_finish)

or something along those lines?  Perhaps you can simplify the
argument set even further.

Re: [PATCH 2.6 0/12]: netfilter update

[Patrick McHardy]

David S. Miller wrote:

>I think the scheme is a bit illogical.
>
>Until the actual TCP socket lookup call, you cannot say for sure
>what that lookup would result in.
>
>For example, let's say that between where the ipt_owner match
>is invoked on input, and the actual call to tcp_v4_rcv(), some
>filtering or other packet mangling scheme changes the IP address
>or ports.  You're going to get different results in ipt_owner()
>than TCP was going to achieve.
>
>It sounds like we want some kind of socket resolution policy check.
>It's starting to get rediculious, as we have two filters _already_
>in that code path, one for the per-socket BPF style filtering, and
>the other for IPSEC.
>
>The IPSEC check is available on output too.
>  
>

It's mainly a problem if the socket is closed or replaced between the
lookup in ipt_owner and the lookup in tcp, for the other cases you
can define the semantic as: the socket that the packet would be
delivered to if the packet is not dropped and none of the keys used
for the lookup are altered.

>I don't see how it's avoidable to add another condition there.
>So what if we had something like:
>
>	NF_SK_HOOK(PF_INET, NF_TCP_IN, sk, skb, skb->dev, NULL,
>		   tcp_v4_rcv_finish)
>
>or something along those lines?  Perhaps you can simplify the
>argument set even further.
>  
>
Unfortunately I have to agree with you, another set of hooks looks
like the only way to solve the race. Let me think some more about
the implications for iptables and ip_conntrack.

Regards
Patrick

Re: [PATCH 2.6 0/12]: netfilter update

[Henrik Nordstrom]

On Sun, 26 Sep 2004, Patrick McHardy wrote:

> Unfortunately I have to agree with you, another set of hooks looks
> like the only way to solve the race. Let me think some more about
> the implications for iptables and ip_conntrack.

conntrack should not see this new hook.

what do do in iptables is a question.. as it is yet another step in the 
packet processing it calls for a new builtin chain I think.

Regards
Henrik

Re: [PATCH 2.6 0/12]: netfilter update

[Patrick McHardy]

Henrik Nordstrom wrote:

> On Sun, 26 Sep 2004, Patrick McHardy wrote:
>
>> Unfortunately I have to agree with you, another set of hooks looks
>> like the only way to solve the race. Let me think some more about
>> the implications for iptables and ip_conntrack.
>
>
> conntrack should not see this new hook.


We confirm conntrack entries in LOCAL_IN after they passed all hooks,
but this new set of hooks would be after LOCAL_IN, so conntrack entries
should be confirmed there.

>
> what do do in iptables is a question.. as it is yet another step in 
> the packet processing it calls for a new builtin chain I think.

I agree. But I wonder if its worth it just for having the owner match
work in the input path, or if there are other uses for these hooks.

Regards
Patrick

Re: [PATCH 2.6 0/12]: netfilter update

[Henrik Nordstrom]

On Sun, 26 Sep 2004, Patrick McHardy wrote:

> We confirm conntrack entries in LOCAL_IN after they passed all hooks,
> but this new set of hooks would be after LOCAL_IN, so conntrack entries
> should be confirmed there.

Right. THis would also ensure that connections not picked up by the host 
does not get confirmed, but there is many complications if attempting this 
path.

- It must be fully assured no packets is silently accepted by the host 
without entering this new hook.

- Fragmented packets is again an issue.

> I agree. But I wonder if its worth it just for having the owner match
> work in the input path, or if there are other uses for these hooks.

Not sure either. But then it is also the case I have never really been 
sure about the current independent relation of conntrack and the local 
host sockets..

Regards
Henrik

Re: [PATCH 2.6 0/12]: netfilter update

[Patrick McHardy]

Henrik Nordstrom wrote:

> On Sun, 26 Sep 2004, Patrick McHardy wrote:
>
>> We confirm conntrack entries in LOCAL_IN after they passed all hooks,
>> but this new set of hooks would be after LOCAL_IN, so conntrack entries
>> should be confirmed there.
>
>
> Right. THis would also ensure that connections not picked up by the 
> host does not get confirmed, but there is many complications if 
> attempting this path.
>
> - It must be fully assured no packets is silently accepted by the host 
> without entering this new hook.


ipip and gre don't have sockets, they could call the hook with
sk = NULL. Some icmp types are processed without a socket, same
here. Packets without a protocol or raw-socket for that protocol
are dropped in ip_local_deliver_finish, another hook would be
required there. With all this, the existing LOCAL_IN hook becomes
pratically useless. So we could just extend the existing NF_HOOK
macro by an "sk" argument. In LOCAL_OUT, it would be set to skb->sk
and, for symetry, to NULL in POST_ROUTING. One big advantage of
this is that it would allow filtering packets from a transport
mode SA without reinjecting them into the stack. I think I have
to try it to see how ugly it gets :)

>
> - Fragmented packets is again an issue.


Fragmentation is not a problem, packets are already defragmented
when received on a socket.


>> I agree. But I wonder if its worth it just for having the owner match
>> work in the input path, or if there are other uses for these hooks.
>
>
> Not sure either. But then it is also the case I have never really been 
> sure about the current independent relation of conntrack and the local 
> host sockets..


The socket could cache the conntrack entry, but then you
run into problems with icmp and tcp, who uses the same socket
(per-CPU in case of icmp) for replies generated by the kernel
(RST, icmp reply, ...). Other than that, I don't see any
advantage, we would still need the socket independent code for
forwarded packets.

Regards
Patrick