Skip to contentSkip to conversation

Implement private protocol info changing

PATCH In review Pablo Neira 3 participants 3 messages Aug 25, 2005 – Sep 5, 2005

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 3/3 Implement private protocol info changing

This patch add support to change the state of the private protocol information via conntrack_netlink.

Signed-off-by Pablo Neira Ayuso <[email protected]>
net/ipv4/netfilter/ip_conntrack_netlink.c +42 -0
Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
@@ -948,6 +948,36 @@
 	return 0;
 }
 
+static inline int
+ctnetlink_change_protoinfo(struct ip_conntrack *ct, struct nfattr *cda[])
+{
+	struct nfattr *tb[CTA_PROTOINFO_MAX], *attr = cda[CTA_PROTOINFO-1];
+	struct ip_conntrack_protocol *proto;
+	u_int16_t npt = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum;
+	int err;
+
+	if (nfattr_parse_nested(tb, CTA_PROTOINFO_MAX, attr) < 0)
+		goto nfattr_failure;
+
+	proto = ip_conntrack_proto_find_get(npt);
+	if (!proto)
+		return -EINVAL;
+
+	if (proto->from_nfattr) {
+		err = proto->from_nfattr(tb, ct);
+		if (err < 0) {
+			ip_conntrack_proto_put(proto);
+			return -EINVAL;
+		}
+	}
+	ip_conntrack_proto_put(proto);
+
+	return 0;
+
+nfattr_failure:
+	return -1;
+}
+
 static int
 ctnetlink_change_conntrack(struct ip_conntrack *ct, struct nfattr *cda[])
 {

@@ -973,6 +1003,12 @@
 			return err;
 	}
 
+	if (cda[CTA_PROTOINFO-1]) {
+		err = ctnetlink_change_protoinfo(ct, cda);
+		if (err < 0)
+			return err;
+	}
+
 	DEBUGP("all done\n");
 	return 0;
 }

@@ -1002,6 +1038,12 @@
 	if (err < 0)
 		goto err;
 
+	if (cda[CTA_PROTOINFO-1]) {
+		err = ctnetlink_change_protoinfo(ct, cda);
+		if (err < 0)
+			return err;
+	}
+
 	ct->helper = ip_conntrack_helper_find_get(rtuple);
 
 	add_timer(&ct->timeout);
#1 lore raw
Quoted 1 line · Pablo Neira

This patch add support to change the state of the private protocol information via conntrack_netlink.

The patches look fine to me, besides the two issues mentioned below. I have a couple of fixes which conflict with them though, please resend after they hit Linus's tree.

Quoted 10 lines

------------------------------------------------------------------------

Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================

net/ipv4/netfilter/ip_conntrack_netlink.c +0 -0
@@ -948,6 +948,36 @@
 	return 0;
 }

net/ipv4/netfilter/ip_conntrack_netlink.c line 960

Quoted 10 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +10 -0
+static inline int
+ctnetlink_change_protoinfo(struct ip_conntrack *ct, struct nfattr *cda[])
+{
+	struct nfattr *tb[CTA_PROTOINFO_MAX], *attr = cda[CTA_PROTOINFO-1];
+	struct ip_conntrack_protocol *proto;
+	u_int16_t npt = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum;
+	int err;
+
+	if (nfattr_parse_nested(tb, CTA_PROTOINFO_MAX, attr) < 0)
+		goto nfattr_failure;

nfattr_failure is usually used when there isn't enough space in the skb, not when parsing. So I would prefer to only use it in functions putting data in the skb to avoid confusion. Also -1 (-EPERM) isn't a suitable error to return to userspace.

Quoted 5 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +5 -0
+
+	proto = ip_conntrack_proto_find_get(npt);
+	if (!proto)
+		return -EINVAL;
+

net/ipv4/netfilter/ip_conntrack_netlink.c line 975

Quoted 10 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +10 -0
+	if (proto->from_nfattr) {
+		err = proto->from_nfattr(tb, ct);
+		if (err < 0) {
+			ip_conntrack_proto_put(proto);
+			return -EINVAL;
+		}
+	}
+	ip_conntrack_proto_put(proto);
+
+	return 0;

This would be simpler and would propagate the error properly if you would write it like this:

...
if (proto->from_nfattr)

	err = proto->from_nfattr(tb, ct);

ip_conntrack_proto_put(proto);

return err;

#2 lore raw
Quoted 7 lines · Patrick McHardy
Quoted 2 lines · Pablo Neira

This patch add support to change the state of the private protocol information via conntrack_netlink.

The patches look fine to me, besides the two issues mentioned below. I have a couple of fixes which conflict with them though, please resend after they hit Linus's tree.

I also wasn't yet sure whether the current nesting is the best possible solution. Sorry, was too busy with non-netfilter stuff recently so I couldn't comment on that. I'll try to comment on it later today.

Signature · 5 lines
- Harald Welte <[email protected]>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie