updates for [nf|ct]netlink and event API

Hi Harald,

This patchset introduces tons of updates for the nfnetlink, ctnetlink and the conntrack event API. I haven't attached the file since it's that big, about 100K.

You can get an incremental diff against SVN from:
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/SVN-patches/ctnetlink-ctevent-nfnetlink-update-2.6.12.patch

I've split this big patch above into four pieces to make it easier to understand the changes:
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/2.6.11-vs-2.6.12/

- Implement ip_conntrack_stats dumping and reset (accounting)

- Implement get conntrack and destroy (accounting)

- Kill event/dump mask based (?). Although it's unique, I think that it could be useful for weak conntrack event notification (think of just new, established and destroy event notification to reduce performance impact).

the patch removes the whole old ip_conntrack_netlink.c file and replaces it with a new one. I don't really see why. An, now, you changed the directory name from 2.6.11 to 2.6.12. Is this neccessarry? Why would your recent changes to ctnetlink not work with a 2.6.11 kernel?

Quoted 2 lines

I've split this big patch above into four pieces to make it easier to understand the changes:
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/2.6.11-vs-2.6.12/

please remove stuff like

Quoted 3 lines

(quoted diff) +1 -1

-#if 0
+#if 1

#define DEBUGP printk

before releasing a patch, thanks.

Also, I would rather not like to have NFNL_SUBSYS_CTNETLINK_EXP deleted. I thought it would be nice to have expect handling separated from conntracks, since they really are two seperate data structures.

Apart from that I'm fine with all of your modifications.

I don't understand why the order of the expectation list was changed, though. Would you please explain why this was done?

Quoted 1 line

- Implement ip_conntrack_stats dumping and reset (accounting)

you want to dump the statistics via netlink? i'm not sure whether that is required. lnstat is the only program using those counters, and using /proc seems fine for that.

Quoted 1 line

- Implement get conntrack and destroy (accounting)

sorry, what are you referring to?

Quoted 2 lines

- Kill event/dump mask based (?). Although it's unique, I think that it
could be useful for weak conntrack event notification (think of just new, established and destroy event notification to reduce performance impact).

well, an interesting optimization. My major problem with this whole system (like the other IPCTNL_MSG_CONFIG stuff) is: how to correctly deal with multipel users.

Now, I think there is a way to do it right. Basically every application would only request the events that it is interested in, and the kernel would bitwise-or those events to create the event mask that actually is to be dumped into userspace.

It's a bit like multicast membership subscription with IGMP...

So the only question remaining is: how does the kernel clean up / expire the masks? If apps only subscribe but never unsubscribe, we would never clean up.

I'm not sure if we could somehow (cleanly) tie into the scoket close/destroy code. If yes, we could check if we can reduce the mask at socket destroy time.

Quoted 7 lines · Pablo Neira

Quoted 2 lines · Harald Welte

I don't understand why the order of the expectation list was changed, though. Would you please explain why this was done?

Review

Quoted 5 lines

Because of the table dumping logic I was using.

(quoted diff) +1 -1

-                       if (ct->id <= *id)
+                       if (ct->id >= *id)
                                continue;

Please ignore the patches I've sent you.

Quoted 5 lines · Harald Welte

Quoted 1 line

- Implement ip_conntrack_stats dumping and reset (accounting)

you want to dump the statistics via netlink? i'm not sure whether that is required. lnstat is the only program using those counters, and using /proc seems fine for that.

Could this be useful for accounting purposes? I mean, dump and reset as a single operation, something similar to conntrack -L -z, say conntrack -S -z.

Quoted 3 lines

Quoted 1 line

- Implement get conntrack and destroy (accounting)

sorry, what are you referring to?

An atomical operation to dump conntrack information and then destroy it. For example, if someone does:

conntrack -D --orig-src 1.1.1.1 --orig-dst 2.2.2.2 -p tcp --orig-src-port 10 --orig-dst-port 20

the it sends the conntrack info and destroy it. This could be worth it for accounting.

I could use multicast netlink subscrition, so the client can get subscribed to those events that the user considers interesting.

Quoted 7 lines · Pablo Neira

Quoted 5 lines · Harald Welte

Quoted 1 line

- Implement ip_conntrack_stats dumping and reset (accounting)

you want to dump the statistics via netlink? i'm not sure whether that is required. lnstat is the only program using those counters, and using /proc seems fine for that.

Quoted 3 lines

Could this be useful for accounting purposes? I mean, dump and reset as a single operation, something similar to conntrack -L -z, say conntrack -S -z.

I think it is convenient for anyone wanting to access these statistics without parsing /proc to have everything accessible through a single interface.

destroy notifications are sent anyway, so we get the stats. What's the difference to normal destruction?

Quoted 2 lines

I could use multicast netlink subscrition, so the client can get subscribed to those events that the user considers interesting.

