[PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Alban Crequy]

With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
in new protocols.

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 net/netfilter/core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e1b7e05..4f16552 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
 	struct nf_hook_ops *elem;
 	int err;
 
+	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
+		BUG();
+		return 1;
+	}
+
 	err = mutex_lock_interruptible(&nf_hook_mutex);
 	if (err < 0)
 		return err;
-- 
1.7.2.5

[PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[Alban Crequy]

NFPROTO_* constants were usually equal to PF_* constants but it is not
necessary and it will waste less memory if we don't do so (see commit 7e9c6e
"netfilter: Introduce NFPROTO_* constants")

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 net/decnet/netfilter/dn_rtmsg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c
index 1531135..7fb7250 100644
--- a/net/decnet/netfilter/dn_rtmsg.c
+++ b/net/decnet/netfilter/dn_rtmsg.c
@@ -118,7 +118,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb)
 
 static struct nf_hook_ops dnrmg_ops __read_mostly = {
 	.hook		= dnrmg_hook,
-	.pf		= PF_DECnet,
+	.pf		= NFPROTO_DECNET,
 	.hooknum	= NF_DN_ROUTE,
 	.priority	= NF_DN_PRI_DNRTMSG,
 };
-- 
1.7.2.5

[PATCH 3/6] netfilter: bridge: switch hook PFs to nfproto

[Alban Crequy]

NFPROTO_* constants were usually equal to PF_* constants but it is not
necessary and it will waste less memory if we don't do so (see commit 7e9c6e
"netfilter: Introduce NFPROTO_* constants")

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 net/bridge/br_netfilter.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index d7f49b6..b8ba7d6 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -749,9 +749,9 @@ static unsigned int br_nf_forward_ip(unsigned int hook, struct sk_buff *skb,
 		return NF_DROP;
 
 	if (IS_IP(skb) || IS_VLAN_IP(skb) || IS_PPPOE_IP(skb))
-		pf = PF_INET;
+		pf = NFPROTO_IPV4;
 	else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb))
-		pf = PF_INET6;
+		pf = NFPROTO_IPV6;
 	else
 		return NF_ACCEPT;
 
@@ -763,13 +763,13 @@ static unsigned int br_nf_forward_ip(unsigned int hook, struct sk_buff *skb,
 		nf_bridge->mask |= BRNF_PKT_TYPE;
 	}
 
-	if (pf == PF_INET && br_parse_ip_options(skb))
+	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
 		return NF_DROP;
 
 	/* The physdev module checks on this */
 	nf_bridge->mask |= BRNF_BRIDGED;
 	nf_bridge->physoutdev = skb->dev;
-	if (pf == PF_INET)
+	if (pf == NFPROTO_IPV4)
 		skb->protocol = htons(ETH_P_IP);
 	else
 		skb->protocol = htons(ETH_P_IPV6);
@@ -856,9 +856,9 @@ static unsigned int br_nf_post_routing(unsigned int hook, struct sk_buff *skb,
 		return NF_DROP;
 
 	if (IS_IP(skb) || IS_VLAN_IP(skb) || IS_PPPOE_IP(skb))
-		pf = PF_INET;
+		pf = NFPROTO_IPV4;
 	else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb))
-		pf = PF_INET6;
+		pf = NFPROTO_IPV6;
 	else
 		return NF_ACCEPT;
 
