All of lore.kernel.org
 help / color / mirror / Atom feed
* use symbol_get to create the magic ipv4/ipv6 tunnels
@ 2020-05-14 14:50 Christoph Hellwig
  2020-05-14 14:50 ` [PATCH 1/4] ipv4: streamline ipmr_new_tunnel Christoph Hellwig
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-14 14:50 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel

Hi Dave,

both the ipv4 and ipv6 code have an ioctl each that can be used to create
a tunnel using code that doesn't live in the core kernel or ipv6 module.
Currently they call ioctls on the tunnel devices to create these, for
which the code needs to override the address limit, which is a "feature"
I plan to get rid of.

Instead this patchset makes the ipip and sit modules export a function
that can be used to create the tunnels, and then uses symbol_get in the
core ipv4/ipv6 code to reference that function at runtime.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/4] ipv4: streamline ipmr_new_tunnel
  2020-05-14 14:50 use symbol_get to create the magic ipv4/ipv6 tunnels Christoph Hellwig
@ 2020-05-14 14:50 ` Christoph Hellwig
  2020-05-14 14:50 ` [PATCH 2/4] ipv4: consolidate the VIFF_TUNNEL handling in ipmr_new_tunnel Christoph Hellwig
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-14 14:50 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel

Reduce a few level of indentation to simplify the function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 net/ipv4/ipmr.c | 73 ++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 9cf83cc85e4ad..84541c601cfab 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -469,50 +469,49 @@ static bool ipmr_init_vif_indev(const struct net_device *dev)
 
 static struct net_device *ipmr_new_tunnel(struct net *net, struct vifctl *v)
 {
-	struct net_device  *dev;
+	mm_segment_t oldfs = get_fs();
+	struct net_device *tunnel_dev, *new_dev;
+	struct ip_tunnel_parm p = { };
+	struct ifreq ifr;
+	int err;
 
-	dev = __dev_get_by_name(net, "tunl0");
+	tunnel_dev = __dev_get_by_name(net, "tunl0");
+	if (!tunnel_dev)
+		goto out;
 
-	if (dev) {
-		const struct net_device_ops *ops = dev->netdev_ops;
-		int err;
-		struct ifreq ifr;
-		struct ip_tunnel_parm p;
+	p.iph.daddr = v->vifc_rmt_addr.s_addr;
+	p.iph.saddr = v->vifc_lcl_addr.s_addr;
+	p.iph.version = 4;
+	p.iph.ihl = 5;
+	p.iph.protocol = IPPROTO_IPIP;
+	sprintf(p.name, "dvmrp%d", v->vifc_vifi);
+	ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
 
-		memset(&p, 0, sizeof(p));
-		p.iph.daddr = v->vifc_rmt_addr.s_addr;
-		p.iph.saddr = v->vifc_lcl_addr.s_addr;
-		p.iph.version = 4;
-		p.iph.ihl = 5;
-		p.iph.protocol = IPPROTO_IPIP;
-		sprintf(p.name, "dvmrp%d", v->vifc_vifi);
-		ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
+	if (!tunnel_dev->netdev_ops->ndo_do_ioctl)
+		goto out;
 
-		if (ops->ndo_do_ioctl) {
-			mm_segment_t oldfs = get_fs();
+	set_fs(KERNEL_DS);
+	err = tunnel_dev->netdev_ops->ndo_do_ioctl(tunnel_dev, &ifr,
+			SIOCADDTUNNEL);
+	set_fs(oldfs);
+	if (err)
+		goto out;
 
-			set_fs(KERNEL_DS);
-			err = ops->ndo_do_ioctl(dev, &ifr, SIOCADDTUNNEL);
-			set_fs(oldfs);
-		} else {
-			err = -EOPNOTSUPP;
-		}
-		dev = NULL;
+	new_dev = __dev_get_by_name(net, p.name);
+	if (!new_dev)
+		goto out;
 
-		if (err == 0 &&
-		    (dev = __dev_get_by_name(net, p.name)) != NULL) {
-			dev->flags |= IFF_MULTICAST;
-			if (!ipmr_init_vif_indev(dev))
-				goto failure;
-			if (dev_open(dev, NULL))
-				goto failure;
-			dev_hold(dev);
-		}
-	}
-	return dev;
+	new_dev->flags |= IFF_MULTICAST;
+	if (!ipmr_init_vif_indev(new_dev))
+		goto out_unregister;
+	if (dev_open(new_dev, NULL))
+		goto out_unregister;
+	dev_hold(new_dev);
+	return new_dev;
 
-failure:
-	unregister_netdevice(dev);
+out_unregister:
+	unregister_netdevice(new_dev);
+out:
 	return NULL;
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] ipv4: consolidate the VIFF_TUNNEL handling in ipmr_new_tunnel
  2020-05-14 14:50 use symbol_get to create the magic ipv4/ipv6 tunnels Christoph Hellwig
  2020-05-14 14:50 ` [PATCH 1/4] ipv4: streamline ipmr_new_tunnel Christoph Hellwig