That sounds like the right thing to do.

Quoted 4 lines · Pablo Neira

Quoted 2 lines · Harald Welte

Quoted 1 line

- Implement ip_conntrack_stats dumping and reset (accounting)

Quoted 7 lines

Quoted 3 lines

you want to dump the statistics via netlink? i'm not sure whether that is required. lnstat is the only program using those counters, and using /proc seems fine for that.

Could this be useful for accounting purposes? I mean, dump and reset as a single operation, something similar to conntrack -L -z, say conntrack -S -z.

I think it is convenient for anyone wanting to access these statistics without parsing /proc to have everything accessible through a single interface.

Quoted 2 lines

I don't understand why the order of the expectation list was changed, though. Would you please explain why this was done?

Because of the table dumping logic I was using.

Quoted 5 lines

Quoted 1 line

- Implement ip_conntrack_stats dumping and reset (accounting)

you want to dump the statistics via netlink? i'm not sure whether that is required. lnstat is the only program using those counters, and using /proc seems fine for that.

Could this be useful for accounting purposes? I mean, dump and reset as a single operation, something similar to conntrack -L -z, say conntrack -S -z.

An atomical operation to dump conntrack information and then destroy it. For example, if someone does:

conntrack -D --orig-src 1.1.1.1 --orig-dst 2.2.2.2 -p tcp --orig-src-port 10 --orig-dst-port 20

the it sends the conntrack info and destroy it. This could be worth it for accounting.

Quoted 3 lines

I'm not sure if we could somehow (cleanly) tie into the scoket close/destroy code. If yes, we could check if we can reduce the mask at socket destroy time.

I think there's no sane way to do this but let me check the netlink code again. I could use multicast netlink subscrition, so the client can get subscribed to those events that the user considers interesting.

Hi Harald,

This patchset introduces tons of updates for the nfnetlink, ctnetlink and the conntrack event API. I haven't attached the file since it's that big, about 100K.

You can get an incremental diff against SVN from:
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/SVN-patches/ctnetlink-ctevent-nfnetlink-update-2.6.12.patch

Pablo Neira wrote:

I haven't looked at the patch for a long time, and it removes and adds the whole file, so not sure if it is new ..

(quoted diff) +9 -0

+		/* This is tricky but it works. ip_nat_setup_info needs the
+		 * hook number as parameter, so let's do the correct
+		 * conversion and run away */
+		if (*status & IPS_SRC_NAT_DONE)
+			hooknum = NF_IP_POST_ROUTING; /* IP_NAT_MANIP_SRC */
+		else if (*status & IPS_DST_NAT_DONE)
+			hooknum = NF_IP_PRE_ROUTING;  /* IP_NAT_MANIP_DST */
+		else
+			return -EINVAL; /* Missing NAT flags */

This doesn't work reliably, locally generated packets never enter PRE_ROUTING but can be DNATed. I think the hook should be supplied by the user.

Quoted 3 lines · Patrick McHardy

This doesn't work reliably, locally generated packets never enter PRE_ROUTING but can be DNATed. I think the hook should be supplied by the user.

The macro HOOK2MANIP used is ip_nat_setup_info returns the same value (maniptype) for NF_IP_PRE_ROUTING and NF_IP_LOCAL_IN, so the same manipulation (DNAT) will be applied to such conntrack. Since we works with conntracks, I don't mind where the packets came from, just want to apply the NAT handling that the user has requested.

This patchset introduces tons of updates for the nfnetlink, ctnetlink and the conntrack event API. I haven't attached the file since it's that big, about 100K.

Quoted 3 lines · Pablo Neira

This patchset introduces tons of updates for the nfnetlink, ctnetlink and the conntrack event API. I haven't attached the file since it's that big, about 100K.

Looks good. A couple more comments on the patch, most are probably not related to recent changes. I remember beeing responsible for at least some of this :)

(quoted diff) +4 -0

+struct cta_proto {
+       unsigned char num_proto;        /* Protocol number IPPROTO_X */
+       union ip_conntrack_proto proto;
+};

These should be changed not to use internal conntrack structures, it will hurt us when we want to change them. Instead it should replicate the fields interesting for the user. Also please use fixed-size types instead of unions etc. All structures including u64 types should be padded to multiples of 8 so they are equally sized on 32-bit and 64-bit systems.

(quoted diff) +6 -0

+#define CTA_HELP_MAXNAMESZ     31
+
+struct cta_help {
+       char name[CTA_HELP_MAXNAMESZ];  /* name of conntrack helper */
+       union ip_conntrack_help help;
+};

I suggest to use 32, the length in the kernel is not fixed AFAICS and it will be padded to 32 in netlink messages anyway.

(quoted diff) +7 -0