@@ -871,7 +871,7 @@ static unsigned int br_nf_post_routing(unsigned int hook, struct sk_buff *skb,
 
 	nf_bridge_pull_encap_header(skb);
 	nf_bridge_save_header(skb);
-	if (pf == PF_INET)
+	if (pf == NFPROTO_IPV4)
 		skb->protocol = htons(ETH_P_IP);
 	else
 		skb->protocol = htons(ETH_P_IPV6);
@@ -904,49 +904,49 @@ static struct nf_hook_ops br_nf_ops[] __read_mostly = {
 	{
 		.hook = br_nf_pre_routing,
 		.owner = THIS_MODULE,
-		.pf = PF_BRIDGE,
+		.pf = NFPROTO_BRIDGE,
 		.hooknum = NF_BR_PRE_ROUTING,
 		.priority = NF_BR_PRI_BRNF,
 	},
 	{
 		.hook = br_nf_local_in,
 		.owner = THIS_MODULE,
-		.pf = PF_BRIDGE,
+		.pf = NFPROTO_BRIDGE,
 		.hooknum = NF_BR_LOCAL_IN,
 		.priority = NF_BR_PRI_BRNF,
 	},
 	{
 		.hook = br_nf_forward_ip,
 		.owner = THIS_MODULE,
-		.pf = PF_BRIDGE,
+		.pf = NFPROTO_BRIDGE,
 		.hooknum = NF_BR_FORWARD,
 		.priority = NF_BR_PRI_BRNF - 1,
 	},
 	{
 		.hook = br_nf_forward_arp,
 		.owner = THIS_MODULE,
-		.pf = PF_BRIDGE,
+		.pf = NFPROTO_BRIDGE,
 		.hooknum = NF_BR_FORWARD,
 		.priority = NF_BR_PRI_BRNF,
 	},
 	{
 		.hook = br_nf_post_routing,
 		.owner = THIS_MODULE,
-		.pf = PF_BRIDGE,
+		.pf = NFPROTO_BRIDGE,
 		.hooknum = NF_BR_POST_ROUTING,
 		.priority = NF_BR_PRI_LAST,
 	},
 	{
 		.hook = ip_sabotage_in,
 		.owner = THIS_MODULE,
-		.pf = PF_INET,
+		.pf = NFPROTO_IPV4,
 		.hooknum = NF_INET_PRE_ROUTING,
 		.priority = NF_IP_PRI_FIRST,
 	},
 	{
 		.hook = ip_sabotage_in,
 		.owner = THIS_MODULE,
-		.pf = PF_INET6,
+		.pf = NFPROTO_IPV6,
 		.hooknum = NF_INET_PRE_ROUTING,
 		.priority = NF_IP6_PRI_FIRST,
 	},
-- 
1.7.2.5

[PATCH 4/6] netfilter: ipv4, defrag: switch hook PFs to nfproto

[Alban Crequy]

NFPROTO_* constants were usually equal to PF_* constants but it is not
necessary and it will waste less memory if we don't do so (see commit 7e9c6e
"netfilter: Introduce NFPROTO_* constants")

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 net/ipv4/netfilter/nf_defrag_ipv4.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/netfilter/nf_defrag_ipv4.c b/net/ipv4/netfilter/nf_defrag_ipv4.c
index 9bb1b8a..7428155 100644
--- a/net/ipv4/netfilter/nf_defrag_ipv4.c
+++ b/net/ipv4/netfilter/nf_defrag_ipv4.c
@@ -94,14 +94,14 @@ static struct nf_hook_ops ipv4_defrag_ops[] = {
 	{
 		.hook		= ipv4_conntrack_defrag,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_PRE_ROUTING,
 		.priority	= NF_IP_PRI_CONNTRACK_DEFRAG,
 	},
 	{
 		.hook           = ipv4_conntrack_defrag,
 		.owner          = THIS_MODULE,
-		.pf             = PF_INET,
+		.pf             = NFPROTO_IPV4,
 		.hooknum        = NF_INET_LOCAL_OUT,
 		.priority       = NF_IP_PRI_CONNTRACK_DEFRAG,
 	},
-- 
1.7.2.5

[PATCH 5/6] netfilter: ipvs: switch hook PFs to nfproto

[Alban Crequy]

NFPROTO_* constants were usually equal to PF_* constants but it is not
necessary and it will waste less memory if we don't do so (see commit 7e9c6e
"netfilter: Introduce NFPROTO_* constants")

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 net/netfilter/ipvs/ip_vs_core.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 00bdb1d..d1c6a77 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1768,7 +1768,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_reply4,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP_PRI_NAT_SRC - 2,
 	},