@ 2020-05-14 14:50 ` Christoph Hellwig
  2020-05-14 14:51 ` [PATCH 3/4] ipv4: use symbol_get to access ipip symbols Christoph Hellwig
  2020-05-14 14:51 ` [PATCH 4/4] ipv6: symbol_get to access a sit symbol Christoph Hellwig
  3 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-14 14:50 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel

Also move the dev_set_allmulti call and the error handling into the
ioctl helper.  This allows reusing already looked up tunnel_dev pointer
and the set up argument structure for the deletion in the error handler.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 net/ipv4/ipmr.c | 53 ++++++++++++-------------------------------------
 1 file changed, 13 insertions(+), 40 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 84541c601cfab..6bf2a88abe86e 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -419,37 +419,6 @@ static void ipmr_free_table(struct mr_table *mrt)
 
 /* Service routines creating virtual interfaces: DVMRP tunnels and PIMREG */
 
-static void ipmr_del_tunnel(struct net_device *dev, struct vifctl *v)
-{
-	struct net *net = dev_net(dev);
-
-	dev_close(dev);
-
-	dev = __dev_get_by_name(net, "tunl0");
-	if (dev) {
-		const struct net_device_ops *ops = dev->netdev_ops;
-		struct ifreq ifr;
-		struct ip_tunnel_parm p;
-
-		memset(&p, 0, sizeof(p));
-		p.iph.daddr = v->vifc_rmt_addr.s_addr;
-		p.iph.saddr = v->vifc_lcl_addr.s_addr;
-		p.iph.version = 4;
-		p.iph.ihl = 5;
-		p.iph.protocol = IPPROTO_IPIP;
-		sprintf(p.name, "dvmrp%d", v->vifc_vifi);
-		ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
-
-		if (ops->ndo_do_ioctl) {
-			mm_segment_t oldfs = get_fs();
-
-			set_fs(KERNEL_DS);
-			ops->ndo_do_ioctl(dev, &ifr, SIOCDELTUNNEL);
-			set_fs(oldfs);
-		}
-	}
-}
-
 /* Initialize ipmr pimreg/tunnel in_device */
 static bool ipmr_init_vif_indev(const struct net_device *dev)
 {
@@ -507,12 +476,22 @@ static struct net_device *ipmr_new_tunnel(struct net *net, struct vifctl *v)
 	if (dev_open(new_dev, NULL))
 		goto out_unregister;
 	dev_hold(new_dev);
+	err = dev_set_allmulti(new_dev, 1);
+	if (err) {
+		dev_close(new_dev);
+		set_fs(KERNEL_DS);
+		tunnel_dev->netdev_ops->ndo_do_ioctl(tunnel_dev, &ifr,
+				SIOCDELTUNNEL);
+		set_fs(oldfs);
+		dev_put(new_dev);
+		new_dev = ERR_PTR(err);
+	}
 	return new_dev;
 
 out_unregister:
 	unregister_netdevice(new_dev);
 out:
-	return NULL;
+	return ERR_PTR(-ENOBUFS);
 }
 
 #if defined(CONFIG_IP_PIMSM_V1) || defined(CONFIG_IP_PIMSM_V2)
@@ -864,14 +843,8 @@ static int vif_add(struct net *net, struct mr_table *mrt,
 		break;
 	case VIFF_TUNNEL:
 		dev = ipmr_new_tunnel(net, vifc);
-		if (!dev)
-			return -ENOBUFS;
-		err = dev_set_allmulti(dev, 1);
-		if (err) {
-			ipmr_del_tunnel(dev, vifc);
-			dev_put(dev);
-			return err;
-		}
+		if (IS_ERR(dev))
+			return PTR_ERR(dev);
 		break;
 	case VIFF_USE_IFINDEX:
 	case 0:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] ipv4: use symbol_get to access ipip symbols
  2020-05-14 14:50 use symbol_get to create the magic ipv4/ipv6 tunnels Christoph Hellwig
  2020-05-14 14:50 ` [PATCH 1/4] ipv4: streamline ipmr_new_tunnel Christoph Hellwig
  2020-05-14 14:50 ` [PATCH 2/4] ipv4: consolidate the VIFF_TUNNEL handling in ipmr_new_tunnel Christoph Hellwig
@ 2020-05-14 14:51 ` Christoph Hellwig
  2020-05-14 14:51 ` [PATCH 4/4] ipv6: symbol_get to access a sit symbol Christoph Hellwig
  3 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-14 14:51 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel

Instead of going through the ioctl handler from kernel space, use
symbol_get to access the ip_tunnel_ioctl directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 net/ipv4/ipmr.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 6bf2a88abe86e..3780ab694c574 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -438,10 +438,9 @@ static bool ipmr_init_vif_indev(const struct net_device *dev)
 
 static struct net_device *ipmr_new_tunnel(struct net *net, struct vifctl *v)
 {
-	mm_segment_t oldfs = get_fs();
+	int (*tunnel_ctl)(struct net_device *, struct ip_tunnel_parm *, int);
 	struct net_device *tunnel_dev, *new_dev;
 	struct ip_tunnel_parm p = { };
-	struct ifreq ifr;
 	int err;
 
 	tunnel_dev = __dev_get_by_name(net, "tunl0");
@@ -454,21 +453,17 @@ static struct net_device *ipmr_new_tunnel(struct net *net, struct vifctl *v)
 	p.iph.ihl = 5;
 	p.iph.protocol = IPPROTO_IPIP;
 	sprintf(p.name, "dvmrp%d", v->vifc_vifi);
-	ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
 
-	if (!tunnel_dev->netdev_ops->ndo_do_ioctl)
-		goto out;
+	tunnel_ctl = symbol_get(ip_tunnel_ioctl);
+	if (!tunnel_ctl)
+		return ERR_PTR(-ENOBUFS);
 
-	set_fs(KERNEL_DS);
-	err = tunnel_dev->netdev_ops->ndo_do_ioctl(tunnel_dev, &ifr,
-			SIOCADDTUNNEL);
-	set_fs(oldfs);
-	if (err)
-		goto out;
+	if (tunnel_ctl(tunnel_dev, &p, SIOCADDTUNNEL))
+		goto out_symbol_put;
 
 	new_dev = __dev_get_by_name(net, p.name);
 	if (!new_dev)
-		goto out;
+		goto out_symbol_put;
 
 	new_dev->flags |= IFF_MULTICAST;
 	if (!ipmr_init_vif_indev(new_dev))
@@ -479,17 +474,18 @@ static struct net_device *ipmr_new_tunnel(struct net *net, struct vifctl *v)
 	err = dev_set_allmulti(new_dev, 1);
 	if (err) {
 		dev_close(new_dev);
-		set_fs(KERNEL_DS);
-		tunnel_dev->netdev_ops->ndo_do_ioctl(tunnel_dev, &ifr,
-				SIOCDELTUNNEL);
-		set_fs(oldfs);
+		tunnel_ctl(tunnel_dev, &p, SIOCDELTUNNEL);
 		dev_put(new_dev);
 		new_dev = ERR_PTR(err);
 	}
+
+	symbol_put(ip_tunnel_ioctl);
 	return new_dev;
 
 out_unregister:
 	unregister_netdevice(new_dev);
+out_symbol_put:
+	symbol_put(ipmr_new_tunnel);
 out:
 	return ERR_PTR(-ENOBUFS);
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] ipv6: symbol_get to access a sit symbol
  2020-05-14 14:50 use symbol_get to create the magic ipv4/ipv6 tunnels Christoph Hellwig
                   ` (2 preceding siblings ...)
  2020-05-14 14:51 ` [PATCH 3/4] ipv4: use symbol_get to access ipip symbols Christoph Hellwig
@ 2020-05-14 14:51 ` Christoph Hellwig
  2020-05-15  0:53   ` David Miller
  3 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-14 14:51 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel

Instead of going through the ioctl handler from kernel space, use
symbol_get to the newly factored out ipip6_set_dstaddr helper, bypassing
addrconf.c entirely.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/net/addrconf.h |  1 -
 include/net/ipv6.h     |  2 ++
 net/ipv6/addrconf.c    | 66 ------------------------------------------
 net/ipv6/af_inet6.c    | 20 ++++++++++++-
 net/ipv6/sit.c         | 41 ++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 68 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index fdb07105384ca..569eb03ae2440 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -76,7 +76,6 @@ void addrconf_cleanup(void);
 
 int addrconf_add_ifaddr(struct net *net, void __user *arg);
 int addrconf_del_ifaddr(struct net *net, void __user *arg);
-int addrconf_set_dstaddr(struct net *net, void __user *arg);
 
 int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 		  const struct net_device *dev, int strict);
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 955badd1e8ffc..1b983f32c87ce 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1080,6 +1080,8 @@ struct in6_addr *fl6_update_dst(struct flowi6 *fl6,
 				const struct ipv6_txoptions *opt,
 				struct in6_addr *orig);
 
+int ipip6_set_dstaddr(struct net *net, void __user *arg);
+
 /*
  *	socket options (ipv6_sockglue.c)
  */
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index fd885f06c4ed6..02186f00f91c5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2783,72 +2783,6 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 	in6_dev_put(in6_dev);
 }
 
-/*
- *	Set destination address.
- *	Special case for SIT interfaces where we create a new "virtual"
- *	device.
- */
-int addrconf_set_dstaddr(struct net *net, void __user *arg)
-{
-	struct in6_ifreq ireq;
-	struct net_device *dev;
-	int err = -EINVAL;
-
-	rtnl_lock();
-
-	err = -EFAULT;
-	if (copy_from_user(&ireq, arg, sizeof(struct in6_ifreq)))
-		goto err_exit;
-
-	dev = __dev_get_by_index(net, ireq.ifr6_ifindex);
-
-	err = -ENODEV;
-	if (!dev)
-		goto err_exit;
-
-#if IS_ENABLED(CONFIG_IPV6_SIT)
-	if (dev->type == ARPHRD_SIT) {
-		const struct net_device_ops *ops = dev->netdev_ops;
-		struct ifreq ifr;
-		struct ip_tunnel_parm p;
-
-		err = -EADDRNOTAVAIL;
-		if (!(ipv6_addr_type(&ireq.ifr6_addr) & IPV6_ADDR_COMPATv4))
-			goto err_exit;
-
-		memset(&p, 0, sizeof(p));
-		p.iph.daddr = ireq.ifr6_addr.s6_addr32[3];
-		p.iph.saddr = 0;
-		p.iph.version = 4;
-		p.iph.ihl = 5;
-		p.iph.protocol = IPPROTO_IPV6;
-		p.iph.ttl = 64;
-		ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
-
-		if (ops->ndo_do_ioctl) {
-			mm_segment_t oldfs = get_fs();
-
-			set_fs(KERNEL_DS);
-			err = ops->ndo_do_ioctl(dev, &ifr, SIOCADDTUNNEL);
-			set_fs(oldfs);
-		} else
-			err = -EOPNOTSUPP;
-
-		if (err == 0) {
-			err = -ENOBUFS;
-			dev = __dev_get_by_name(net, p.name);
-			if (!dev)
-				goto err_exit;
-			err = dev_open(dev, NULL);
-		}
-	}
-#endif
-
-err_exit:
-	rtnl_unlock();
-	return err;
-}
-
 static int ipv6_mc_config(struct sock *sk, bool join,
 			  const struct in6_addr *addr, int ifindex)
 {
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 345baa0a754f4..3ec9734c7bb11 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -538,6 +538,19 @@ int inet6_getname(struct socket *sock, struct sockaddr *uaddr,
 }
 EXPORT_SYMBOL(inet6_getname);
 
+static int inet6_ioctl_set_dstaddr(struct net *net, void __user *arg)
+{
+	int (*set_dstaddr)(struct net *, void __user *);
+	int err;
+
+	set_dstaddr = symbol_get(ipip6_set_dstaddr);
+	if (!set_dstaddr)
+		return -EOPNOTSUPP;
+	err = set_dstaddr(net, arg);
+	symbol_put(ipip6_set_dstaddr);
+	return err;
+}
+
 int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 {
 	struct sock *sk = sock->sk;
@@ -554,7 +567,12 @@ int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 	case SIOCDIFADDR:
 		return addrconf_del_ifaddr(net, (void __user *) arg);
 	case SIOCSIFDSTADDR:
-		return addrconf_set_dstaddr(net, (void __user *) arg);
+		/* Special case for SIT interfaces where we create a new
+		 * "virtual" device.
+		 */
+		if (!IS_ENABLED(CONFIG_IPV6_SIT))
+			return -ENODEV;
+		return inet6_ioctl_set_dstaddr(net, (void __user *) arg);
 	default:
 		if (!sk->sk_prot->ioctl)
 			return -ENOIOCTLCMD;
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 98954830c40ba..cb2cfa297f72e 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -274,6 +274,47 @@ static struct ip_tunnel *ipip6_tunnel_locate(struct net *net,
 	return NULL;
 }
 
+int ipip6_set_dstaddr(struct net *net, void __user *arg)
+{
+	struct ip_tunnel_parm p = { };
+	struct in6_ifreq ireq;
+	struct net_device *tunnel_dev, *new_dev;
+	int err;
+
+	rtnl_lock();
+	err = -EFAULT;
+	if (copy_from_user(&ireq, arg, sizeof(struct in6_ifreq)))
+		goto out_unlock;
+
+	err = -ENODEV;
+	tunnel_dev = __dev_get_by_index(net, ireq.ifr6_ifindex);
+	if (!tunnel_dev || tunnel_dev->type != ARPHRD_SIT)
+		goto out_unlock;
+
+	err = -EADDRNOTAVAIL;
+	if (!(ipv6_addr_type(&ireq.ifr6_addr) & IPV6_ADDR_COMPATv4))
+		goto out_unlock;
+
+	p.iph.daddr = ireq.ifr6_addr.s6_addr32[3];
+	p.iph.version = 4;
+	p.iph.ihl = 5;
+	p.iph.protocol = IPPROTO_IPV6;
+	p.iph.ttl = 64;
+	p.iph.frag_off |= htons(IP_DF);
+
+	err = -ENOBUFS;
+	if (!ipip6_tunnel_locate(dev_net(tunnel_dev), &p, true))
+		goto out_unlock;
+	new_dev = __dev_get_by_name(net, p.name);
+	if (!new_dev)
+		goto out_unlock;
+	err = dev_open(new_dev, NULL);
+out_unlock:
+	rtnl_unlock();
+	return err;
+}
+EXPORT_SYMBOL_GPL(ipip6_set_dstaddr);
+
 #define for_each_prl_rcu(start)			\
 	for (prl = rcu_dereference(start);	\
 	     prl;				\
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] ipv6: symbol_get to access a sit symbol
  2020-05-14 14:51 ` [PATCH 4/4] ipv6: symbol_get to access a sit symbol Christoph Hellwig
@ 2020-05-15  0:53   ` David Miller
  2020-05-15  6:33     ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2020-05-15  0:53 UTC (permalink / raw)
  To: hch; +Cc: kuba, kuznet, yoshfuji, netdev, linux-kernel

From: Christoph Hellwig <hch@lst.de>
Date: Thu, 14 May 2020 16:51:01 +0200

> Instead of going through the ioctl handler from kernel space, use
> symbol_get to the newly factored out ipip6_set_dstaddr helper, bypassing
> addrconf.c entirely.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
 ...
> -		memset(&p, 0, sizeof(p));
> -		p.iph.daddr = ireq.ifr6_addr.s6_addr32[3];
> -		p.iph.saddr = 0;
> -		p.iph.version = 4;
> -		p.iph.ihl = 5;
> -		p.iph.protocol = IPPROTO_IPV6;
> -		p.iph.ttl = 64;
> -		ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
> -
> -		if (ops->ndo_do_ioctl) {
> -			mm_segment_t oldfs = get_fs();
> -
> -			set_fs(KERNEL_DS);
> -			err = ops->ndo_do_ioctl(dev, &ifr, SIOCADDTUNNEL);
> -			set_fs(oldfs);
> -		} else
> -			err = -EOPNOTSUPP;
 ...
> +	p.iph.daddr = ireq.ifr6_addr.s6_addr32[3];
> +	p.iph.version = 4;
> +	p.iph.ihl = 5;
> +	p.iph.protocol = IPPROTO_IPV6;
> +	p.iph.ttl = 64;
> +	p.iph.frag_off |= htons(IP_DF);
> +
> +	err = -ENOBUFS;
> +	if (!ipip6_tunnel_locate(dev_net(tunnel_dev), &p, true))
> +		goto out_unlock;

You're not undoing one, but two levels of abstraction here.

Is this "ipip6_tunnel_locate()" call part of the SIT ioctl implementation?
Where did it come from?   Why are ->ndo_do_ioctl() implementations no longer
allowed from here?

Honestly, this feels like a bit much.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] ipv6: symbol_get to access a sit symbol
  2020-05-15  0:53   ` David Miller
@ 2020-05-15  6:33     ` Christoph Hellwig
  2020-05-16 20:55       ` David Miller
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-15  6:33 UTC (permalink / raw)
  To: David Miller; +Cc: hch, kuba, kuznet, yoshfuji, netdev, linux-kernel

On Thu, May 14, 2020 at 05:53:55PM -0700, David Miller wrote:
> You're not undoing one, but two levels of abstraction here.
> 
> Is this "ipip6_tunnel_locate()" call part of the SIT ioctl implementation?

Yes.  Take a look at the convoluted case handling the
SIOCADDTUNNEL and SIOCCHGTUNNEL commands in ipip6_tunnel_ioctl.

> Where did it come from?   Why are ->ndo_do_ioctl() implementations no longer
> allowed from here?

The problem is that we feed kernel pointers to it, which requires
set_fs address space overrides that I plan to kill off entirely.

> Honestly, this feels like a bit much.

My initial plan was to add a ->tunnel_ctl method to the net_device_ops,
and lift the copy_{to,from}_user for SIOCADDTUNNEL, SIOCCHGTUNNEL,
SIOCDELTUNNEL and maybe SIOCGETTUNNEL to net/socket.c.  But that turned
out to have two problems:

 - first these ioctls names use SIOCDEVPRIVATE range, that can also
   be implemented by other drivers
 - the ip_tunnel_parm struture is only used by the ipv4 tunneling
   drivers (including sit), the "real" ipv6 tunnels use a
   ip6_tnl_parm or ip6_tnl_parm structure instead