+/* ctnetlink multicast groups: reports any change of ctinfo,
+ * ctstatus, or protocol state change.
+ */
+#define NFGRP_IPV4_CT_TCP      0x01
+#define NFGRP_IPV4_CT_UDP      0x02
+#define NFGRP_IPV4_CT_ICMP     0x04
+#define NFGRP_IPV4_CT_OTHER    0x08

I'm not sure how useful these groups are. I think groups for different event-types might be more useful to reduce the noise.

(quoted diff) +3 -0

+       h->name[CTA_HELP_MAXNAMESZ - 1] = '\0';
+       if (strcmp(helper->name, h->name))
+               return -EINVAL;

We changed ipt_helper to accept a null-byte string as wildcard string, probably a good idea to do the same here.

(quoted diff) +3 -0

+       ct = ip_conntrack_alloc(otuple, rtuple);
+       if (ct == NULL || IS_ERR(ct))
+               return -ENOMEM;

ip_conntrack_alloc doesn't return ERR_PTR()

++

++      /* Events were delivered: loopback mustn't notify events twice */
++      IPCT_DELIVERED_BIT = 11,
++      IPCT_DELIVERED = (1 << IPCT_DELIVERED_BIT),

I couldn't find any users, probably since this can't work with a per-conntrack flag since events are generated by packets.

++ IPEXP_TIMEOUT_BIT = 1,
++ IPEXP_TIMEOUT = (1 << IPEXP_TIMEOUT_BIT),
++};
++

No user here either.

It would be great if you could send a patch without renaming the file, that makes reviewing the changes a lot easier. Renaming can then be done in a seperate patch that doesn't change anything.

Quoted 8 lines · Patrick McHardy

(quoted diff) +7 -0

+/* ctnetlink multicast groups: reports any change of ctinfo,
+ * ctstatus, or protocol state change.
+ */
+#define NFGRP_IPV4_CT_TCP      0x01
+#define NFGRP_IPV4_CT_UDP      0x02
+#define NFGRP_IPV4_CT_ICMP     0x04
+#define NFGRP_IPV4_CT_OTHER    0x08

Quoted 2 lines

I'm not sure how useful these groups are. I think groups for different event-types might be more useful to reduce the noise.

Yes, this looks fine. So we could kill those and use an event subscription.

Quoted 4 lines

I couldn't find this function, but in 2.6.12 expectations aren't refcounted anymore. If they are again by this patch, the refcnt would be leaked in the following lines:

Review

Quoted 3 lines

(quoted diff) +3 -0

+       skb2 = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+       if (!skb2)
+               return -ENOMEM;

Thanks for catching up this, I'll fix it. I recovered the refcounting for expectations to avoid any possible race condition since I could be working with an expectation whose timeout has expired.

Does this help? Hope so.
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/2.6.11-vs-2.6.12/04ctnetlink.patch

(quoted diff) +4 -0

+struct cta_proto {
+       unsigned char num_proto;        /* Protocol number IPPROTO_X */
+       union ip_conntrack_proto proto;
+};

These should be changed not to use internal conntrack structures, it will hurt us when we want to change them. Instead it should replicate the fields interesting for the user. Also please use fixed-size types instead of unions etc. All structures including u64 types should be padded to multiples of 8 so they are equally sized on 32-bit and 64-bit systems.

(quoted diff) +7 -0

+/* ctnetlink multicast groups: reports any change of ctinfo,
+ * ctstatus, or protocol state change.
+ */
+#define NFGRP_IPV4_CT_TCP      0x01
+#define NFGRP_IPV4_CT_UDP      0x02
+#define NFGRP_IPV4_CT_ICMP     0x04
+#define NFGRP_IPV4_CT_OTHER    0x08

I'm not sure how useful these groups are. I think groups for different event-types might be more useful to reduce the noise.

Quoted 6 lines · Patrick McHardy

These should be changed not to use internal conntrack structures, it will hurt us when we want to change them. Instead it should replicate the fields interesting for the user. Also please use fixed-size types instead of unions etc. All structures including u64 types should be padded to multiples of 8 so they are equally sized on 32-bit and 64-bit systems.

agreed. However, we still don't have some kind of versioning in the protocol, too. I think we've learned by now that we need versioned structures ;)

Quoted 10 lines

(quoted diff) +7 -0

+/* ctnetlink multicast groups: reports any change of ctinfo,
+ * ctstatus, or protocol state change.
+ */
+#define NFGRP_IPV4_CT_TCP      0x01
+#define NFGRP_IPV4_CT_UDP      0x02
+#define NFGRP_IPV4_CT_ICMP     0x04
+#define NFGRP_IPV4_CT_OTHER    0x08

I'm not sure how useful these groups are. I think groups for different event-types might be more useful to reduce the noise.

that was my idea in the beginning (since I didn't think of events at that point).

Still, I think creating messages for any kind of event (even if noone listens) is too much overhead. netlink needs to be extended to deal with that issue.

Maybe the 'which socket is subscribed to which group' accounting should be done by the core netlink layer, which would then only export a merged bitmask of all netlink sockets. This way ctnetlink can easily check whether it makes sense to create a certain event message or not.

This should be useful for other netlink users, too.

Quoted 10 lines · Harald Welte

Quoted 6 lines · Patrick McHardy

These should be changed not to use internal conntrack structures, it will hurt us when we want to change them. Instead it should replicate the fields interesting for the user. Also please use fixed-size types instead of unions etc. All structures including u64 types should be padded to multiples of 8 so they are equally sized on 32-bit and 64-bit systems.

Quoted 3 lines

agreed. However, we still don't have some kind of versioning in the protocol, too. I think we've learned by now that we need versioned structures ;)

Netlink is easy to extend by adding new fields at the end if users only check for msgsize >= sizeof(struct). Do you think we should have versioning anyway?

Quoted 19 lines

Quoted 10 lines

(quoted diff) +7 -0

+/* ctnetlink multicast groups: reports any change of ctinfo,
+ * ctstatus, or protocol state change.
+ */
+#define NFGRP_IPV4_CT_TCP      0x01
+#define NFGRP_IPV4_CT_UDP      0x02
+#define NFGRP_IPV4_CT_ICMP     0x04
+#define NFGRP_IPV4_CT_OTHER    0x08

I'm not sure how useful these groups are. I think groups for different event-types might be more useful to reduce the noise.

that was my idea in the beginning (since I didn't think of events at that point).

Still, I think creating messages for any kind of event (even if noone listens) is too much overhead. netlink needs to be extended to deal with that issue.

Quoted 6 lines

Maybe the 'which socket is subscribed to which group' accounting should be done by the core netlink layer, which would then only export a merged bitmask of all netlink sockets. This way ctnetlink can easily check whether it makes sense to create a certain event message or not.

This should be useful for other netlink users, too.

I think that we could split the structure into fine grain fields. For example, CTA_TUPLE_ORIG would composed of:

CTA_ORIG_IPV4_SRC
CTA_ORIG_IPV4_DST
CTA_L4_PROTONUM
CTA_PROTO_IPV4_SRC
CTA_PROTO_IPV4_DST
CTA_DIR

So, instead of sending a packet that contains a reference to an ip_conntrack_tuple (CTA_TUPLE_ORIG), we'll have a set of fields (CTA_ORIG_IPV4_SRC + CTA_ORIG_IPV4_DST + ...) that compose such structure.

But I'll need a function to glue all the fields to create a ip_conntrack_tuple. Maybe too bloated?

Quoted 9 lines

Quoted 9 lines

Still, I think creating messages for any kind of event (even if noone listens) is too much overhead. netlink needs to be extended to deal with that issue.

Maybe the 'which socket is subscribed to which group' accounting should be done by the core netlink layer, which would then only export a merged bitmask of all netlink sockets. This way ctnetlink can easily check whether it makes sense to create a certain event message or not.

Quoted 1 line

Quoted 1 line

This should be useful for other netlink users, too.

Isn't netlink broadcast subscription enough? netlink_broadcast doesn't enqueue packets for a socket that isn't subscribed to a group, so the process never gets useless packets.

So I think that we can group event, say:

level 1 (weak):
- IPCT_NEW
- IPCT_DESTROY

level 2 (normal):
- IPCT_NEW
- IPCT_UPDATE
- IPCT_STATUS
- ...
- IPCT_DESTROY

At reserve some groups to let the user define some level whenever he wants. Although such level would be unique.

OTOH, there are 10 events currently, why is that bad creating a group per event?

Quoted 16 lines

So I think that we can group event, say:

level 1 (weak):
- IPCT_NEW
- IPCT_DESTROY

level 2 (normal):
- IPCT_NEW
- IPCT_UPDATE
- IPCT_STATUS
- ...
- IPCT_DESTROY

At reserve some groups to let the user define some level whenever he wants. Although such level would be unique.

Quoted 2 lines

OTOH, there are 10 events currently, why is that bad creating a group per event?

I don't think its bad, but I fear hitting the 32 group limit some day. Probably that fear is unfounded and we're never going to reach 32 different types of events. Harald, what do you think?

Quoted 16 lines · Patrick McHardy

Quoted 16 lines

So I think that we can group event, say:

level 1 (weak):
- IPCT_NEW
- IPCT_DESTROY

level 2 (normal):
- IPCT_NEW
- IPCT_UPDATE
- IPCT_STATUS
- ...
- IPCT_DESTROY

At reserve some groups to let the user define some level whenever he wants. Although such level would be unique.

Quoted 6 lines

Quoted 2 lines

OTOH, there are 10 events currently, why is that bad creating a group per event?

I don't think its bad, but I fear hitting the 32 group limit some day. Probably that fear is unfounded and we're never going to reach 32 different types of events. Harald, what do you think?

I think it's not all that much of a problem. I doubt that conntrack will get 22 more events anytime soon. The other problem is, don't we overlap with other nfnetlink-based groups?

nfnetlink acts as a a multiplex between netlink and the higher layers, so I guess we only have 32groups for nfnetlink as a whole... and there is other stuff like ip_queue and ULOG that is to be ported on top of nfnetlink...

Quoted 8 lines · Harald Welte

I think it's not all that much of a problem. I doubt that conntrack will get 22 more events anytime soon. The other problem is, don't we overlap with other nfnetlink-based groups?

nfnetlink acts as a a multiplex between netlink and the higher layers, so I guess we only have 32groups for nfnetlink as a whole... and there is other stuff like ip_queue and ULOG that is to be ported on top of nfnetlink...

You're right, we can't do that. In short:

nfnetlink currently relies on the fact that is the user program that decides if a packet was sent to it or not. In case that it wasn't, the program drops it.

Say we've got ip_queue and ip_conntrack_netlink on top of nfnetlink:
ip_queue sends a packet to user space and ip_conntrack_netlink sends event notifications. If both the ipqueue userspace program and libctnetlink are subscribed to the same group, they both will receive spam from each other.

To avoid this spamming situation, every subsystem could reserve just one group and user programs must get subscribed to the corresponding group that is interested in. In conclusion, nfnetlink would support up to 32 subsystems but user space programs won't get spam from other subsystem.

So, back to ip_conntrack_netlink, we still have the performance issue with regards to the events. Userspace programs will decide if the event received is interesting for them or not.

a) We could extend netlink code with some kind of ioctl that sets a mask to say what kind of events are interesting for a given userspace program.