@@ -1778,7 +1778,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_remote_request4,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP_PRI_NAT_SRC - 1,
 	},
@@ -1786,7 +1786,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_local_reply4,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP_PRI_NAT_DST + 1,
 	},
@@ -1794,7 +1794,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_local_request4,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP_PRI_NAT_DST + 2,
 	},
@@ -1803,7 +1803,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_forward_icmp,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_FORWARD,
 		.priority	= 99,
 	},
@@ -1811,7 +1811,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_reply4,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_FORWARD,
 		.priority	= 100,
 	},
@@ -1820,7 +1820,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_reply6,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET6,
+		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP6_PRI_NAT_SRC - 2,
 	},
@@ -1830,7 +1830,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_remote_request6,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET6,
+		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_LOCAL_IN,
 		.priority	= NF_IP6_PRI_NAT_SRC - 1,
 	},
@@ -1838,7 +1838,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_local_reply6,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET,
+		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP6_PRI_NAT_DST + 1,
 	},
@@ -1846,7 +1846,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_local_request6,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET6,
+		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_LOCAL_OUT,
 		.priority	= NF_IP6_PRI_NAT_DST + 2,
 	},
@@ -1855,7 +1855,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_forward_icmp_v6,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET6,
+		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_FORWARD,
 		.priority	= 99,
 	},
