Patchwork net: Add compat ioctl support for the ipv4 multicast ioctl SIOCGETSGCNT

login
register
mail settings
Submitter Eric W. Biederman
Date Jan. 30, 2011, 2:15 a.m.
Message ID <m18vy3uof7.fsf@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/80994/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - Jan. 30, 2011, 2:15 a.m.
SIOCGETSGCNT is not a unique ioctl value as it it maps tio SIOCPROTOPRIVATE +1,
which unfortunately means the existing infrastructure for compat networking
ioctls is insufficient.  A trivial compact ioctl implementation would conflict
with:

SIOCAX25ADDUID
SIOCAIPXPRISLT
SIOCGETSGCNT_IN6
SIOCGETSGCNT
SIOCRSSCAUSE
SIOCX25SSUBSCRIP
SIOCX25SDTEFACILITIES

To make this work I have updated the compat_ioctl decode path to mirror the
the normal ioctl decode path.  I have added an ipv4 inet_compat_ioctl function
so that I can have ipv4 specific compat ioctls.   I have added a compat_ioctl
function into struct proto so I can break out ioctls by which kind of ip socket
I am using.  I have added a compat_raw_ioctl function because SIOCGETSGCNT only
works on raw sockets.  I have added a ipmr_compat_ioctl that mirrors the normal
ipmr_ioctl.

This was necessary because unfortunately the struct layout for the SIOCGETSGCNT
has unsigned longs in it so changes between 32bit and 64bit kernels.

This change was sufficient to run a 32bit ip multicast routing daemon on a
64bit kernel.

Reported-by: Bill Fenner <fenner@aristanetworks.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/mroute.h |    1 +
 include/net/sock.h     |    2 ++
 net/ipv4/af_inet.c     |   16 ++++++++++++++++
 net/ipv4/ipmr.c        |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/raw.c         |   19 +++++++++++++++++++
 5 files changed, 84 insertions(+), 0 deletions(-)
David Miller - Jan. 30, 2011, 9:14 a.m.
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sat, 29 Jan 2011 18:15:56 -0800

> 
> SIOCGETSGCNT is not a unique ioctl value as it it maps tio SIOCPROTOPRIVATE +1,
> which unfortunately means the existing infrastructure for compat networking
> ioctls is insufficient.  A trivial compact ioctl implementation would conflict
> with:
> 
> SIOCAX25ADDUID
> SIOCAIPXPRISLT
> SIOCGETSGCNT_IN6
> SIOCGETSGCNT
> SIOCRSSCAUSE
> SIOCX25SSUBSCRIP
> SIOCX25SDTEFACILITIES
> 
> To make this work I have updated the compat_ioctl decode path to mirror the
> the normal ioctl decode path.  I have added an ipv4 inet_compat_ioctl function
> so that I can have ipv4 specific compat ioctls.   I have added a compat_ioctl
> function into struct proto so I can break out ioctls by which kind of ip socket
> I am using.  I have added a compat_raw_ioctl function because SIOCGETSGCNT only
> works on raw sockets.  I have added a ipmr_compat_ioctl that mirrors the normal
> ipmr_ioctl.
> 
> This was necessary because unfortunately the struct layout for the SIOCGETSGCNT
> has unsigned longs in it so changes between 32bit and 64bit kernels.
> 
> This change was sufficient to run a 32bit ip multicast routing daemon on a
> 64bit kernel.
> 
> Reported-by: Bill Fenner <fenner@aristanetworks.com>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

Thanks a lot for doing this work Eric, applied to net-2.6.
--
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
Arnd Bergmann - Jan. 30, 2011, 12:26 p.m.
On Sunday 30 January 2011 03:15:56 Eric W. Biederman wrote:
> A trivial compact ioctl implementation would conflict with:
> SIOCAX25ADDUID
> SIOCAIPXPRISLT
> SIOCGETSGCNT_IN6
> SIOCGETSGCNT
> SIOCRSSCAUSE
> SIOCX25SSUBSCRIP
> SIOCX25SDTEFACILITIES

Since you have compiled the list, did you see if these are all handled
compatible, or would it make sense to create patches for the other
protocols as well, to handle them individually?

> This was necessary because unfortunately the struct layout for the SIOCGETSGCNT
> has unsigned longs in it so changes between 32bit and 64bit kernels.
> 
> This change was sufficient to run a 32bit ip multicast routing daemon on a
> 64bit kernel.
> 
> Reported-by: Bill Fenner <fenner@aristanetworks.com>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

Looks good,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> +#ifdef CONFIG_COMPAT
> +static int compat_raw_ioctl(struct sock *sk, unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case SIOCOUTQ:
> +	case SIOCINQ:
> +		return -ENOIOCTLCMD;

I would have suggested doing

	return raw_ioctl(sk, cmd, (unsigned long)compat_ptr(arg));

here, but returning -ENOIOCTLCMD is equivalent and correct. Your solution
is slightly more compact, the other one would be slightly faster.

	Arnd
--
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
Eric W. Biederman - Jan. 30, 2011, 6:15 p.m.
Arnd Bergmann <arnd@arndb.de> writes:

> On Sunday 30 January 2011 03:15:56 Eric W. Biederman wrote:
>> A trivial compact ioctl implementation would conflict with:
>> SIOCAX25ADDUID
>> SIOCAIPXPRISLT
>> SIOCGETSGCNT_IN6
>> SIOCGETSGCNT
>> SIOCRSSCAUSE
>> SIOCX25SSUBSCRIP
>> SIOCX25SDTEFACILITIES
>
> Since you have compiled the list, did you see if these are all handled
> compatible, or would it make sense to create patches for the other
> protocols as well, to handle them individually?

I didn't look.  I had a specific pre-existing application that didn't
work, and those ioctls meant the existing solutions for network compat
ioctls wouldn't work and the infrastructure needed fixing.

>> +#ifdef CONFIG_COMPAT
>> +static int compat_raw_ioctl(struct sock *sk, unsigned int cmd, unsigned long arg)
>> +{
>> +	switch (cmd) {
>> +	case SIOCOUTQ:
>> +	case SIOCINQ:
>> +		return -ENOIOCTLCMD;
>
> I would have suggested doing
>
> 	return raw_ioctl(sk, cmd, (unsigned long)compat_ptr(arg));
>
> here, but returning -ENOIOCTLCMD is equivalent and correct. Your solution
> is slightly more compact, the other one would be slightly faster.

Correct and maintainable is fine by me.

Right now my network stack performance problems are almost all rtnl_lock
hold time problems.

Eric
--
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
Arnd Bergmann - Jan. 30, 2011, 6:59 p.m.
On Sunday 30 January 2011 19:15:40 Eric W. Biederman wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > On Sunday 30 January 2011 03:15:56 Eric W. Biederman wrote:
> >> A trivial compact ioctl implementation would conflict with:
> >> SIOCAX25ADDUID
> >> SIOCAIPXPRISLT
> >> SIOCGETSGCNT_IN6
> >> SIOCGETSGCNT
> >> SIOCRSSCAUSE
> >> SIOCX25SSUBSCRIP
> >> SIOCX25SDTEFACILITIES
> >
> > Since you have compiled the list, did you see if these are all handled
> > compatible, or would it make sense to create patches for the other
> > protocols as well, to handle them individually?
> 
> I didn't look.  I had a specific pre-existing application that didn't
> work, and those ioctls meant the existing solutions for network compat
> ioctls wouldn't work and the infrastructure needed fixing.

I just looked at all users of SIOCPROTOPRIVATE to check what else
is needed here, for reference. What I found is:

* appletalk, ipx and x25 have full compat_ioctl support.
* ax25 needs some real work in order to be usable in compat mode.
* phonet and rose use only data structures that are compatible,
  so adding support would be trivial.
* ip multicast actually needs support for SIOCGETVIFCNT in
  addition to SIOCGETSGCNT to be complete.
* ipv6 multicast needs the same patch as ipv4 multicast for
  SIOCGETMIFCNT_IN6/SIOCGETSGCNT_IN6.

It would probably be a good idea if someone could complete the
work on ipv4/v6 multicast compat_ioctl, on top of your patch.

	Arnd
--
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
David Miller - Feb. 4, 2011, 1:19 a.m.
From: Arnd Bergmann <arnd@arndb.de>
Date: Sun, 30 Jan 2011 19:59:53 +0100

> * ip multicast actually needs support for SIOCGETVIFCNT in
>   addition to SIOCGETSGCNT to be complete.
> * ipv6 multicast needs the same patch as ipv4 multicast for
>   SIOCGETMIFCNT_IN6/SIOCGETSGCNT_IN6.
> 
> It would probably be a good idea if someone could complete the
> work on ipv4/v6 multicast compat_ioctl, on top of your patch.

Actually, on top of this, Eric's patch is buggy.

He defines the "struct compat_sioc_sg_req" but doesn't actually
use it.

I'll fix that, then take care of the missing cases.  Thanks Arnd.
--
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
Eric W. Biederman - Feb. 4, 2011, 2:07 a.m.
David Miller <davem@davemloft.net> writes:

> From: Arnd Bergmann <arnd@arndb.de>
> Date: Sun, 30 Jan 2011 19:59:53 +0100
>
>> * ip multicast actually needs support for SIOCGETVIFCNT in
>>   addition to SIOCGETSGCNT to be complete.
>> * ipv6 multicast needs the same patch as ipv4 multicast for
>>   SIOCGETMIFCNT_IN6/SIOCGETSGCNT_IN6.
>> 
>> It would probably be a good idea if someone could complete the
>> work on ipv4/v6 multicast compat_ioctl, on top of your patch.
>
> Actually, on top of this, Eric's patch is buggy.

Ouch.  Thanks for catching that.

> He defines the "struct compat_sioc_sg_req" but doesn't actually
> use it.
>
> I'll fix that, then take care of the missing cases.  Thanks Arnd.

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

Patch

diff --git a/include/linux/mroute.h b/include/linux/mroute.h
index 0fa7a3a..b21d567 100644
--- a/include/linux/mroute.h
+++ b/include/linux/mroute.h
@@ -150,6 +150,7 @@  static inline int ip_mroute_opt(int opt)
 extern int ip_mroute_setsockopt(struct sock *, int, char __user *, unsigned int);
 extern int ip_mroute_getsockopt(struct sock *, int, char __user *, int __user *);
 extern int ipmr_ioctl(struct sock *sk, int cmd, void __user *arg);
+extern int ipmr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg);
 extern int ip_mr_init(void);
 #else
 static inline
diff --git a/include/net/sock.h b/include/net/sock.h
index 73a4f97..221212e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -734,6 +734,8 @@  struct proto {
 					int level,
 					int optname, char __user *optval,
 					int __user *option);
+	int			(*compat_ioctl)(struct sock *sk,
+					unsigned int cmd, unsigned long arg);
 #endif
 	int			(*sendmsg)(struct kiocb *iocb, struct sock *sk,
 					   struct msghdr *msg, size_t len);
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index f581f77..8bda7c9 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -880,6 +880,19 @@  int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 }
 EXPORT_SYMBOL(inet_ioctl);
 
+#ifdef CONFIG_COMPAT
+int inet_compat_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
+{
+	struct sock *sk = sock->sk;
+	int err = -ENOIOCTLCMD;
+
+	if (sk->sk_prot->compat_ioctl)
+		err = sk->sk_prot->compat_ioctl(sk, cmd, arg);
+
+	return err;
+}
+#endif
+
 const struct proto_ops inet_stream_ops = {
 	.family		   = PF_INET,
 	.owner		   = THIS_MODULE,
@@ -903,6 +916,7 @@  const struct proto_ops inet_stream_ops = {
 #ifdef CONFIG_COMPAT
 	.compat_setsockopt = compat_sock_common_setsockopt,
 	.compat_getsockopt = compat_sock_common_getsockopt,
+	.compat_ioctl	   = inet_compat_ioctl,
 #endif
 };
 EXPORT_SYMBOL(inet_stream_ops);
@@ -929,6 +943,7 @@  const struct proto_ops inet_dgram_ops = {
 #ifdef CONFIG_COMPAT
 	.compat_setsockopt = compat_sock_common_setsockopt,
 	.compat_getsockopt = compat_sock_common_getsockopt,
+	.compat_ioctl	   = inet_compat_ioctl,
 #endif
 };
 EXPORT_SYMBOL(inet_dgram_ops);
@@ -959,6 +974,7 @@  static const struct proto_ops inet_sockraw_ops = {
 #ifdef CONFIG_COMPAT
 	.compat_setsockopt = compat_sock_common_setsockopt,
 	.compat_getsockopt = compat_sock_common_getsockopt,
+	.compat_ioctl	   = inet_compat_ioctl,
 #endif
 };
 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 86dd569..b294da7 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -60,6 +60,7 @@ 
 #include <linux/notifier.h>
 #include <linux/if_arp.h>
 #include <linux/netfilter_ipv4.h>
+#include <linux/compat.h>
 #include <net/ipip.h>
 #include <net/checksum.h>
 #include <net/netlink.h>
@@ -1434,6 +1435,51 @@  int ipmr_ioctl(struct sock *sk, int cmd, void __user *arg)
 	}
 }
 
