On Thu, May 24, 2012 at 09:35:50AM +0800, Gao feng wrote:

Hi pablo:

于 2012年05月23日 18:12, Pablo Neira Ayuso 写道:

quoted

On Mon, May 14, 2012 at 04:52:11PM +0800, Gao feng wrote:

quoted

From: Gao feng <redacted>

the struct nf_proto_net stroes proto's ctl_table_header and ctl_table,
nf_ct_l4proto_(un)register_sysctl use it to register sysctl.

there are some changes for struct nf_conntrack_l4proto:
- add field compat to identify if this proto should do compat.
- the net_id field is used to store the pernet_operations id
  that belones to l4proto.
- init_net will be used to initial the proto's pernet data

and add init_net for struct nf_conntrack_l3proto too.

This patchset looks bette but there are still things that we have to
resolve.

The first one (regarding this patch 1/17) changes in:
* include/net/netfilter/nf_conntrack_l4proto.h
* include/net/netns/conntrack.h

should be included in:
[PATCH] netfilter: add namespace support for l4proto

And changes in:
* include/net/netfilter/nf_conntrack_l3proto.h

should be included in:
[PATCH] netfilter: add namespace support for l3proto

I already told you. A patch that adds a structure without using it,
is not good. The structure has to go together with the code uses it.

It seams this patch should be merged to "netfilter: add namespace support for l4proto"
the struct nf_proto_net is first used there.

quoted

More comments below.

quoted

Acked-by: Eric W. Biederman <redacted>
Signed-off-by: Gao feng <redacted>
---
 include/net/netfilter/nf_conntrack_l3proto.h |    3 +++
 include/net/netfilter/nf_conntrack_l4proto.h |    6 ++++++
 include/net/netns/conntrack.h                |   12 ++++++++++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
index 9699c02..9766005 100644
--- a/include/net/netfilter/nf_conntrack_l3proto.h
+++ b/include/net/netfilter/nf_conntrack_l3proto.h

@@ -69,6 +69,9 @@ struct nf_conntrack_l3proto {
 	struct ctl_table	*ctl_table;
 #endif /* CONFIG_SYSCTL */
 
+	/* Init l3proto pernet data */
+	int (*init_net)(struct net *net);
+
 	/* Module (if any) which this is connected to. */
 	struct module *me;
 };

diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
index 3b572bb..a90eab5 100644
--- a/include/net/netfilter/nf_conntrack_l4proto.h
+++ b/include/net/netfilter/nf_conntrack_l4proto.h

@@ -22,6 +22,8 @@ struct nf_conntrack_l4proto {
 	/* L4 Protocol number. */
 	u_int8_t l4proto;
 
+	u_int8_t compat;

I don't see why we need this new field.

It seems to be set to 1 in each structure that has set:

.ctl_compat_table

to non-NULL. So, it's redundant.

Moreover, you already know from the protocol tracker itself if you
have to allocate the compat ctl table or not.

In other words: You set compat to 1 for nf_conntrack_l4proto_generic.
Then, you pass that compat value to generic_init_net via ->inet_net
again, but this information (that determines if the compat has to be
done or not) is already in the scope of the protocol tracker.

because some protocols such l4proto_tcp6 and l4proto_tcp use the same init_net
function. the l4proto_tcp6 doesn't need compat sysctl, so we should use this new
field to identify if we should kmemdup compat_sysctl_table.

Then, could you use two init_net functions? one for TCP for IPv4 and another
for TCP for IPv6?

and beacuse protocols will have pernet ctl_compat_table and ctl_table,the .ctl_compat_table
field will be deleted in patch 15/17. so we should the new field compat.

actually, we don't need to pass compat value for generic_init_net,beacuse
we know l4proto_generic need compat. But consider there are l4proto_tcp(6), and in order to keep
code readable,I prefer to add compat field and pass it to init_net.

quoted

You have to fix this.

quoted

+
 	/* Try to fill in the third arg: dataoff is offset past network protocol
            hdr.  Return true if possible. */
 	bool (*pkt_to_tuple)(const struct sk_buff *skb, unsigned int dataoff,

@@ -103,6 +105,10 @@ struct nf_conntrack_l4proto {
 	struct ctl_table	*ctl_compat_table;
 #endif
 #endif
+	int	*net_id;
+	/* Init l4proto pernet data */
+	int (*init_net)(struct net *net, u_int8_t compat);
+
 	/* Protocol name */
 	const char *name;
 

diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
index a053a19..1f53038 100644
--- a/include/net/netns/conntrack.h
+++ b/include/net/netns/conntrack.h

@@ -8,6 +8,18 @@
 struct ctl_table_header;
 struct nf_conntrack_ecache;
 
+struct nf_proto_net {
+#ifdef CONFIG_SYSCTL
+	struct ctl_table_header *ctl_table_header;
+	struct ctl_table        *ctl_table;
+#ifdef CONFIG_NF_CONNTRACK_PROC_COMPAT
+	struct ctl_table_header *ctl_compat_header;
+	struct ctl_table        *ctl_compat_table;
+#endif
+#endif
+	unsigned int		users;
+};
+
 struct netns_ct {
 	atomic_t		count;
 	unsigned int		expect_count;

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html