But if you don't like the symbol_get approach, I could do the
tunnel_ctl operation, just for the іpv4-ish tunnels, and only for
the kernel callers.

---end quoted text---

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] ipv6: symbol_get to access a sit symbol
  2020-05-15  6:33     ` Christoph Hellwig
@ 2020-05-16 20:55       ` David Miller
  2020-05-18  6:30         ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2020-05-16 20:55 UTC (permalink / raw)
  To: hch; +Cc: kuba, kuznet, yoshfuji, netdev, linux-kernel

From: Christoph Hellwig <hch@lst.de>
Date: Fri, 15 May 2020 08:33:24 +0200

> My initial plan was to add a ->tunnel_ctl method to the net_device_ops,
> and lift the copy_{to,from}_user for SIOCADDTUNNEL, SIOCCHGTUNNEL,
> SIOCDELTUNNEL and maybe SIOCGETTUNNEL to net/socket.c.  But that turned
> out to have two problems:
> 
>  - first these ioctls names use SIOCDEVPRIVATE range, that can also
>    be implemented by other drivers
>  - the ip_tunnel_parm struture is only used by the ipv4 tunneling
>    drivers (including sit), the "real" ipv6 tunnels use a
>    ip6_tnl_parm or ip6_tnl_parm structure instead

Yes, this is the core of the problem, the user provided data's type
is unknown until we are very deep in the call chains.