+#ifdef CONFIG_COMPAT
+struct compat_sioc_sg_req {
+	struct in_addr src;
+	struct in_addr grp;
+	compat_ulong_t pktcnt;
+	compat_ulong_t bytecnt;
+	compat_ulong_t wrong_if;
+};
+
+int ipmr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
+{
+	struct sioc_sg_req sr;
+	struct mfc_cache *c;
+	struct net *net = sock_net(sk);
+	struct mr_table *mrt;
+
+	mrt = ipmr_get_table(net, raw_sk(sk)->ipmr_table ? : RT_TABLE_DEFAULT);
+	if (mrt == NULL)
+		return -ENOENT;
+
+	switch (cmd) {
+	case SIOCGETSGCNT:
+		if (copy_from_user(&sr, arg, sizeof(sr)))
+			return -EFAULT;
+
+		rcu_read_lock();
+		c = ipmr_cache_find(mrt, sr.src.s_addr, sr.grp.s_addr);
+		if (c) {
+			sr.pktcnt = c->mfc_un.res.pkt;
+			sr.bytecnt = c->mfc_un.res.bytes;
+			sr.wrong_if = c->mfc_un.res.wrong_if;
+			rcu_read_unlock();
+
+			if (copy_to_user(arg, &sr, sizeof(sr)))
+				return -EFAULT;
+			return 0;
+		}
+		rcu_read_unlock();
+		return -EADDRNOTAVAIL;
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+#endif
+
 
 static int ipmr_device_event(struct notifier_block *this, unsigned long event, void *ptr)
 {
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 1f85ef2..f5967b0 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -76,6 +76,7 @@ 
 #include <linux/seq_file.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv4.h>
+#include <linux/compat.h>
 
 static struct raw_hashinfo raw_v4_hashinfo = {
 	.lock = __RW_LOCK_UNLOCKED(raw_v4_hashinfo.lock),
@@ -839,6 +840,23 @@  static int raw_ioctl(struct sock *sk, int cmd, unsigned long arg)
 	}
 }
 
+#ifdef CONFIG_COMPAT
+static int compat_raw_ioctl(struct sock *sk, unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	case SIOCOUTQ:
+	case SIOCINQ:
+		return -ENOIOCTLCMD;
+	default:
+#ifdef CONFIG_IP_MROUTE
+		return ipmr_compat_ioctl(sk, cmd, compat_ptr(arg));
+#else
+		return -ENOIOCTLCMD;
+#endif	
+	}
+}
+#endif
+
 struct proto raw_prot = {
 	.name		   = "RAW",
 	.owner		   = THIS_MODULE,
@@ -861,6 +879,7 @@  struct proto raw_prot = {
 #ifdef CONFIG_COMPAT
 	.compat_setsockopt = compat_raw_setsockopt,
 	.compat_getsockopt = compat_raw_getsockopt,
+	.compat_ioctl	   = compat_raw_ioctl,
 #endif
 };