b) The fact that we can send events and nobody's listening to them is more difficult to catch up. Maybe a function that checks if there are listeners available before sending an event could help: In that case such function would be kind of racy because we can lose some messages while a userspace program is binding to a given netlink socket. This isn't that annoying, I prefer losing a couple of messages than hurting performance generating messages that won't be ever delivered.

Quoted 4 lines · Pablo Neira

To avoid this spamming situation, every subsystem could reserve just one group and user programs must get subscribed to the corresponding group that is interested in. In conclusion, nfnetlink would support up to 32 subsystems but user space programs won't get spam from other subsystem.

That's how it should be done. Conntrack is an exceptional case in that it is the only part in the kernel I know of that generates asynchronous netlink messages at high speed, so I think reserving multiple groups is the right way.

Quoted 6 lines

So, back to ip_conntrack_netlink, we still have the performance issue with regards to the events. Userspace programs will decide if the event received is interesting for them or not.

a) We could extend netlink code with some kind of ioctl that sets a mask to say what kind of events are interesting for a given userspace program.

Please no ioctls. Groups are fine, if one group per event is too much we need to group them somehow. Maybe:

1) creation/destruction
2) status changes
3) timeout
4) protoinfo
5) helper
6) nat
7) expectations

Quoted 7 lines

b) The fact that we can send events and nobody's listening to them is more difficult to catch up. Maybe a function that checks if there are listeners available before sending an event could help: In that case such function would be kind of racy because we can lose some messages while a userspace program is binding to a given netlink socket. This isn't that annoying, I prefer losing a couple of messages than hurting performance generating messages that won't be ever delivered.

That race shouldn't be a problem. Userspace can't know when exactly it will start receiving events anyway.

So, should be a netlink function to check if there are currently listeners worth it?

Quoted 14 lines · Pablo Neira

I think that we could split the structure into fine grain fields. For example, CTA_TUPLE_ORIG would composed of:

CTA_ORIG_IPV4_SRC
CTA_ORIG_IPV4_DST
CTA_L4_PROTONUM
CTA_PROTO_IPV4_SRC
CTA_PROTO_IPV4_DST
CTA_DIR

So, instead of sending a packet that contains a reference to an ip_conntrack_tuple (CTA_TUPLE_ORIG), we'll have a set of fields (CTA_ORIG_IPV4_SRC + CTA_ORIG_IPV4_DST + ...) that compose such structure.

Quoted 2 lines

But I'll need a function to glue all the fields to create a ip_conntrack_tuple. Maybe too bloated?

I think its fine, but CTA_PROTO_IPV4_{SRC,DST} should be CTA_PROTO_{SRC,DST}, there's nothing related to IPv4 in them.

Please nest all attributes related to a tuple in CTA_TUPLE instead of adding them to the top-level.

Quoted 3 lines · Patrick McHardy

I think its fine, but CTA_PROTO_IPV4_{SRC,DST} should be CTA_PROTO_{SRC,DST}, there's nothing related to IPv4 in them.

That could be fine for nf_conntrack to differenciate between conntracks that represents an ipv4 and ipv6 connections.

