: netfilter update

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

[email protected], 2004-09-20 11:55:28+02:00, [email protected]
[NETFILTER]: add comment match

2.4 version by Brad Fisher <[email protected]>

[email protected], 2004-09-20 11:54:00+02:00, [email protected]
[NETFILTER]: Fix invalid return values in sctp_new

[email protected], 2004-09-20 11:52:16+02:00, [email protected]
[NETFILTER]: Fix two broken assertions

[email protected], 2004-09-19 18:18:43+02:00, [email protected]
[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.

[email protected], 2004-09-19 18:08:05+02:00, [email protected]
[NETFILTER]: lookup sockets for incoming packets in ipt_owner

[email protected], 2004-09-19 16:28:21+02:00, [email protected]
[NETFILTER]: Keep conntrack/nat protocols in array instead of linked list

[email protected], 2004-09-19 15:33:35+02:00, [email protected]
[NETFILTER]: Use u_int16_t for initialized/num_manips in struct
ip_nat_info

[email protected], 2004-09-19 15:29:24+02:00, [email protected]
[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.

[email protected], 2004-09-19 00:05:29+02:00, [email protected]
[NETFILTER]: kill struct ip_nat_hash, saves two pointers per conntrack

The back-pointer is not needed when using list.h macros.

[email protected], 2004-09-18 23:44:40+02:00, [email protected]
[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).

[email protected], 2004-09-18 23:27:31+02:00, [email protected]
[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.

[email protected], 2004-09-18 23:18:23+02:00, [email protected]
[NETFILTER]: Don't try to do any random dropping since we now use
jenkins hash

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

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.

Oops. That wasn't very smart of me.

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 ?

Thanks.

Regards
Patrick

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.

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.

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

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

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.

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

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.

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

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 :)

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

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