Patchwork [net-next,1/4] ipip: allow to deactivate the creation of fb dev

login
register
mail settings
Submitter Nicolas Dichtel
Date Nov. 16, 2012, 4:14 p.m.
Message ID <1353082456-21234-2-git-send-email-nicolas.dichtel@6wind.com>
Download mbox | patch
Permalink /patch/199679/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Nicolas Dichtel - Nov. 16, 2012, 4:14 p.m.
Now that tunnels can be configured via rtnetlink, this device is not mandatory.
The default is conservative.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ipip.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)
stephen hemminger - Nov. 16, 2012, 4:29 p.m.
On Fri, 16 Nov 2012 17:14:13 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

> Now that tunnels can be configured via rtnetlink, this device is not mandatory.
> The default is conservative.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>


Although I am in favor of reducing clutter, and we even have to put in special case
code to ignore these stub devices in the Vyatta scripts. Module parameters are bit of a nuisance to deal with, but maybe
the only way for this kind of thing and keep the required ABI.

Not sure if I can fully endorse this. The device may still have uses.
It is still useful for capturing "none of the above" packets
and is used to auto-load module via module aliases.
--
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
Nicolas Dichtel - Nov. 16, 2012, 4:46 p.m.
Le 16/11/2012 17:29, Stephen Hemminger a écrit :
> On Fri, 16 Nov 2012 17:14:13 +0100
> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>
>> Now that tunnels can be configured via rtnetlink, this device is not mandatory.
>> The default is conservative.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
>
> Although I am in favor of reducing clutter, and we even have to put in special case
> code to ignore these stub devices in the Vyatta scripts. Module parameters are bit of a nuisance to deal with, but maybe
> the only way for this kind of thing and keep the required ABI.
>
> Not sure if I can fully endorse this. The device may still have uses.
> It is still useful for capturing "none of the above" packets
If you need to capture these packets, you can still create a tunnel with local 
any and remote any, even if the fb_device has not been created.

> and is used to auto-load module via module aliases.
Right, but if user uses netlink, the problem exists without these patches too.

By default, the fb device is created, so there is no change if you don't set 
explicitly setup_fb to 0.
--
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 - Nov. 20, 2012, 12:06 a.m.
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri, 16 Nov 2012 17:14:13 +0100

> Now that tunnels can be configured via rtnetlink, this device is not mandatory.
> The default is conservative.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

I'm not too thrilled about this change, mostly because of my dislike of
module parameters in general.

But in this case there appears to be real bugs in the two sets of changes
where you add this setup_fb thing.

> @@ -1057,7 +1066,8 @@ static void __net_exit ipip_exit_net(struct net *net)
>  
>  	rtnl_lock();
>  	ipip_destroy_tunnels(ipn, &list);
> -	unregister_netdevice_queue(ipn->fb_tunnel_dev, &list);
> +	if (setup_fb)
> +		unregister_netdevice_queue(ipn->fb_tunnel_dev, &list);
>  	unregister_netdevice_many(&list);
>  	rtnl_unlock();
>  }

Users can modify module parameter values via sysfs after the module
is loaded, so you need a more internal and protected state to use
to decide whether you really need to unregister the thing or not.

But to me it's just symptomatic of what a bad idea this is in the
first place.
--
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 - Nov. 20, 2012, 12:07 a.m.
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri, 16 Nov 2012 17:46:11 +0100

> By default, the fb device is created, so there is no change if you
> don't set explicitly setup_fb to 0.


Nicolas, this idea is contentous to me too.

Why not put this aside and submit the other parts of your
patch set on their own, since those looked fine to me?

Thanks.
--
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
Nicolas Dichtel - Nov. 20, 2012, 8:30 a.m.
Le 20/11/2012 01:07, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Fri, 16 Nov 2012 17:46:11 +0100
>
>> By default, the fb device is created, so there is no change if you
>> don't set explicitly setup_fb to 0.
>
>
> Nicolas, this idea is contentous to me too.
>
> Why not put this aside and submit the other parts of your
> patch set on their own, since those looked fine to me?
Ok, no problem. I wanted to get feedback about this kind of idea, I understand 
the point.