CTA_PROTO_IPV4_SRC and CTA_PROTO_IPV6_SRC

Quoted 2 lines

Please nest all attributes related to a tuple in CTA_TUPLE instead of adding them to the top-level.

Sorry, how that will look like?

You add a new nfattr header with the type set to CTA_TUPLE and note the position of skb->tail. Then you add the nested attributes as usual. When you're done you set the length of the nfattr header to skb->tail - old_tail. The RTA_NEST/RTA_NEST_END macros handle this for rtnetlink (include/linux/rtnetlink.h).

* Patrick McHardy <[email protected]> 2005-06-30 03:00

Quoted 5 lines

You add a new nfattr header with the type set to CTA_TUPLE and note the position of skb->tail. Then you add the nested attributes as usual. When you're done you set the length of the nfattr header to skb->tail - old_tail. The RTA_NEST/RTA_NEST_END macros handle this for rtnetlink (include/linux/rtnetlink.h).

At some point we should introduce a "generic" attribute architecture for all netlink families. The connector stuff will need it as well, so maybe the time has come to actually do it.

Quoted 4 lines · Thomas Graf

At some point we should introduce a "generic" attribute architecture for all netlink families. The connector stuff will need it as well, so maybe the time has come to actually do it.

I agree, most of the macros are just copied without modification anyway. One more thing I would like to change is the excessive use of RTA_PUT with structures on the stack. Using __RTA_PUT and putting together these structures in-place would be much nicer. nfnetlink has copied this part of rtnetlink.

Good point, after all, I think structs have been used too often and we now suffer from backwards compatbility issues. The points likely to be argueable are things like trim responsibility in the error handling, i.e. delegate it down to the function which also created the header or trim on every level. Other than that the whole thing should be pretty straight forward.

What do you think about naming it nlattr?

Quoted 6 lines · Thomas Graf

Good point, after all, I think structs have been used too often and we now suffer from backwards compatbility issues. The points likely to be argueable are things like trim responsibility in the error handling, i.e. delegate it down to the function which also created the header or trim on every level. Other than that the whole thing should be pretty straight forward.

I think for nested attributes error handling should happen on the outer level. Just trimming on the inner level would leave a half-finished nested attribute.

* Patrick McHardy <[email protected]> 2005-06-30 15:27

Quoted 3 lines

I think for nested attributes error handling should happen on the outer level. Just trimming on the inner level would leave a half-finished nested attribute.

I can agree with this, the question that remains is: do we want to trim in functions where no nesting is done at all? i.e. things like the generic network statistics dumping interface.

Quoted 6 lines · Thomas Graf

* Patrick McHardy <[email protected]> 2005-06-30 15:27

Quoted 3 lines

I think for nested attributes error handling should happen on the outer level. Just trimming on the inner level would leave a half-finished nested attribute.

Quoted 3 lines

I can agree with this, the question that remains is: do we want to trim in functions where no nesting is done at all? i.e. things like the generic network statistics dumping interface.

Trimming isn't required in this case since the length is know and checked in advance. An error should still be propagated back of course so potential outer levels can do trimming.

This is only true if one attribute is added. What I really meant is something like em_text_dump() in em_text.c.

From: Thomas Graf <[email protected]>
Date: Thu, 30 Jun 2005 23:34:59 +0200

Quoted 2 lines

This is only true if one attribute is added. What I really meant is something like em_text_dump() in em_text.c.

Speaking of em_text.c, when one loads ematch rules into that module it does a printk(). I assume that's a thinko and should be removed?

Yes, it's a debugging leftover but I'll keep it in until rc3 so if problems appear I have some debugging information. Given that is acceptable.

Hi,

Here below a candidate of attributes for ctnetlink. As you could see some attributes are still missing, for example, those to modify internal helper structures like ip_ct_ftp... but that would be easy to add later.

enum ctattr_type {
    CTA_UNSPEC,
    CTA_TUPLE,
    CTA_STATUS,
    CTA_PROTOINFO,
    CTA_HELP,
    CTA_NAT,
    CTA_TIMEOUT,
    CTA_MARK,
    CTA_COUNTERS,
    CTA_EXPECT,
    __CTA_MAX
};

enum ctattr_tuple {
    CTA_TUPLE_DIR,
    CTA_TUPLE_IP,
    CTA_TUPLE_PROTO,
    __CTA_TUPLE_MAX
};

enum ctattr_ip {
    CTA_IP_V4_SRC,
    CTA_IP_V4_DST,
    CTA_IP_V6_SRC,
    CTA_IP_V6_DST,
    __CTA_IP_MAX
};

enum ctattr_l4proto {
    CTA_PROTO_NUM,
    CTA_PROTO_TCP_SRC,
    CTA_PROTO_TCP_DST,
    CTA_PROTO_UDP_SRC,
    CTA_PROTO_UDP_DST,
    CTA_PROTO_ICMP_TYPE,
    CTA_PROTO_ICMP_CODE,
    CTA_PROTO_SCTP_CODE,
    __CTA_PROTO_MAX
};

