Patchwork tun: add tun_flags, owner, group attributes in sysfs

login
register
mail settings
Submitter David Woodhouse
Date May 4, 2009, 10:32 a.m.
Message ID <1241433136.6126.70.camel@macbook.infradead.org>
Download mbox | patch
Permalink /patch/26838/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Woodhouse - May 4, 2009, 10:32 a.m.
This patch adds three attribute files in /sys/class/net/$dev/ for tun
devices; allowing userspace to obtain the information which TUNGETIFF
offers, and more, but without having to attach to the device in question
(which may not be possible if it's in use).

It also fixes a bug which has been present in the TUNGETIFF ioctl since
its inception, where it would never set IFF_TUN or IFF_TAP according to
the device type. (Look carefully at the code which I remove from
tun_get_iff() and how the new tun_flags() helper is subtly different).

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
I don't have to explicitly remove the files, do I? They go away
naturally when the device is unregistered?
Michael Tokarev - May 4, 2009, 10:36 a.m.
David Woodhouse wrote:
> This patch adds three attribute files in /sys/class/net/$dev/ for tun
> devices; allowing userspace to obtain the information which TUNGETIFF
> offers, and more, but without having to attach to the device in question
> (which may not be possible if it's in use).
> 
[]
> +static DEVICE_ATTR(tun_flags, 0444, tun_show_flags, NULL);
> +static DEVICE_ATTR(owner, 0444, tun_show_owner, NULL);
> +static DEVICE_ATTR(group, 0444, tun_show_group, NULL);

Is there any reason why those files are not writable?

I understand flags one, sorta (but it is still useful to
be able to change some flags, like persistent, while it's
running), but for owner/group - it's just an integer that's
used to check permissions for ioctl, and can be set in sysfs
just fine.  I think anyway.

/mjt
--
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
stephen hemminger - May 4, 2009, 2:47 p.m.
On Mon, 04 May 2009 11:32:16 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> This patch adds three attribute files in /sys/class/net/$dev/ for tun
> devices; allowing userspace to obtain the information which TUNGETIFF
> offers, and more, but without having to attach to the device in question
> (which may not be possible if it's in use).
> 
> It also fixes a bug which has been present in the TUNGETIFF ioctl since
> its inception, where it would never set IFF_TUN or IFF_TAP according to
> the device type. (Look carefully at the code which I remove from
> tun_get_iff() and how the new tun_flags() helper is subtly different).
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> ---
> I don't have to explicitly remove the files, do I? They go away
> naturally when the device is unregistered?
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 94622e5..4cda69b 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -865,6 +865,52 @@ static struct proto tun_proto = {
>  	.obj_size	= sizeof(struct tun_sock),
>  };
>  
> +static int tun_flags(struct tun_struct *tun)
> +{
> +	int flags = 0;
> +
> +	if (tun->flags & TUN_TUN_DEV)
> +		flags |= IFF_TUN;
> +	else
> +		flags |= IFF_TAP;
> +
> +	if (tun->flags & TUN_NO_PI)
> +		flags |= IFF_NO_PI;
> +
> +	if (tun->flags & TUN_ONE_QUEUE)
> +		flags |= IFF_ONE_QUEUE;
> +
> +	if (tun->flags & TUN_VNET_HDR)
> +		flags |= IFF_VNET_HDR;
> +
> +	return flags;
> +}
> +
> +static ssize_t tun_show_flags(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct tun_struct *tun = netdev_priv(to_net_dev(dev));
> +	return sprintf(buf, "0x%x\n", tun_flags(tun));
> +}
> +
> +static ssize_t tun_show_owner(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct tun_struct *tun = netdev_priv(to_net_dev(dev));
> +	return sprintf(buf, "%d\n", tun->owner);
> +}
> +
> +static ssize_t tun_show_group(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct tun_struct *tun = netdev_priv(to_net_dev(dev));
> +	return sprintf(buf, "%d\n", tun->group);
> +}
> +
> +static DEVICE_ATTR(tun_flags, 0444, tun_show_flags, NULL);
> +static DEVICE_ATTR(owner, 0444, tun_show_owner, NULL);
> +static DEVICE_ATTR(group, 0444, tun_show_group, NULL);
> +
>  static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  {
>  	struct sock *sk;
> @@ -950,6 +996,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		if (err < 0)
>  			goto err_free_sk;
>  
> +		if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
> +		    device_create_file(&tun->dev->dev, &dev_attr_owner) ||
> +		    device_create_file(&tun->dev->dev, &dev_attr_group))
> +			printk(KERN_ERR "Failed to create tun sysfs files\n");
> +
>  		sk->sk_destruct = tun_sock_destruct;
>  
>  		err = tun_attach(tun, file);
> @@ -1002,21 +1053,7 @@ static int tun_get_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  
>  	strcpy(ifr->ifr_name, tun->dev->name);
>  
> -	ifr->ifr_flags = 0;
> -
> -	if (ifr->ifr_flags & TUN_TUN_DEV)
> -		ifr->ifr_flags |= IFF_TUN;
> -	else
> -		ifr->ifr_flags |= IFF_TAP;
> -
> -	if (tun->flags & TUN_NO_PI)
> -		ifr->ifr_flags |= IFF_NO_PI;
> -
> -	if (tun->flags & TUN_ONE_QUEUE)
> -		ifr->ifr_flags |= IFF_ONE_QUEUE;
> -
> -	if (tun->flags & TUN_VNET_HDR)
> -		ifr->ifr_flags |= IFF_VNET_HDR;
> +	ifr->ifr_flags = tun_flags(tun);
>  
>  	tun_put(tun);
>  	return 0;

Netlink please not sysfs. also, any sysfs attributes have to handle unregistration
issues.
      ( sleep 900; read owner ) </sys/class/net/tun0/owner &
      rmmod tun
--
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 Woodhouse - May 4, 2009, 2:58 p.m.
On Mon, 2009-05-04 at 07:47 -0700, Stephen Hemminger wrote:
> Netlink please not sysfs. 

That seems like overkill in this case.

> also, any sysfs attributes have to handle unregistration issues.
>       ( sleep 900; read owner ) </sys/class/net/tun0/owner &
>       rmmod tun

That gets handled for you automatically...

[root@tylersburg ~]# ( sleep 900 ; read owner; echo $owner ) < /sys/class/net/tun0/owner &
[1] 4728
[root@tylersburg ~]# ./ip tuntap del tun0 mode tun
[root@tylersburg ~]# rmmod tun
[root@tylersburg ~]# killall sleep
-bash: line 28:  4729 Terminated              sleep 900
[root@tylersburg ~]# -bash: line 28: read: read error: 0: No such device
David Woodhouse - May 5, 2009, 10:18 p.m.
On Mon, 2009-05-04 at 14:36 +0400, Michael Tokarev wrote:
> David Woodhouse wrote:
> > This patch adds three attribute files in /sys/class/net/$dev/ for tun
> > devices; allowing userspace to obtain the information which TUNGETIFF
> > offers, and more, but without having to attach to the device in question
> > (which may not be possible if it's in use).
> > 
> []
> > +static DEVICE_ATTR(tun_flags, 0444, tun_show_flags, NULL);
> > +static DEVICE_ATTR(owner, 0444, tun_show_owner, NULL);
> > +static DEVICE_ATTR(group, 0444, tun_show_group, NULL);
> 
> Is there any reason why those files are not writable?
> 
> I understand flags one, sorta (but it is still useful to
> be able to change some flags, like persistent, while it's
> running), but for owner/group - it's just an integer that's
> used to check permissions for ioctl, and can be set in sysfs
> just fine.  I think anyway.

I did think about it, but didn't see the point. There's little benefit
in being able to write owner/group through sysfs, and it's non-trivial
to get the permissions right.

You can currently change owner/group if you can attach to the device...
and you can attach to the device if you're _already_ the appropriate
uid/gid, or if you have CAP_NET_ADMIN. 

We can't make opens for write fail, as far as I'm aware -- the best we
could do is to reproduce the permissions check in the sysfs set
function, and return -EPERM to the _write_, which doesn't really fill me
with joy.

Being able to mark a device as non-persistent through sysfs while it's
open would be cute, I suppose -- but not cute enough that it's worth
having to deal with the case of marking it non-persistent that way while
it's _not_ open, which means it needs to disappear as soon as you write
the flags variable...

On the whole, it seemed better just to have them read-only. That's all
we really need, after all.
David Miller - May 9, 2009, 8:27 p.m.
From: David Woodhouse <dwmw2@infradead.org>
Date: Mon, 04 May 2009 11:32:16 +0100

> I don't have to explicitly remove the files, do I? They go away
> naturally when the device is unregistered?

I'm not so sure about this.  I can't see anything in the
drivers/base/core.c code that automatically deletes anything
other than device groups and attributes.
--
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 Woodhouse - May 9, 2009, 10:27 p.m.
On Sat, 2009-05-09 at 13:27 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Mon, 04 May 2009 11:32:16 +0100
> 
> > I don't have to explicitly remove the files, do I? They go away
> > naturally when the device is unregistered?
> 
> I'm not so sure about this.  I can't see anything in the
> drivers/base/core.c code that automatically deletes anything
> other than device groups and attributes.

Hm, I really do think the attributes go away automatically when the
parent device does.

If not, gianfar_sysfs.c and niu.c want fixing too -- and we should
probably have a WARN_ON() in the sysfs code when it happens, if
possible.

I'll experiment (since I'm getting lost when I try to work it out by
inspection).
David Woodhouse - May 9, 2009, 10:55 p.m.
On Sat, 2009-05-09 at 23:27 +0100, David Woodhouse wrote:
> On Sat, 2009-05-09 at 13:27 -0700, David Miller wrote:
> > From: David Woodhouse <dwmw2@infradead.org>
> > Date: Mon, 04 May 2009 11:32:16 +0100
> > 
> > > I don't have to explicitly remove the files, do I? They go away
> > > naturally when the device is unregistered?
> > 
> > I'm not so sure about this.  I can't see anything in the
> > drivers/base/core.c code that automatically deletes anything
> > other than device groups and attributes.
> 
> Hm, I really do think the attributes go away automatically when the
> parent device does.
> 
> If not, gianfar_sysfs.c and niu.c want fixing too -- and we should
> probably have a WARN_ON() in the sysfs code when it happens, if
> possible.
> 
> I'll experiment (since I'm getting lost when I try to work it out by
> inspection).

I added a WARN_ON(!strcmp(sd->s_name, "tun_flags")) into
release_sysfs_dirent() to see when (and if) it gets called.

It does.

Looking at the backtrace, I see it's __sysfs_remove_dir() which iterates
over all the children of the device's directory and kills them:

[   98.024138]  [<ffffffff810f2e4e>] release_sysfs_dirent+0x49/0xc8
[   98.030178]  [<ffffffff810f2eff>] __sysfs_put+0x32/0x36
[   98.035432]  [<ffffffff810f3133>] sysfs_addrm_finish+0x20c/0x239
[   98.041470]  [<ffffffff810f2870>] ? sysfs_ilookup_test+0x0/0x14
[   98.047417]  [<ffffffff810f3214>] sysfs_remove_dir+0x6f/0x82
[   98.053107]  [<ffffffff8118fce4>] kobject_del+0x16/0x37
[   98.058367]  [<ffffffff8123953f>] device_del+0x16b/0x17b
[   98.063769]  [<ffffffff813b2d8f>] netdev_unregister_kobject+0x24/0x29
[   98.070241]  [<ffffffff813a7f0f>] rollback_registered+0x1d2/0x1e3
[   98.076364]  [<ffffffff813a7f5a>] unregister_netdevice+0x3a/0x69
[   98.082402]  [<ffffffffa0006458>] tun_chr_close+0x3d/0x66 [tun]
David Miller - May 9, 2009, 10:56 p.m.
From: David Woodhouse <dwmw2@infradead.org>
Date: Sat, 09 May 2009 23:27:53 +0100

> I'll experiment (since I'm getting lost when I try to work it out by
> inspection).

Thanks David.
--
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 - May 10, 2009, 5:54 a.m.
From: David Woodhouse <dwmw2@infradead.org>
Date: Mon, 04 May 2009 11:32:16 +0100

> This patch adds three attribute files in /sys/class/net/$dev/ for tun
> devices; allowing userspace to obtain the information which TUNGETIFF
> offers, and more, but without having to attach to the device in question
> (which may not be possible if it's in use).
> 
> It also fixes a bug which has been present in the TUNGETIFF ioctl since
> its inception, where it would never set IFF_TUN or IFF_TAP according to
> the device type. (Look carefully at the code which I remove from
> tun_get_iff() and how the new tun_flags() helper is subtly different).
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Applied to net-next-2.6, 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

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 94622e5..4cda69b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -865,6 +865,52 @@  static struct proto tun_proto = {
 	.obj_size	= sizeof(struct tun_sock),
 };
 
+static int tun_flags(struct tun_struct *tun)
+{
+	int flags = 0;
+
+	if (tun->flags & TUN_TUN_DEV)
+		flags |= IFF_TUN;
+	else
+		flags |= IFF_TAP;
+
+	if (tun->flags & TUN_NO_PI)
+		flags |= IFF_NO_PI;
+
+	if (tun->flags & TUN_ONE_QUEUE)
+		flags |= IFF_ONE_QUEUE;
+
+	if (tun->flags & TUN_VNET_HDR)
+		flags |= IFF_VNET_HDR;
+
+	return flags;
+}
+
+static ssize_t tun_show_flags(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct tun_struct *tun = netdev_priv(to_net_dev(dev));
+	return sprintf(buf, "0x%x\n", tun_flags(tun));
+}
+
+static ssize_t tun_show_owner(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct tun_struct *tun = netdev_priv(to_net_dev(dev));
+	return sprintf(buf, "%d\n", tun->owner);
+}
+
+static ssize_t tun_show_group(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct tun_struct *tun = netdev_priv(to_net_dev(dev));
+	return sprintf(buf, "%d\n", tun->group);
+}
+
+static DEVICE_ATTR(tun_flags, 0444, tun_show_flags, NULL);
+static DEVICE_ATTR(owner, 0444, tun_show_owner, NULL);
+static DEVICE_ATTR(group, 0444, tun_show_group, NULL);
+
 static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 {
 	struct sock *sk;
@@ -950,6 +996,11 @@  static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		if (err < 0)
 			goto err_free_sk;
 
+		if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
+		    device_create_file(&tun->dev->dev, &dev_attr_owner) ||
+		    device_create_file(&tun->dev->dev, &dev_attr_group))
+			printk(KERN_ERR "Failed to create tun sysfs files\n");
+
 		sk->sk_destruct = tun_sock_destruct;
 
 		err = tun_attach(tun, file);
@@ -1002,21 +1053,7 @@  static int tun_get_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 	strcpy(ifr->ifr_name, tun->dev->name);
 
-	ifr->ifr_flags = 0;
-
-	if (ifr->ifr_flags & TUN_TUN_DEV)
-		ifr->ifr_flags |= IFF_TUN;
-	else
-		ifr->ifr_flags |= IFF_TAP;
-
-	if (tun->flags & TUN_NO_PI)
-		ifr->ifr_flags |= IFF_NO_PI;
-
-	if (tun->flags & TUN_ONE_QUEUE)
-		ifr->ifr_flags |= IFF_ONE_QUEUE;
-
-	if (tun->flags & TUN_VNET_HDR)
-		ifr->ifr_flags |= IFF_VNET_HDR;
+	ifr->ifr_flags = tun_flags(tun);
 
 	tun_put(tun);
 	return 0;