| Message ID | 1336985547-31960-2-git-send-email-gaofeng@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
于 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
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
于 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
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
于 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
于 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
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
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;