enum ctattr_protoinfo {
CTA_PROTOINFO_TCP,
__CTA_PROTOINFO_MAX
};

enum ctattr_protoinfo_tcp {
    CTA_TCP_STATE,
    __CTA_TCP_MAX
};

enum ctattr_counters {
CTA_COUNTERS_PACKETS,
CTA_COUNTERS_BYTES,
__CTA_COUNTERS_MAX
};

enum ctattr_nat {
    CTA_NAT_MINIP,
    CTA_NAT_MAXIP,
    CTA_NAT_PROTO,
    __CTA_NAT_MAX
};

enum ctattr_protonat {
CTA_PROTONAT_MINIP,
CTA_PROTONAT_MAXIP,
CTA_PROTONAT_TCP,
CTA_PROTONAT_UDP,
__CTA_PROTONAT_MAX
};

enum ctattr_expect {
CTA_EXPECT_TUPLE,
CTA_EXPECT_MASK,
CTA_EXPECT_TIMEOUT,
__CTA_EXPECT_MAX
};

enum ctattr_help {
    CTA_HELP_NAME,
    __CTA_HELP_MAX
};

Comments welcome.

Signature · 1 line

Pablo

I'm interested in adding MAC addr attributes and CTA_TUPLE_MAC; not just because of my fixation with MAC addresses but also to use conntrack to record flow data for non IP-related protocols.

Quoted 3 lines

I'm interested in adding MAC addr attributes and CTA_TUPLE_MAC; not just because of my fixation with MAC addresses but also to use conntrack to record flow data for non IP-related protocols.

Can't this be done with ebtables?

On another note, how about bringing ebt and ipt closer together, maybe merge them?

Quoted 3 lines

On another note, how about bringing ebt and ipt closer together, maybe merge them?