When you said "the other parts", you mean only patch 2/4 (sit: allow to 
configure 6rd tunnels via netlink) or 3/4 and 4/4 too?
Because 3/4 and 4/4 are equivalent to this one.
--
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 - Nov. 20, 2012, 8:34 a.m.
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 20 Nov 2012 09:30:17 +0100

> Le 20/11/2012 01:07, David Miller a écrit :
>> Why not put this aside and submit the other parts of your
>> patch set on their own, since those looked fine to me?
> Ok, no problem. I wanted to get feedback about this kind of idea, I
> understand the point.
> 
> When you said "the other parts", you mean only patch 2/4 (sit: allow
> to configure 6rd tunnels via netlink) or 3/4 and 4/4 too?
> Because 3/4 and 4/4 are equivalent to this one.

I meant patch #2.
--
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/net/ipv4/ipip.c b/net/ipv4/ipip.c
index c26c171..9e11633 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -124,6 +124,11 @@  static bool log_ecn_error = true;
 module_param(log_ecn_error, bool, 0644);
 MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN");
 
+static bool setup_fb = true;
+module_param(setup_fb, bool, 0644);
+MODULE_PARM_DESC(setup_fb,
+		 "Setup the fb device to configure tunnel via IOCTL");
+
 static int ipip_net_id __read_mostly;
 struct ipip_net {
 	struct ip_tunnel __rcu *tunnels_r_l[HASH_SIZE];
@@ -1022,25 +1027,29 @@  static int __net_init ipip_init_net(struct net *net)
 	ipn->tunnels[2] = ipn->tunnels_r;
 	ipn->tunnels[3] = ipn->tunnels_r_l;
 
-	ipn->fb_tunnel_dev = alloc_netdev(sizeof(struct ip_tunnel),
-					   "tunl0",
-					   ipip_tunnel_setup);
-	if (!ipn->fb_tunnel_dev) {
-		err = -ENOMEM;
-		goto err_alloc_dev;
-	}
-	dev_net_set(ipn->fb_tunnel_dev, net);
+	if (setup_fb) {
+		ipn->fb_tunnel_dev = alloc_netdev(sizeof(struct ip_tunnel),
+						  "tunl0",
+						  ipip_tunnel_setup);
+		if (!ipn->fb_tunnel_dev) {
+			err = -ENOMEM;
+			goto err_alloc_dev;
+		}
+		dev_net_set(ipn->fb_tunnel_dev, net);
 
-	err = ipip_fb_tunnel_init(ipn->fb_tunnel_dev);
-	if (err)
-		goto err_reg_dev;
+		err = ipip_fb_tunnel_init(ipn->fb_tunnel_dev);
+		if (err)
+			goto err_reg_dev;
 
-	if ((err = register_netdev(ipn->fb_tunnel_dev)))
-		goto err_reg_dev;
+		err = register_netdev(ipn->fb_tunnel_dev);
+		if (err)
+			goto err_reg_dev;
 
-	t = netdev_priv(ipn->fb_tunnel_dev);
+		t = netdev_priv(ipn->fb_tunnel_dev);
 
-	strcpy(t->parms.name, ipn->fb_tunnel_dev->name);
+		strcpy(t->parms.name, ipn->fb_tunnel_dev->name);
+	} else
+		ipn->fb_tunnel_dev = NULL;
 	return 0;
 
 err_reg_dev:
@@ -1057,7 +1066,8 @@  static void __net_exit ipip_exit_net(struct net *net)
 
 	rtnl_lock();
 	ipip_destroy_tunnels(ipn, &list);
-	unregister_netdevice_queue(ipn->fb_tunnel_dev, &list);
+	if (setup_fb)
+		unregister_netdevice_queue(ipn->fb_tunnel_dev, &list);
 	unregister_netdevice_many(&list);
 	rtnl_unlock();
 }