@@ -1863,7 +1863,7 @@ static struct nf_hook_ops ip_vs_ops[] __read_mostly = {
 	{
 		.hook		= ip_vs_reply6,
 		.owner		= THIS_MODULE,
-		.pf		= PF_INET6,
+		.pf		= NFPROTO_IPV6,
 		.hooknum	= NF_INET_FORWARD,
 		.priority	= 100,
 	},
-- 
1.7.2.5

[PATCH 6/6] netfilter: selinux: switch hook PFs to nfproto

[Alban Crequy]

NFPROTO_* constants were usually equal to PF_* constants but it is not
necessary and it will waste less memory if we don't do so (see commit 7e9c6e
"netfilter: Introduce NFPROTO_* constants")

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 security/selinux/hooks.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d85b793..1ab4d6b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5776,21 +5776,21 @@ static struct nf_hook_ops selinux_ipv4_ops[] = {
 	{
 		.hook =		selinux_ipv4_postroute,
 		.owner =	THIS_MODULE,
-		.pf =		PF_INET,
+		.pf =		NFPROTO_IPV4,
 		.hooknum =	NF_INET_POST_ROUTING,
 		.priority =	NF_IP_PRI_SELINUX_LAST,
 	},
 	{
 		.hook =		selinux_ipv4_forward,
 		.owner =	THIS_MODULE,
-		.pf =		PF_INET,
+		.pf =		NFPROTO_IPV4,
 		.hooknum =	NF_INET_FORWARD,
 		.priority =	NF_IP_PRI_SELINUX_FIRST,
 	},
 	{
 		.hook =		selinux_ipv4_output,
 		.owner =	THIS_MODULE,
-		.pf =		PF_INET,
+		.pf =		NFPROTO_IPV4,
 		.hooknum =	NF_INET_LOCAL_OUT,
 		.priority =	NF_IP_PRI_SELINUX_FIRST,
 	}
@@ -5802,14 +5802,14 @@ static struct nf_hook_ops selinux_ipv6_ops[] = {
 	{
 		.hook =		selinux_ipv6_postroute,
 		.owner =	THIS_MODULE,
-		.pf =		PF_INET6,
+		.pf =		NFPROTO_IPV6,
 		.hooknum =	NF_INET_POST_ROUTING,
 		.priority =	NF_IP6_PRI_SELINUX_LAST,
 	},
 	{
 		.hook =		selinux_ipv6_forward,
 		.owner =	THIS_MODULE,
-		.pf =		PF_INET6,
+		.pf =		NFPROTO_IPV6,
 		.hooknum =	NF_INET_FORWARD,
 		.priority =	NF_IP6_PRI_SELINUX_FIRST,
 	}
-- 
1.7.2.5

RE: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[David Laight]

 
> NFPROTO_* constants were usually equal to PF_* constants but it is not
> necessary and it will waste less memory if we don't do so 
> (see commit 7e9c6e
> "netfilter: Introduce NFPROTO_* constants")
...
>  
>  static struct nf_hook_ops dnrmg_ops __read_mostly = {
>  	.hook		= dnrmg_hook,
> -	.pf		= PF_DECnet,
> +	.pf		= NFPROTO_DECNET,
>  	.hooknum	= NF_DN_ROUTE,
>  	.priority	= NF_DN_PRI_DNRTMSG,
>  };

Might it be worth renaming the .pf member to (say) .nfproto
to help avoid confusion?

	David

Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[Florian Westphal]

David Laight <David.Laight@ACULAB.COM> wrote:
>  
> > NFPROTO_* constants were usually equal to PF_* constants but it is not
> > necessary and it will waste less memory if we don't do so 
> > (see commit 7e9c6e
> > "netfilter: Introduce NFPROTO_* constants")
> ...
> >  
> >  static struct nf_hook_ops dnrmg_ops __read_mostly = {
> >  	.hook		= dnrmg_hook,
> > -	.pf		= PF_DECnet,
> > +	.pf		= NFPROTO_DECNET,
> >  	.hooknum	= NF_DN_ROUTE,
> >  	.priority	= NF_DN_PRI_DNRTMSG,
> >  };
> 
> Might it be worth renaming the .pf member to (say) .nfproto
> to help avoid confusion?

NFPROTO_* values are exported to userspace, so I don't think
its safe to change these values.

Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 03:18:16PM +0100, David Laight wrote:
>  
> > NFPROTO_* constants were usually equal to PF_* constants but it is not
> > necessary and it will waste less memory if we don't do so 
> > (see commit 7e9c6e
> > "netfilter: Introduce NFPROTO_* constants")
> ...
> >  
> >  static struct nf_hook_ops dnrmg_ops __read_mostly = {
> >  	.hook		= dnrmg_hook,
> > -	.pf		= PF_DECnet,
> > +	.pf		= NFPROTO_DECNET,
> >  	.hooknum	= NF_DN_ROUTE,
> >  	.priority	= NF_DN_PRI_DNRTMSG,
> >  };
> 
> Might it be worth renaming the .pf member to (say) .nfproto
> to help avoid confusion?

That can be done follow-up patch, I guess.

Re: [PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
> NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
> in new protocols.
> 
> Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
> ---
>  net/netfilter/core.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index e1b7e05..4f16552 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
>  	struct nf_hook_ops *elem;
>  	int err;
>  
> +	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
> +		BUG();
> +		return 1;

nf_register_hook returns a negative value on error. -EINVAL can be
fine.

> +	}
> +
>  	err = mutex_lock_interruptible(&nf_hook_mutex);
>  	if (err < 0)
>  		return err;
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[Pablo Neira Ayuso]

@Jan,

I remember you introduced all this NFPROTO_* thing time ago.

Any complain on this patchset?

Thanks.

On Mon, May 14, 2012 at 02:56:35PM +0100, Alban Crequy wrote:
> NFPROTO_* constants were usually equal to PF_* constants but it is not
> necessary and it will waste less memory if we don't do so (see commit 7e9c6e
> "netfilter: Introduce NFPROTO_* constants")
> 
> Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
> ---
>  net/decnet/netfilter/dn_rtmsg.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c
> index 1531135..7fb7250 100644
> --- a/net/decnet/netfilter/dn_rtmsg.c
> +++ b/net/decnet/netfilter/dn_rtmsg.c
> @@ -118,7 +118,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb)
>  
>  static struct nf_hook_ops dnrmg_ops __read_mostly = {
>  	.hook		= dnrmg_hook,
> -	.pf		= PF_DECnet,
> +	.pf		= NFPROTO_DECNET,
>  	.hooknum	= NF_DN_ROUTE,
>  	.priority	= NF_DN_PRI_DNRTMSG,
>  };
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Jan Engelhardt]

