| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120524095859.GC13091@1984>
Date: Thu, 24 May 2012 11:58:59 +0200
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Gao feng <gaofeng@...fujitsu.com>
Cc: netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
serge.hallyn@...onical.com, ebiederm@...ssion.com,
dlezcano@...ibm.com, Gao feng <gaofeng@...fujitus.com>
Subject: Re: [PATCH 01/17] netfilter: add struct nf_proto_net for register
l4proto sysctl
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@...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@...ssion.com>
> >> Signed-off-by: Gao feng <gaofeng@...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@...r.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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists