[PATCH net-next] ipv4: ipmr: various fixes and cleanups

[PATCH net-next] ipv4: ipmr: various fixes and cleanups

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

1) ip_mroute_setsockopt() & ip_mroute_getsockopt() should not
   access/set raw_sk(sk)->ipmr_table before making sure the socket
   is a raw socket, and protocol is IGMP

2) MRT_INIT should return -EINVAL if optlen != sizeof(int), not
   -ENOPROTOOPT

3) MRT_ASSERT & MRT_PIM should validate optlen

4) " (v) ? 1 : 0 " can be written as " !!v "

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ipmr.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 6168c4d..af4ef54 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1207,6 +1207,10 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
 
+	if (sk->sk_type != SOCK_RAW ||
+	    inet_sk(sk)->inet_num != IPPROTO_IGMP)
+		return -EOPNOTSUPP;
+
 	mrt = ipmr_get_table(net, raw_sk(sk)->ipmr_table ? : RT_TABLE_DEFAULT);
 	if (mrt == NULL)
 		return -ENOENT;
@@ -1219,11 +1223,8 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 
 	switch (optname) {
 	case MRT_INIT:
-		if (sk->sk_type != SOCK_RAW ||
-		    inet_sk(sk)->inet_num != IPPROTO_IGMP)
-			return -EOPNOTSUPP;
 		if (optlen != sizeof(int))
-			return -ENOPROTOOPT;
+			return -EINVAL;
 
 		rtnl_lock();
 		if (rtnl_dereference(mrt->mroute_sk)) {
@@ -1284,9 +1285,11 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 	case MRT_ASSERT:
 	{
 		int v;
+		if (optlen != sizeof(v))
+			return -EINVAL;
 		if (get_user(v, (int __user *)optval))
 			return -EFAULT;
-		mrt->mroute_do_assert = (v) ? 1 : 0;
+		mrt->mroute_do_assert = !!v;
 		return 0;
 	}
 #ifdef CONFIG_IP_PIMSM
@@ -1294,9 +1297,11 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 	{
 		int v;
 
+		if (optlen != sizeof(v))
+			return -EINVAL;
 		if (get_user(v, (int __user *)optval))
 			return -EFAULT;
-		v = (v) ? 1 : 0;
+		v = !!v;
 
 		rtnl_lock();
 		ret = 0;
@@ -1325,7 +1330,8 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 		} else {
 			if (!ipmr_new_table(net, v))
 				ret = -ENOMEM;
-			raw_sk(sk)->ipmr_table = v;
+			else
+				raw_sk(sk)->ipmr_table = v;
 		}
 		rtnl_unlock();
 		return ret;
@@ -1351,6 +1357,10 @@ int ip_mroute_getsockopt(struct sock *sk, int optname, char __user *optval, int
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
 
+	if (sk->sk_type != SOCK_RAW ||
+	    inet_sk(sk)->inet_num != IPPROTO_IGMP)
+		return -EOPNOTSUPP;
+
 	mrt = ipmr_get_table(net, raw_sk(sk)->ipmr_table ? : RT_TABLE_DEFAULT);
 	if (mrt == NULL)
 		return -ENOENT;

Re: [PATCH net-next] ipv4: ipmr: various fixes and cleanups

[Joe Perches]

On Sun, 2012-11-25 at 08:41 -0800, Eric Dumazet wrote:
> 4) " (v) ? 1 : 0 " can be written as " !!v "

Or maybe the compiler could do it by changing
mroute_do_assert and mroute_do_pim to bool to
save a few bytes per table too.

Re: [PATCH net-next] ipv4: ipmr: various fixes and cleanups

[Eric Dumazet]

On Sun, 2012-11-25 at 10:27 -0800, Joe Perches wrote:
> On Sun, 2012-11-25 at 08:41 -0800, Eric Dumazet wrote:
> > 4) " (v) ? 1 : 0 " can be written as " !!v "
> 
> Or maybe the compiler could do it by changing
> mroute_do_assert and mroute_do_pim to bool to
> save a few bytes per table too.
> 
> 

Yeah, but mroute is probably used only by a very limited
number of machines in the world.

Please submit an additional patch, as I already put too many
things in mine.

[PATCH] ipv4/ipmr and ipv6/ip6mr: Convert int mroute_do_<foo> to bool

[Joe Perches]

Save a few bytes per table by convert mroute_do_assert and
mroute_do_pim from int to bool.

Remove !! as the compiler does that when assigning int to bool.

Signed-off-by: Joe Perches <joe@perches.com>
---
> mroute is probably used only by a very limited
> number of machines in the world.
> 
> Please submit an additional patch, as I already put too many
> things in mine.

On top of Eric's cleanup...

 net/ipv4/ipmr.c  |    6 +++---
 net/ipv6/ip6mr.c |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 0394a8e..fc09ef9 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -83,8 +83,8 @@ struct mr_table {
 	struct vif_device	vif_table[MAXVIFS];
 	int			maxvif;
 	atomic_t		cache_resolve_queue_len;
-	int			mroute_do_assert;
-	int			mroute_do_pim;
+	bool			mroute_do_assert;
+	bool			mroute_do_pim;
 #if defined(CONFIG_IP_PIMSM_V1) || defined(CONFIG_IP_PIMSM_V2)
 	int			mroute_reg_vif_num;
 #endif
@@ -1289,7 +1289,7 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 			return -EINVAL;
 		if (get_user(v, (int __user *)optval))
 			return -EFAULT;
-		mrt->mroute_do_assert = !!v;
+		mrt->mroute_do_assert = v;
 		return 0;
 	}
 #ifdef CONFIG_IP_PIMSM
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index d7330f8..79bb490 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -66,8 +66,8 @@ struct mr6_table {
 	struct mif_device	vif6_table[MAXMIFS];
 	int			maxvif;
 	atomic_t		cache_resolve_queue_len;
-	int			mroute_do_assert;
-	int			mroute_do_pim;
+	bool			mroute_do_assert;
+	bool			mroute_do_pim;
 #ifdef CONFIG_IPV6_PIMSM_V2
 	int			mroute_reg_vif_num;
 #endif
@@ -1648,7 +1648,7 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, uns
 		int v;
 		if (get_user(v, (int __user *)optval))
 			return -EFAULT;
-		mrt->mroute_do_assert = !!v;
+		mrt->mroute_do_assert = v;
 		return 0;
 	}
 

Re: [PATCH net-next] ipv4: ipmr: various fixes and cleanups

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 25 Nov 2012 08:41:45 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> 1) ip_mroute_setsockopt() & ip_mroute_getsockopt() should not
>    access/set raw_sk(sk)->ipmr_table before making sure the socket
>    is a raw socket, and protocol is IGMP
> 
> 2) MRT_INIT should return -EINVAL if optlen != sizeof(int), not
>    -ENOPROTOOPT
> 
> 3) MRT_ASSERT & MRT_PIM should validate optlen
> 
> 4) " (v) ? 1 : 0 " can be written as " !!v "
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

Re: [PATCH] ipv4/ipmr and ipv6/ip6mr: Convert int mroute_do_<foo> to bool

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sun, 25 Nov 2012 11:35:30 -0800

> Save a few bytes per table by convert mroute_do_assert and
> mroute_do_pim from int to bool.
> 
> Remove !! as the compiler does that when assigning int to bool.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Applied.

[PATCH] ip6mr: Add sizeof verification to MRT6_ASSERT and MT6_PIM

[Joe Perches]

Verify the length of the user-space arguments.

Signed-off-by: Joe Perches <joe@perches.com>
---
 net/ipv6/ip6mr.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 79bb490..926ea54 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1646,6 +1646,9 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, uns
 	case MRT6_ASSERT:
 	{
 		int v;
+
+		if (optlen != sizeof(v))
+			return -EINVAL;
 		if (get_user(v, (int __user *)optval))
 			return -EFAULT;
 		mrt->mroute_do_assert = v;
@@ -1656,6 +1659,9 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, uns
 	case MRT6_PIM:
 	{
 		int v;
+
+		if (optlen != sizeof(v))
+			return -EINVAL;
 		if (get_user(v, (int __user *)optval))
 			return -EFAULT;
 		v = !!v;

Re: [PATCH] ip6mr: Add sizeof verification to MRT6_ASSERT and MT6_PIM

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sun, 25 Nov 2012 20:26:34 -0800

> Verify the length of the user-space arguments.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Applied to net-next, thanks.