I wonder if there is some clever way to propagate this size value
"up"?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] ipv6: symbol_get to access a sit symbol
  2020-05-16 20:55       ` David Miller
@ 2020-05-18  6:30         ` Christoph Hellwig
  2020-05-19  0:36           ` David Miller
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2020-05-18  6:30 UTC (permalink / raw)
  To: David Miller; +Cc: hch, kuba, kuznet, yoshfuji, netdev, linux-kernel

On Sat, May 16, 2020 at 01:55:48PM -0700, David Miller wrote:
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 15 May 2020 08:33:24 +0200
> 
> > My initial plan was to add a ->tunnel_ctl method to the net_device_ops,
> > and lift the copy_{to,from}_user for SIOCADDTUNNEL, SIOCCHGTUNNEL,
> > SIOCDELTUNNEL and maybe SIOCGETTUNNEL to net/socket.c.  But that turned
> > out to have two problems:
> > 
> >  - first these ioctls names use SIOCDEVPRIVATE range, that can also
> >    be implemented by other drivers
> >  - the ip_tunnel_parm struture is only used by the ipv4 tunneling
> >    drivers (including sit), the "real" ipv6 tunnels use a
> >    ip6_tnl_parm or ip6_tnl_parm structure instead
> 
> Yes, this is the core of the problem, the user provided data's type
> is unknown until we are very deep in the call chains.
> 
> I wonder if there is some clever way to propagate this size value
> "up"?

As far as I can tell the only information vectors is the net_device
structure or its op vector.  But even then we have the problem that
other devices use the SIOCDEVPRIVATE range for something else.

I'll look into implenenting the tunnel_ctl method just for kernel
callers (plus maybe a generic helper for the ioctl), and we'll see if
you like that better.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] ipv6: symbol_get to access a sit symbol
  2020-05-18  6:30         ` Christoph Hellwig
