Patchwork [01/17] netfilter: add struct nf_proto_net for register l4proto sysctl

login
register
mail settings
Submitter Gao feng
Date May 14, 2012, 8:52 a.m.
Message ID <1336985547-31960-2-git-send-email-gaofeng@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/158923/
State Superseded
Headers show

Comments

Gao feng - May 14, 2012, 8:52 a.m.
From: Gao feng <gaofeng@cn.fujitus.com>

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.

Acked-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Gao feng <gaofeng@cn.fujitus.com>
---
 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(-)
Pablo Neira - May 23, 2012, 10:12 a.m.
On Mon, May 14, 2012 at 04:52:11PM +0800, Gao feng wrote:
> From: Gao feng <gaofeng@cn.fujitus.com>
> 
> 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.

More comments below.

> Acked-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Gao feng <gaofeng@cn.fujitus.com>
> ---
>  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.

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gao feng - May 24, 2012, 1:35 a.m.
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@cn.fujitus.com>
>>
>> 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@xmission.com>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitus.com>
>> ---
>>  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@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
Pablo Neira - May 24, 2012, 9:58 a.m.
On Thu, May 24, 2012 at 09:35:50AM +0800, Gao feng wrote:
> 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@cn.fujitus.com>
> >>
> >> 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@xmission.com>
> >> Signed-off-by: Gao feng <gaofeng@cn.fujitus.com>
> >> ---
> >>  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.
> 
> > 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@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
--
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
Gao feng - May 24, 2012, 10:54 a.m.
于 2012年05月24日 17:58, Pablo Neira Ayuso 写道:
> On Thu, May 24, 2012 at 09:35:50AM +0800, Gao feng wrote:
>> 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@cn.fujitus.com>
>>>>
>>>> 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@xmission.com>
>>>> Signed-off-by: Gao feng <gaofeng@cn.fujitus.com>
>>>> ---
>>>>  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?

Of cause, if you prefer to impletment it in this way.
--
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
Pablo Neira - May 24, 2012, 2:38 p.m.
On Thu, May 24, 2012 at 06:54:42PM +0800, Gao feng wrote:
[...]
> >>> 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?
> 
> Of cause, if you prefer to impletment it in this way.

If this removes the .compat field that you added, then use two
init_net functions, yes.
--
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
Gao feng - May 25, 2012, 1:05 a.m.
于 2012年05月24日 22:38, Pablo Neira Ayuso 写道:
> On Thu, May 24, 2012 at 06:54:42PM +0800, Gao feng wrote:
> [...]
>>>>> 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?
>>
>> Of cause, if you prefer to impletment it in this way.
> 
> If this removes the .compat field that you added, then use two
> init_net functions, yes.

Sorry I miss something.

nf_ct_l4proto_unregister_sysctl also uses .compat to identify if we
can unregister the compat sysctl.

if we register l4proto_tcp and l4proto_tcp6 both. without .compat,
when unregister l4proto_tcp6, the compat sysctl will be unregister too.

So maybe we have to use .compat.
--
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
Pablo Neira - May 25, 2012, 2:54 a.m.
On Fri, May 25, 2012 at 09:05:34AM +0800, Gao feng wrote:
> 于 2012年05月24日 22:38, Pablo Neira Ayuso 写道:
> > On Thu, May 24, 2012 at 06:54:42PM +0800, Gao feng wrote:
> > [...]
> >>>>> 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?
> >>
> >> Of cause, if you prefer to impletment it in this way.
> > 
> > If this removes the .compat field that you added, then use two
> > init_net functions, yes.
> 
> Sorry I miss something.
> 
> nf_ct_l4proto_unregister_sysctl also uses .compat to identify if we
> can unregister the compat sysctl.
> 
> if we register l4proto_tcp and l4proto_tcp6 both. without .compat,
> when unregister l4proto_tcp6, the compat sysctl will be unregister too.
> 
> So maybe we have to use .compat.

Could you resolve this by checking pn->ctl_compat_header != NULL ?
--
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
Gao feng - May 25, 2012, 6:02 a.m.
于 2012年05月25日 10:54, Pablo Neira Ayuso 写道:
> On Fri, May 25, 2012 at 09:05:34AM +0800, Gao feng wrote:
>> 于 2012年05月24日 22:38, Pablo Neira Ayuso 写道:
>>> On Thu, May 24, 2012 at 06:54:42PM +0800, Gao feng wrote:
>>> [...]
>>>>>>> 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?
>>>>
>>>> Of cause, if you prefer to impletment it in this way.
>>>
>>> If this removes the .compat field that you added, then use two
>>> init_net functions, yes.
>>
>> Sorry I miss something.
>>
>> nf_ct_l4proto_unregister_sysctl also uses .compat to identify if we
>> can unregister the compat sysctl.
>>
>> if we register l4proto_tcp and l4proto_tcp6 both. without .compat,
>> when unregister l4proto_tcp6, the compat sysctl will be unregister too.
>>
>> So maybe we have to use .compat.
> 
> Could you resolve this by checking pn->ctl_compat_header != NULL ?

pn->ctl_table_header and ctl_compat_header is shared by l4proto_tcp and l4proto_tcp6.
if we both register l4proto_tcp and l4proto_tcp6, when unregister l4proto_tcp6
pn->ctl_compat_header must not be NULL.

--
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
Gao feng - May 26, 2012, 2:28 a.m.
于 2012年05月25日 14:02, Gao feng 写道:
> 于 2012年05月25日 10:54, Pablo Neira Ayuso 写道:
>> On Fri, May 25, 2012 at 09:05:34AM +0800, Gao feng wrote:
>>> 于 2012年05月24日 22:38, Pablo Neira Ayuso 写道:
>>>> On Thu, May 24, 2012 at 06:54:42PM +0800, Gao feng wrote:
>>>> [...]
>>>>>>>> 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?
>>>>>
>>>>> Of cause, if you prefer to impletment it in this way.
>>>>
>>>> If this removes the .compat field that you added, then use two
>>>> init_net functions, yes.
>>>
>>> Sorry I miss something.
>>>
>>> nf_ct_l4proto_unregister_sysctl also uses .compat to identify if we
>>> can unregister the compat sysctl.
>>>
>>> if we register l4proto_tcp and l4proto_tcp6 both. without .compat,
>>> when unregister l4proto_tcp6, the compat sysctl will be unregister too.
>>>
>>> So maybe we have to use .compat.
>>
>> Could you resolve this by checking pn->ctl_compat_header != NULL ?
> 
> pn->ctl_table_header and ctl_compat_header is shared by l4proto_tcp and l4proto_tcp6.
> if we both register l4proto_tcp and l4proto_tcp6, when unregister l4proto_tcp6
> pn->ctl_compat_header must not be NULL.
> 

Maybe we can resolve this by  nf_conntrack_l4proto.l3proto == AF_INET &&  pn->ctl_compat_header != NULL
Because compat sysctl is registered by AF_INET's proto only.

--
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
Pablo Neira - May 28, 2012, 9:53 a.m.
On Sat, May 26, 2012 at 10:28:33AM +0800, Gao feng wrote:
> 于 2012年05月25日 14:02, Gao feng 写道:
> > 于 2012年05月25日 10:54, Pablo Neira Ayuso 写道:
[...]
> >> Could you resolve this by checking pn->ctl_compat_header != NULL ?
> > 
> > pn->ctl_table_header and ctl_compat_header is shared by l4proto_tcp and l4proto_tcp6.
> > if we both register l4proto_tcp and l4proto_tcp6, when unregister l4proto_tcp6
> > pn->ctl_compat_header must not be NULL.
> > 
> 
> Maybe we can resolve this by  nf_conntrack_l4proto.l3proto == AF_INET &&  pn->ctl_compat_header != NULL
> Because compat sysctl is registered by AF_INET's proto only.

OK, as soon as it can remove the compat field, I prefer it.
--
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

Patch

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;
+
 	/* 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;