On Monday 2012-05-14 15:56, Alban Crequy wrote:

>With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
>NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
>in new protocols.

>index e1b7e05..4f16552 100644
>--- a/net/netfilter/core.c
>+++ b/net/netfilter/core.c
>@@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> 	struct nf_hook_ops *elem;
> 	int err;
> 
>+	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
>+		BUG();
>+		return 1;
>+	}

Like always, I'd prefer a WARN() instead, here paired with return -EINVAL.
Especially when the error path is (seems) simple, halting the entire machine
does not look very nice.

RE: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[Jan Engelhardt]

On Monday 2012-05-14 16:18, David Laight wrote:

> 
>> NFPROTO_* constants were usually equal to PF_* constants but it is not
>> necessary and it will waste less memory if we don't do so 
>> (see commit 7e9c6e
>> "netfilter: Introduce NFPROTO_* constants")
>...
>>  
>>  static struct nf_hook_ops dnrmg_ops __read_mostly = {
>>  	.hook		= dnrmg_hook,
>> -	.pf		= PF_DECnet,
>> +	.pf		= NFPROTO_DECNET,
>>  	.hooknum	= NF_DN_ROUTE,
>>  	.priority	= NF_DN_PRI_DNRTMSG,
>>  };
>
>Might it be worth renaming the .pf member to (say) .nfproto
>to help avoid confusion?

Yes that seems like a sensible thing.

Re: [PATCH 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Alban Crequy]

Le Mon, 14 May 2012 16:42:35 +0200,
Pablo Neira Ayuso <pablo@netfilter.org> a écrit :

> On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> > With the NFPROTO_* constants introduced by commit 7e9c6e
> > ("netfilter: Introduce NFPROTO_* constants"), it is too easy to
> > confuse PF_* and NFPROTO_* constants in new protocols.
> > 
> > Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> > Reviewed-by: Javier Martinez Canillas
> > <javier.martinez@collabora.co.uk> Reviewed-by: Vincent Sanders
> > <vincent.sanders@collabora.co.uk> ---
> >  net/netfilter/core.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> > index e1b7e05..4f16552 100644
> > --- a/net/netfilter/core.c
> > +++ b/net/netfilter/core.c
> > @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> >  	struct nf_hook_ops *elem;
> >  	int err;
> >  
> > +	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >=
> > NF_MAX_HOOKS) {
> > +		BUG();
> > +		return 1;
> 
> nf_register_hook returns a negative value on error. -EINVAL can be
> fine.

Is it the patch you mean? Do you want me to do a series repost?


From: Alban Crequy <alban.crequy@collabora.co.uk>

netfilter: sanity checks on NFPROTO_NUMPROTO

With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
in new protocols.

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Vincent Sanders <vincent.sanders@collabora.co.uk>
---
 net/netfilter/core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e1b7e05..ac56c5b 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
 	struct nf_hook_ops *elem;
 	int err;
 
+	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
+		WARN();
+		return -EINVAL;
+	}
+
 	err = mutex_lock_interruptible(&nf_hook_mutex);
 	if (err < 0)
 		return err;
-- 
1.7.2.5
--
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 v2 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Alban Crequy]

Le Mon, 14 May 2012 16:39:49 +0100,
Alban Crequy <alban.crequy@collabora.co.uk> a écrit :