@ 2020-05-19  0:36           ` David Miller
  0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2020-05-19  0:36 UTC (permalink / raw)
  To: hch; +Cc: kuba, kuznet, yoshfuji, netdev, linux-kernel

From: Christoph Hellwig <hch@lst.de>
Date: Mon, 18 May 2020 08:30:43 +0200

> I'll look into implenenting the tunnel_ctl method just for kernel
> callers (plus maybe a generic helper for the ioctl), and we'll see if
> you like that better.

Ok, thank you.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-19  0:36 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14 14:50 use symbol_get to create the magic ipv4/ipv6 tunnels Christoph Hellwig
2020-05-14 14:50 ` [PATCH 1/4] ipv4: streamline ipmr_new_tunnel Christoph Hellwig
2020-05-14 14:50 ` [PATCH 2/4] ipv4: consolidate the VIFF_TUNNEL handling in ipmr_new_tunnel Christoph Hellwig
2020-05-14 14:51 ` [PATCH 3/4] ipv4: use symbol_get to access ipip symbols Christoph Hellwig
2020-05-14 14:51 ` [PATCH 4/4] ipv6: symbol_get to access a sit symbol Christoph Hellwig
2020-05-15  0:53   ` David Miller
2020-05-15  6:33     ` Christoph Hellwig
2020-05-16 20:55       ` David Miller
2020-05-18  6:30         ` Christoph Hellwig
2020-05-19  0:36           ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.