netdev - Re: [PATCH 01/17] netfilter: add struct nf_proto

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120523101200.GA2836@1984>
Date:	Wed, 23 May 2012 12:12:00 +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 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.

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.

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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html