| Message ID | 1241433136.6126.70.camel@macbook.infradead.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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.
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
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).
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]
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
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
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;
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?