yes, the good old pkttables idea. Unfortunately the ever growing number of gpl infringing companies keeps me from tackling this rater large project :(

Quoted 7 lines

Quoted 3 lines

On another note, how about bringing ebt and ipt closer together, maybe merge them?

yes, the good old pkttables idea. Unfortunately the ever growing number of gpl infringing companies keeps me from tackling this rater large project :(

Where is the (legal) problem?

I'm interested in adding MAC addr attributes and CTA_TUPLE_MAC; not just because of my fixation with MAC addresses but also to use conntrack to record flow data for non IP-related protocols.

I realise it is not the current intent of conntrack to do this, but as much as conntrack solves the high-load problems of ulog, it is desirable for it to keep counters for non-ip protocols.

As fast as protocol handlers can be devised it will become "conntrack" again instead of "flowtrack", but the universal identifier between hosts involved in a connection is the mac address and protocol.

The question is, how do others feel about conntrack tracking non-ip connections? I intend to provide patches to do this if it is welcome.

nf_conntrack goes the way to layer-3-independent connection tracking. However, conntrack will remain to happen at layer3 and 4 (with minor exceptions to higher layers in the case of conntrack helpers).

Quoted 4 lines

nf_conntrack goes the way to layer-3-independent connection tracking. However, conntrack will remain to happen at layer3 and 4 (with minor exceptions to higher layers in the case of conntrack helpers).

Minor? Hehe that's good :p Reminds me of l7-filter.sf.net (It's not even a helper as in ip_conntrack_irc, but a match module)

Quoted 5 lines · Amin Azez

I'm interested in adding MAC addr attributes and CTA_TUPLE_MAC; not just because of my fixation with MAC addresses but also to use conntrack to record flow data for non IP-related protocols.

ctnetlink MAC attributes are IMHO nonsense because there is no mac address tracking in conntrack.

Quoted 5 lines

I realise it is not the current intent of conntrack to do this, but as much as conntrack solves the high-load problems of ulog, it is desirable for it to keep counters for non-ip protocols.

Also, MAC address tracking doesn't really make sense since as an intermediate router there is no way that the assumption "source and destination mac never change" is ever valid.

Quoted 5 lines

As fast as protocol handlers can be devised it will become "conntrack" again instead of "flowtrack", but the universal identifier between hosts involved in a connection is the mac address and protocol.

no. you always only see the next hop mac address. let's say you have an upsteram and a downstream router, then all your 'connections' would be between the same two mac addresses.

Quoted 4 lines

The question is, how do others feel about conntrack tracking non-ip connections? I intend to provide patches to do this if it is welcome.

nf_conntrack goes the way to layer-3-independent connection tracking. However, conntrack will remain to happen at layer3 and 4 (with minor exceptions to higher layers in the case of conntrack helpers).

o conntrack event API

- IPCT_DELIVERED bit. Loopback reports event are reported twice, this bit is set once event are delivered. I just came up with a better idea, reset nfcache once the events have been delivered, but I'll apply this change in the next patchset.

Quoted 1 line · Pablo Neira

o conntrack event API

Quoted 2 lines

- IPCT_DELIVERED bit. Loopback reports event are reported twice, this
bit is set once event are delivered. I just came up with a better idea, reset nfcache once the events have been delivered, but I'll apply this change in the next patchset.

I noticed this with my flow module, any interface connecting to itself would create two IPCT_NEW events. I had made my own patch to your linux-2.6.11.conntrack-event-api.patch to call ip_conntrack_event_cache_init() after the events were delivered.

I was gonna send it your way but other things came up and there were other problems that I was trying to figure out that were not quite working the way I thought they should. I investigated further and think I've found the problem, but I'm not sure how to move further on it, so now it's time to ask the list:

Can anyone explain why in ip_conntrack_proto_icmp.c
we delete the conntrack of an ICMP packet as soon as all replies are seen instead of letting ip_conntrack_icmp_timeout take the conntrack like other protocol types? As far as I can see, this breaks the conntrack event API because I get the following sequence of notifications for an ICMP echo request to the host (this is with the ip_conntrack_event_cache_init() patch after notifications have been delivered so I don't get two IPCT_NEW events):

IPCT_NEW
IPCT_DESTROY
IPCT_STATUS

I think nfct->use is still held and the conntrack is never really destroyed after death_by_timeout(), so later when the reply comes the conntrack still exists in the hash and the conntrack API fires a IPCT_STATUS event after the IPCT_DESTROY event was fired, which I'm assuming is an invalid sequence of events. ;)

So, what would be the harm in the following patch to ip_conntrack_proto_icmp.c?

diff -u a/ip_conntrack_proto_icmp.c b/ip_conntrack_proto_icmp.c

ip_conntrack_proto_icmp.c 2005-06-28 16:52:46.000000000 -0500 renamed from ip_conntrack_proto_icmp.c 2005-06-28 16:52:18.000000000 -0500 +0 -3

@@ -92,15 +92,7 @@
                       const struct sk_buff *skb,
                       enum ip_conntrack_info ctinfo)
 {
-       /* Try to delete connection immediately after all replies:
-           won't actually vanish as we still have skb, and del_timer
-           means this will only run once even if count hits zero twice

Hi Harald,

This patchset introduces tons of updates for the nfnetlink, ctnetlink and the conntrack event API. I haven't attached the file since it's that big, about 100K.

You can get an incremental diff against SVN from:
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/SVN-patches/ctnetlink-ctevent-nfnetlink-update-2.6.12.patch

Please apply.

I've split this big patch above into four pieces to make it easier to understand the changes:
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/2.6.11-vs-2.6.12/

So these four patches shouldn't be applied, just they are meant to make your life easier to track the changes.

Summary of changes
------------------

o conntrack event API
- Don't kill NFC_IP_* stuff, keep it there to ensure for old iptables versions compilation.
- new file ip_conntrack_events.h that contains all event related functions to reduce pollution in ip_conntrack.h
- IPCT_DELIVERED bit. Loopback reports event are reported twice, this bit is set once event are delivered. I just came up with a better idea, reset nfcache once the events have been delivered, but I'll apply this change in the next patchset.

o nfnetlink
- kill unused list.
- kill nfnl_exlock(), not needed anymore.
- kill duplicated check: NFNL_SUBSYS_ID(type) > NFNL_SUBSYS_COUNT.
- kill unneeded initialization of subsys_table to NULL, since it's in BSS section (already set to zero).
- kill dead define CONFIG_NF_NETLINK.

o ctnetlink
- merge ctnetlink_get_mcgroups and ctnetlink_get_exp_mcgroups
- implemented NAT handlings
- kill unused ctnetlink_kill
- use __u64 id's for conntracks
- stop using NLMSG_DONE to report the end of a dump, use explicite ACK instead (NLM_F_ACK).
- fixed broken expectation timeout dumping.
- kill unused ctnetlink_exp_dump_proto
- kill ctnetlink_exp_dump: fairly small and just used once
- kill NFNL_SUBSYS_CTNETLINK_EXP, use NFNL_SUBSYS_CTNETLINK instead
- Fix expectation table dumping
- Fix expectation creation
- implemented flushing of the expect table

TODO
----

- Implement ip_conntrack_stats dumping and reset (accounting)
- Implement get conntrack and destroy (accounting)
- Kill event/dump mask based (?). Although it's unique, I think that it could be useful for weak conntrack event notification (think of just new, established and destroy event notification to reduce performance impact).

Once ip_conntrack_netlink gets fully featured and people don't report bugs for quite some time. I'll create a nf_conntrack_netlink tree.

Signature · 1 line

Pablo