Hi pablo:
于 2012年05月23日 18:12, Pablo Neira Ayuso 写道:
> On Mon, May 14, 2012 at 04:52:11PM +0800, Gao feng wrote:
>> From: Gao feng <gaofeng@xxxxxxxxxxxxxx>
>>
>> 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.
> More comments below.
>
>> Acked-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
>> Signed-off-by: Gao feng <gaofeng@xxxxxxxxxxxxxx>
>> ---
>> 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.
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.
> You have to fix this.
>
>> +
>> /* 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
