| Message ID | 152579647246.21100.10461408116587658568.stgit@localhost.localdomain |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] tun: Do SIOCGSKNS out of rtnl_lock() | expand |
On 2018年05月09日 00:21, Kirill Tkhai wrote: > Since net ns of tun device is assigned on the device creation, > and it never changes, we do not need to use any lock to get it > from alive tun. > > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> > --- > drivers/net/tun.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index d3c04ab9752a..44d4f3d25350 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -2850,10 +2850,10 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > unsigned long arg, int ifreq_len) > { > struct tun_file *tfile = file->private_data; > + struct net *net = sock_net(&tfile->sk); > struct tun_struct *tun; > void __user* argp = (void __user*)arg; > struct ifreq ifr; > - struct net *net; > kuid_t owner; > kgid_t group; > int sndbuf; > @@ -2877,14 +2877,18 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > */ > return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, > (unsigned int __user*)argp); > - } else if (cmd == TUNSETQUEUE) > + } else if (cmd == TUNSETQUEUE) { > return tun_set_queue(file, &ifr); > + } else if (cmd == SIOCGSKNS) { Not for this patch, reusing socket ioctl cmd is probably not good though they were probably not intersected (see ioctl-number.txt). We probably need to introduce TUN specific ioctls for SIOCGSKNS and SIOCGIFHWADDR and warn for socket ones. Thanks > + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) > + return -EPERM; > + return open_related_ns(&net->ns, get_net_ns); > + } > > ret = 0; > rtnl_lock(); > > tun = tun_get(tfile); > - net = sock_net(&tfile->sk); > if (cmd == TUNSETIFF) { > ret = -EEXIST; > if (tun) > @@ -2914,14 +2918,6 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > tfile->ifindex = ifindex; > goto unlock; > } > - if (cmd == SIOCGSKNS) { > - ret = -EPERM; > - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) > - goto unlock; > - > - ret = open_related_ns(&net->ns, get_net_ns); > - goto unlock; > - } > > ret = -EBADFD; > if (!tun) >
Hi, Jason, On 09.05.2018 10:18, Jason Wang wrote: > > > On 2018年05月09日 00:21, Kirill Tkhai wrote: >> Since net ns of tun device is assigned on the device creation, >> and it never changes, we do not need to use any lock to get it >> from alive tun. >> >> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >> --- >> drivers/net/tun.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index d3c04ab9752a..44d4f3d25350 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -2850,10 +2850,10 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >> unsigned long arg, int ifreq_len) >> { >> struct tun_file *tfile = file->private_data; >> + struct net *net = sock_net(&tfile->sk); >> struct tun_struct *tun; >> void __user* argp = (void __user*)arg; >> struct ifreq ifr; >> - struct net *net; >> kuid_t owner; >> kgid_t group; >> int sndbuf; >> @@ -2877,14 +2877,18 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >> */ >> return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, >> (unsigned int __user*)argp); >> - } else if (cmd == TUNSETQUEUE) >> + } else if (cmd == TUNSETQUEUE) { >> return tun_set_queue(file, &ifr); >> + } else if (cmd == SIOCGSKNS) { > > Not for this patch, reusing socket ioctl cmd is probably not good though they were probably not intersected (see ioctl-number.txt). We probably need to introduce TUN specific ioctls for SIOCGSKNS and SIOCGIFHWADDR and warn for socket ones. The most of socket ioctl cmds use 0x8900 type: #define SOCK_IOC_TYPE 0x89 while tun cmd is 5400 ('T'). They should not intersect. The only exceptions are #define SIOCINQ FIONREAD #define SIOCOUTQ TIOCOUTQ #define TIOCOUTQ 0x5411 #define FIONREAD 0x541B But they can't intersect even with exceptions, since tun nr starts from 200: #define TUNSETNOCSUM _IOW('T', 200, int) and 200 > 0x1b (==27). I implemented SIOCGSKNS cmd in the same style as older socket cmds were used. I'm not sure, we can remove existing SIOCGIFHWADDR, since they are already used. If we add a warn, which time will we able to remove them? Some old software may use it, and in case of the program isn't developed any more, nobody can fix this warnings, even if he/she sees them.. Kirill > >> + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >> + return -EPERM; >> + return open_related_ns(&net->ns, get_net_ns); >> + } >> ret = 0; >> rtnl_lock(); >> tun = tun_get(tfile); >> - net = sock_net(&tfile->sk); >> if (cmd == TUNSETIFF) { >> ret = -EEXIST; >> if (tun) >> @@ -2914,14 +2918,6 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >> tfile->ifindex = ifindex; >> goto unlock; >> } >> - if (cmd == SIOCGSKNS) { >> - ret = -EPERM; >> - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >> - goto unlock; >> - >> - ret = open_related_ns(&net->ns, get_net_ns); >> - goto unlock; >> - } >> ret = -EBADFD; >> if (!tun) >> >
On 2018年05月09日 17:00, Kirill Tkhai wrote: > Hi, Jason, > > On 09.05.2018 10:18, Jason Wang wrote: >> >> On 2018年05月09日 00:21, Kirill Tkhai wrote: >>> Since net ns of tun device is assigned on the device creation, >>> and it never changes, we do not need to use any lock to get it >>> from alive tun. >>> >>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>> --- >>> drivers/net/tun.c | 18 +++++++----------- >>> 1 file changed, 7 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >>> index d3c04ab9752a..44d4f3d25350 100644 >>> --- a/drivers/net/tun.c >>> +++ b/drivers/net/tun.c >>> @@ -2850,10 +2850,10 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >>> unsigned long arg, int ifreq_len) >>> { >>> struct tun_file *tfile = file->private_data; >>> + struct net *net = sock_net(&tfile->sk); >>> struct tun_struct *tun; >>> void __user* argp = (void __user*)arg; >>> struct ifreq ifr; >>> - struct net *net; >>> kuid_t owner; >>> kgid_t group; >>> int sndbuf; >>> @@ -2877,14 +2877,18 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >>> */ >>> return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, >>> (unsigned int __user*)argp); >>> - } else if (cmd == TUNSETQUEUE) >>> + } else if (cmd == TUNSETQUEUE) { >>> return tun_set_queue(file, &ifr); >>> + } else if (cmd == SIOCGSKNS) { >> Not for this patch, reusing socket ioctl cmd is probably not good though they were probably not intersected (see ioctl-number.txt). We probably need to introduce TUN specific ioctls for SIOCGSKNS and SIOCGIFHWADDR and warn for socket ones. > The most of socket ioctl cmds use 0x8900 type: > > #define SOCK_IOC_TYPE 0x89 > > while tun cmd is 5400 ('T'). They should not intersect. > > The only exceptions are > > #define SIOCINQ FIONREAD > #define SIOCOUTQ TIOCOUTQ > > #define TIOCOUTQ 0x5411 > #define FIONREAD 0x541B > > But they can't intersect even with exceptions, since tun nr starts from 200: > > #define TUNSETNOCSUM _IOW('T', 200, int) > > and 200 > 0x1b (==27). > > I implemented SIOCGSKNS cmd in the same style as older socket cmds were used. > I'm not sure, we can remove existing SIOCGIFHWADDR, since they are already used. I think it's too late to remove it. > If we add a warn, which time will we able to remove them? Some old software may > use it, and in case of the program isn't developed any more, nobody can fix this > warnings, even if he/she sees them.. I think this give a chance to push new wrote userspace to use new ioctl cmd. Thanks > > Kirill > >>> + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >>> + return -EPERM; >>> + return open_related_ns(&net->ns, get_net_ns); >>> + } >>> ret = 0; >>> rtnl_lock(); >>> tun = tun_get(tfile); >>> - net = sock_net(&tfile->sk); >>> if (cmd == TUNSETIFF) { >>> ret = -EEXIST; >>> if (tun) >>> @@ -2914,14 +2918,6 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, >>> tfile->ifindex = ifindex; >>> goto unlock; >>> } >>> - if (cmd == SIOCGSKNS) { >>> - ret = -EPERM; >>> - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >>> - goto unlock; >>> - >>> - ret = open_related_ns(&net->ns, get_net_ns); >>> - goto unlock; >>> - } >>> ret = -EBADFD; >>> if (!tun) >>>
From: Kirill Tkhai <ktkhai@virtuozzo.com> Date: Tue, 08 May 2018 19:21:34 +0300 > Since net ns of tun device is assigned on the device creation, > and it never changes, we do not need to use any lock to get it > from alive tun. > > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> Applied, thank you.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index d3c04ab9752a..44d4f3d25350 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -2850,10 +2850,10 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg, int ifreq_len) { struct tun_file *tfile = file->private_data; + struct net *net = sock_net(&tfile->sk); struct tun_struct *tun; void __user* argp = (void __user*)arg; struct ifreq ifr; - struct net *net; kuid_t owner; kgid_t group; int sndbuf; @@ -2877,14 +2877,18 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, */ return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES, (unsigned int __user*)argp); - } else if (cmd == TUNSETQUEUE) + } else if (cmd == TUNSETQUEUE) { return tun_set_queue(file, &ifr); + } else if (cmd == SIOCGSKNS) { + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + return -EPERM; + return open_related_ns(&net->ns, get_net_ns); + } ret = 0; rtnl_lock(); tun = tun_get(tfile); - net = sock_net(&tfile->sk); if (cmd == TUNSETIFF) { ret = -EEXIST; if (tun) @@ -2914,14 +2918,6 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, tfile->ifindex = ifindex; goto unlock; } - if (cmd == SIOCGSKNS) { - ret = -EPERM; - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) - goto unlock; - - ret = open_related_ns(&net->ns, get_net_ns); - goto unlock; - } ret = -EBADFD; if (!tun)
Since net ns of tun device is assigned on the device creation, and it never changes, we do not need to use any lock to get it from alive tun. Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> --- drivers/net/tun.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)