> Le Mon, 14 May 2012 16:42:35 +0200,
> Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
> 
> > On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> > > With the NFPROTO_* constants introduced by commit 7e9c6e
> > > ("netfilter: Introduce NFPROTO_* constants"), it is too easy to
> > > confuse PF_* and NFPROTO_* constants in new protocols.
> > > 
> > > Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> > > Reviewed-by: Javier Martinez Canillas
> > > <javier.martinez@collabora.co.uk> Reviewed-by: Vincent Sanders
> > > <vincent.sanders@collabora.co.uk> ---
> > >  net/netfilter/core.c |    5 +++++
> > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> > > index e1b7e05..4f16552 100644
> > > --- a/net/netfilter/core.c
> > > +++ b/net/netfilter/core.c
> > > @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> > >  	struct nf_hook_ops *elem;
> > >  	int err;
> > >  
> > > +	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >=
> > > NF_MAX_HOOKS) {
> > > +		BUG();
> > > +		return 1;
> > 
> > nf_register_hook returns a negative value on error. -EINVAL can be
> > fine.
> 
> Is it the patch you mean? Do you want me to do a series repost?

Please disregard the previous patch, this is the correct one.


From: Alban Crequy <alban.crequy@collabora.co.uk>

netfilter: sanity checks on NFPROTO_NUMPROTO

With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
in new protocols.

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
---
 net/netfilter/core.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e1b7e05..7422989 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -67,6 +67,14 @@ int nf_register_hook(struct nf_hook_ops *reg)
 	struct nf_hook_ops *elem;
 	int err;
 
+	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
+		WARN(reg->pf >= NFPROTO_NUMPROTO,
+		     "netfilter: Invalid nfproto %d\n", reg->pf);
+		WARN(reg->hooknum >= NF_MAX_HOOKS,
+		     "netfilter: Invalid hooknum %d\n", reg->hooknum);
+		return -EINVAL;
+	}
+
 	err = mutex_lock_interruptible(&nf_hook_mutex);
 	if (err < 0)
 		return err;
-- 
1.7.2.5

--
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

Re: [PATCH v2 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 05:04:10PM +0100, Alban Crequy wrote:
> Le Mon, 14 May 2012 16:39:49 +0100,
> Alban Crequy <alban.crequy@collabora.co.uk> a écrit :
> 
> > Le Mon, 14 May 2012 16:42:35 +0200,
> > Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
> > 
> > > On Mon, May 14, 2012 at 02:56:34PM +0100, Alban Crequy wrote:
> > > > With the NFPROTO_* constants introduced by commit 7e9c6e
> > > > ("netfilter: Introduce NFPROTO_* constants"), it is too easy to
> > > > confuse PF_* and NFPROTO_* constants in new protocols.
> > > > 
> > > > Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> > > > Reviewed-by: Javier Martinez Canillas
> > > > <javier.martinez@collabora.co.uk> Reviewed-by: Vincent Sanders
> > > > <vincent.sanders@collabora.co.uk> ---
> > > >  net/netfilter/core.c |    5 +++++
> > > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> > > > index e1b7e05..4f16552 100644
> > > > --- a/net/netfilter/core.c
> > > > +++ b/net/netfilter/core.c
> > > > @@ -67,6 +67,11 @@ int nf_register_hook(struct nf_hook_ops *reg)
> > > >  	struct nf_hook_ops *elem;
> > > >  	int err;
> > > >  
> > > > +	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >=
> > > > NF_MAX_HOOKS) {
> > > > +		BUG();
> > > > +		return 1;
> > > 
> > > nf_register_hook returns a negative value on error. -EINVAL can be
> > > fine.
> > 
> > Is it the patch you mean? Do you want me to do a series repost?
> 
> Please disregard the previous patch, this is the correct one.
> 
> 
> From: Alban Crequy <alban.crequy@collabora.co.uk>
> 
> netfilter: sanity checks on NFPROTO_NUMPROTO
> 
> With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
> NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
> in new protocols.
> 
> Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
> ---
>  net/netfilter/core.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index e1b7e05..7422989 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -67,6 +67,14 @@ int nf_register_hook(struct nf_hook_ops *reg)
>  	struct nf_hook_ops *elem;
>  	int err;
>  
> +	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS) {
> +		WARN(reg->pf >= NFPROTO_NUMPROTO,
> +		     "netfilter: Invalid nfproto %d\n", reg->pf);
> +		WARN(reg->hooknum >= NF_MAX_HOOKS,
> +		     "netfilter: Invalid hooknum %d\n", reg->hooknum);

Then, better add two checkings. One to spot the first warning, and
another to spot the second.

I havent seen such a code in any netfilter code and I like that things
remain consistent.

> +		return -EINVAL;
> +	}
> +
>  	err = mutex_lock_interruptible(&nf_hook_mutex);
>  	if (err < 0)
>  		return err;
> -- 
> 1.7.2.5
> 

[PATCH v3 1/6] netfilter: sanity checks on NFPROTO_NUMPROTO

[Alban Crequy]

With the NFPROTO_* constants introduced by commit 7e9c6e ("netfilter: Introduce
NFPROTO_* constants"), it is too easy to confuse PF_* and NFPROTO_* constants
in new protocols.

Signed-off-by: Alban Crequy <alban.crequy@collabora.co.uk>
---
v2:
 - use WARN
 - return -EINVAL
v3:
 - two checkings

 net/netfilter/core.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e1b7e05..448b531 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -67,6 +67,16 @@ int nf_register_hook(struct nf_hook_ops *reg)
 	struct nf_hook_ops *elem;
 	int err;
 
+	if (reg->pf >= NFPROTO_NUMPROTO) {
+		WARN(1, "netfilter: Invalid nfproto %d\n", reg->pf);
+		return -EINVAL;
+	}
+
+	if (reg->hooknum >= NF_MAX_HOOKS) {
+		WARN(1, "netfilter: Invalid hooknum %d\n", reg->hooknum);
+		return -EINVAL;
+	}
+
 	err = mutex_lock_interruptible(&nf_hook_mutex);
 	if (err < 0)
 		return err;
-- 
1.7.2.5

Re: [PATCH 2/6] netfilter: decnet: switch hook PFs to nfproto

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 02:56:35PM +0100, Alban Crequy wrote:
> NFPROTO_* constants were usually equal to PF_* constants but it is not
> necessary and it will waste less memory if we don't do so (see commit 7e9c6e
> "netfilter: Introduce NFPROTO_* constants")

Applied, thanks.

But I rewrote the description to just say that this is a consistency
cleanup (other hooks use NFPROTO_*).

Hm, by grepping net/netfilter for PF_* still some netfilter subsystems
(like ipset) show up using it.

Perhaps we can take another patch for these.

Re: [PATCH 3/6] netfilter: bridge: switch hook PFs to nfproto

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 02:56:36PM +0100, Alban Crequy wrote:
> NFPROTO_* constants were usually equal to PF_* constants but it is not
> necessary and it will waste less memory if we don't do so (see commit 7e9c6e
> "netfilter: Introduce NFPROTO_* constants")

Applied, thanks.

Re: [PATCH 4/6] netfilter: ipv4, defrag: switch hook PFs to nfproto

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 02:56:37PM +0100, Alban Crequy wrote:
> NFPROTO_* constants were usually equal to PF_* constants but it is not
> necessary and it will waste less memory if we don't do so (see commit 7e9c6e
> "netfilter: Introduce NFPROTO_* constants")

Applied (with that description rewrite as said), thanks.

Re: [PATCH 5/6] netfilter: ipvs: switch hook PFs to nfproto

[Pablo Neira Ayuso]

Applied, thanks.

Re: [PATCH 6/6] netfilter: selinux: switch hook PFs to nfproto

[Pablo Neira Ayuso]

And finally this applied as well.