[PATCH net-next 0/2] tun: support socket system calls

[PATCH net-next 0/2] tun: support socket system calls

[Alex Gartrell]

There is an underlying socket object in struct tun that isn't accessible,
so if you try to do a recvmmsg on a tun file descriptor you'll get an
error. This small patchset allows external file objects to claim socket
status and enables it for tun_files.

Alex Gartrell (2):
  socket: Allow external sockets to use socket syscalls
  tun: enable socket system calls

 drivers/net/tun.c  | 34 ++++++++++++++++++++++------------
 include/linux/fs.h |  2 +-
 net/socket.c       |  3 ++-
 3 files changed, 25 insertions(+), 14 deletions(-)

-- 
Alex Gartrell <agartrell@fb.com>

[PATCH net-next 1/2] socket: Allow external sockets to use socket syscalls

[Alex Gartrell]

Currently the "is-socket" test for a file compares the ops table pointer,
which is static and local to the socket.c.  Instead, this adds a flag for
private_data_is_socket.  This is an exceptionally long commit message for a
two-line patch.

Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
 include/linux/fs.h | 2 +-
 net/socket.c       | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index bb29b02..d162476 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -809,7 +809,7 @@ struct file {
 #endif
 	/* needed for tty driver, and maybe others */
 	void			*private_data;
-
+	bool			private_data_is_socket : 1;
 #ifdef CONFIG_EPOLL
 	/* Used by fs/eventpoll.c to link all the hooks to this file */
 	struct list_head	f_ep_links;
diff --git a/net/socket.c b/net/socket.c
index 8809afc..cd853be 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -388,6 +388,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
 	sock->file = file;
 	file->f_flags = O_RDWR | (flags & O_NONBLOCK);
 	file->private_data = sock;
+	file->private_data_is_socket = true;
 	return file;
 }
 EXPORT_SYMBOL(sock_alloc_file);
@@ -411,7 +412,7 @@ static int sock_map_fd(struct socket *sock, int flags)
 
 struct socket *sock_from_file(struct file *file, int *err)
 {
-	if (file->f_op == &socket_file_ops)
+	if (file->private_data_is_socket)
 		return file->private_data;	/* set in sock_map_fd */
 
 	*err = -ENOTSOCK;
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH net-next 1/2] socket: Allow external sockets to use socket syscalls

[Jason Wang]

On 12/26/2014 02:50 PM, Alex Gartrell wrote:
> Currently the "is-socket" test for a file compares the ops table pointer,
> which is static and local to the socket.c.  Instead, this adds a flag for
> private_data_is_socket.  This is an exceptionally long commit message for a
> two-line patch.
>
> Signed-off-by: Alex Gartrell <agartrell@fb.com>
> ---
>  include/linux/fs.h | 2 +-
>  net/socket.c       | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bb29b02..d162476 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -809,7 +809,7 @@ struct file {
>  #endif
>  	/* needed for tty driver, and maybe others */
>  	void			*private_data;
> -
> +	bool			private_data_is_socket : 1;
>  #ifdef CONFIG_EPOLL
>  	/* Used by fs/eventpoll.c to link all the hooks to this file */
>  	struct list_head	f_ep_links;
> diff --git a/net/socket.c b/net/socket.c
> index 8809afc..cd853be 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -388,6 +388,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
>  	sock->file = file;
>  	file->f_flags = O_RDWR | (flags & O_NONBLOCK);
>  	file->private_data = sock;
> +	file->private_data_is_socket = true;

This is only safe if all user of sock_alloc_file() have full support for
each method in proto_ops.
>  	return file;
>  }
>  EXPORT_SYMBOL(sock_alloc_file);
> @@ -411,7 +412,7 @@ static int sock_map_fd(struct socket *sock, int flags)
>  
>  struct socket *sock_from_file(struct file *file, int *err)
>  {
> -	if (file->f_op == &socket_file_ops)
> +	if (file->private_data_is_socket)
>  		return file->private_data;	/* set in sock_map_fd */
>  
>  	*err = -ENOTSOCK;

Not sure it's the best method, how about a dedicated f_op to do this?

Re: [PATCH net-next 1/2] socket: Allow external sockets to use socket syscalls

[Alex Gartrell]

Hello Jason,

Thanks again for your comments.

On 12/26/14 4:45 AM, Jason Wang wrote:
>> @@ -388,6 +388,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
>>   	sock->file = file;
>>   	file->f_flags = O_RDWR | (flags & O_NONBLOCK);
>>   	file->private_data = sock;
>> +	file->private_data_is_socket = true;
>
> This is only safe if all user of sock_alloc_file() have full support for
> each method in proto_ops.

This doesn't change anything in the invocation of the syscalls, as every 
file allocated through this path already passes the "is_socket" test due 
to the (sadly missing here)
     file = alloc_file(..., &socket_file_ops)
above, so this makes things no less safe than they were.

Of course we also need to implement these proto_ops for the tun device 
in the subsequent patch to make it work.

>>   	return file;
>>   }
>>   EXPORT_SYMBOL(sock_alloc_file);
>> @@ -411,7 +412,7 @@ static int sock_map_fd(struct socket *sock, int flags)
>>
>>   struct socket *sock_from_file(struct file *file, int *err)
>>   {
>> -	if (file->f_op == &socket_file_ops)
>> +	if (file->private_data_is_socket)
>>   		return file->private_data;	/* set in sock_map_fd */
>>
>>   	*err = -ENOTSOCK;
>
> Not sure it's the best method, how about a dedicated f_op to do this?

So like a get_socket operation?  That would simplify this a lot, 
certainly (we wouldn't need to move private_data around in the tun driver).

Thanks,
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH net-next 1/2] socket: Allow external sockets to use socket syscalls

[Al Viro]

On Thu, Dec 25, 2014 at 10:50:23PM -0800, Alex Gartrell wrote:
> Currently the "is-socket" test for a file compares the ops table pointer,
> which is static and local to the socket.c.  Instead, this adds a flag for
> private_data_is_socket.  This is an exceptionally long commit message for a
> two-line patch.

NAK.  Don't crap into struct file, please.

Re: [PATCH net-next 1/2] socket: Allow external sockets to use socket syscalls

[Alex Gartrell]

Hello Al,

On 12/26/14 2:56 PM, Al Viro wrote:
> On Thu, Dec 25, 2014 at 10:50:23PM -0800, Alex Gartrell wrote:
>> Currently the "is-socket" test for a file compares the ops table pointer,
>> which is static and local to the socket.c.  Instead, this adds a flag for
>> private_data_is_socket.  This is an exceptionally long commit message for a
>> two-line patch.
>
> NAK.  Don't crap into struct file, please.
>

I don't disagree with your sentiment here.  Is the additional f_op 
approach less gross or do you have something else in mind?

Thanks,
-- 
Alex Gartrell <agartrell@fb.com>

[PATCH net-next 2/2] tun: enable socket system calls

[Alex Gartrell]

By setting private_data to a socket and private_data_is_socket to true, we
can use the socket syscalls.  We also can't just blindly use private_data
anymore, so there's a __tun_file_get function that returns the container_of
private_data appropriately.

Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
 drivers/net/tun.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a5cbf67..b16ddc5 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -547,9 +547,18 @@ static void tun_detach_all(struct net_device *dev)
 		module_put(THIS_MODULE);
 }
 
+static struct tun_file *tun_file_from_file(struct file *file)
+{
+	struct socket *s = (struct socket *)file->private_data;
+
+	if (!s)
+		return NULL;
+	return container_of(s, struct tun_file, socket);
+}
+
 static int tun_attach(struct tun_struct *tun, struct file *file, bool skip_filter)
 {
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	int err;
 
 	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
@@ -612,7 +621,7 @@ static struct tun_struct *__tun_get(struct tun_file *tfile)
 
 static struct tun_struct *tun_get(struct file *file)
 {
-	return __tun_get(file->private_data);
+	return __tun_get(tun_file_from_file(file));
 }
 
 static void tun_put(struct tun_struct *tun)
@@ -973,7 +982,7 @@ static void tun_net_init(struct net_device *dev)
 /* Poll */
 static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
 {
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	struct tun_struct *tun = __tun_get(tfile);
 	struct sock *sk;
 	unsigned int mask = 0;
@@ -1235,7 +1244,7 @@ static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
 	struct tun_struct *tun = tun_get(file);
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	ssize_t result;
 
 	if (!tun)
@@ -1392,7 +1401,7 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
 static ssize_t tun_chr_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	struct file *file = iocb->ki_filp;
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	struct tun_struct *tun = __tun_get(tfile);
 	ssize_t len = iov_iter_count(to), ret;
 
@@ -1567,7 +1576,7 @@ static DEVICE_ATTR(group, 0444, tun_show_group, NULL);
 static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 {
 	struct tun_struct *tun;
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	struct net_device *dev;
 	int err;
 
@@ -1801,7 +1810,7 @@ static void tun_set_sndbuf(struct tun_struct *tun)
 
 static int tun_set_queue(struct file *file, struct ifreq *ifr)
 {
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	struct tun_struct *tun;
 	int ret = 0;
 
@@ -1834,7 +1843,7 @@ unlock:
 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 tun_file *tfile = tun_file_from_file(file);
 	struct tun_struct *tun;
 	void __user* argp = (void __user*)arg;
 	struct ifreq ifr;
@@ -2122,7 +2131,7 @@ static long tun_chr_compat_ioctl(struct file *file,
 
 static int tun_chr_fasync(int fd, struct file *file, int on)
 {
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	int ret;
 
 	if ((ret = fasync_helper(fd, file, on, &tfile->fasync)) < 0)
@@ -2165,7 +2174,8 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 	tfile->sk.sk_write_space = tun_sock_write_space;
 	tfile->sk.sk_sndbuf = INT_MAX;
 
-	file->private_data = tfile;
+	file->private_data = &tfile->socket;
+	file->private_data_is_socket = true;
 	set_bit(SOCK_EXTERNALLY_ALLOCATED, &tfile->socket.flags);
 	INIT_LIST_HEAD(&tfile->next);
 
@@ -2176,7 +2186,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 
 static int tun_chr_close(struct inode *inode, struct file *file)
 {
-	struct tun_file *tfile = file->private_data;
+	struct tun_file *tfile = tun_file_from_file(file);
 	struct net *net = tfile->net;
 
 	tun_detach(tfile, true);
@@ -2335,7 +2345,7 @@ struct socket *tun_get_socket(struct file *file)
 	struct tun_file *tfile;
 	if (file->f_op != &tun_fops)
 		return ERR_PTR(-EINVAL);
-	tfile = file->private_data;
+	tfile = tun_file_from_file(file);
 	if (!tfile)
 		return ERR_PTR(-EBADFD);
 	return &tfile->socket;
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH net-next 2/2] tun: enable socket system calls

[Jason Wang]

On 12/26/2014 02:50 PM, Alex Gartrell wrote:
> By setting private_data to a socket and private_data_is_socket to true, we
> can use the socket syscalls.  We also can't just blindly use private_data
> anymore, so there's a __tun_file_get function that returns the container_of
> private_data appropriately.

So this in fact expose other socket syscalls to userspace. But some of
proto_ops was not supported. E.g consider what happens if a bind() was
called for tun socket?
> Signed-off-by: Alex Gartrell <agartrell@fb.com>
> ---
>  drivers/net/tun.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index a5cbf67..b16ddc5 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -547,9 +547,18 @@ static void tun_detach_all(struct net_device *dev)
>  		module_put(THIS_MODULE);
>  }
>  
> +static struct tun_file *tun_file_from_file(struct file *file)
> +{
> +	struct socket *s = (struct socket *)file->private_data;
> +
> +	if (!s)

Can s be NULL here? If yes, why tun_get() didn't check for NULL?
> +		return NULL;
> +	return container_of(s, struct tun_file, socket);
> +}
> +
>  static int tun_attach(struct tun_struct *tun, struct file *file, bool skip_filter)
>  {
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	int err;
>  
>  	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
> @@ -612,7 +621,7 @@ static struct tun_struct *__tun_get(struct tun_file *tfile)
>  
>  static struct tun_struct *tun_get(struct file *file)
>  {
> -	return __tun_get(file->private_data);
> +	return __tun_get(tun_file_from_file(file));
>  }
>  
>  static void tun_put(struct tun_struct *tun)
> @@ -973,7 +982,7 @@ static void tun_net_init(struct net_device *dev)
>  /* Poll */
>  static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
>  {
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	struct tun_struct *tun = __tun_get(tfile);
>  	struct sock *sk;
>  	unsigned int mask = 0;
> @@ -1235,7 +1244,7 @@ static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct tun_struct *tun = tun_get(file);
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	ssize_t result;
>  
>  	if (!tun)
> @@ -1392,7 +1401,7 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
>  static ssize_t tun_chr_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
>  	struct file *file = iocb->ki_filp;
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	struct tun_struct *tun = __tun_get(tfile);
>  	ssize_t len = iov_iter_count(to), ret;
>  
> @@ -1567,7 +1576,7 @@ static DEVICE_ATTR(group, 0444, tun_show_group, NULL);
>  static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  {
>  	struct tun_struct *tun;
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	struct net_device *dev;
>  	int err;
>  
> @@ -1801,7 +1810,7 @@ static void tun_set_sndbuf(struct tun_struct *tun)
>  
>  static int tun_set_queue(struct file *file, struct ifreq *ifr)
>  {
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	struct tun_struct *tun;
>  	int ret = 0;
>  
> @@ -1834,7 +1843,7 @@ unlock:
>  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 tun_file *tfile = tun_file_from_file(file);
>  	struct tun_struct *tun;
>  	void __user* argp = (void __user*)arg;
>  	struct ifreq ifr;
> @@ -2122,7 +2131,7 @@ static long tun_chr_compat_ioctl(struct file *file,
>  
>  static int tun_chr_fasync(int fd, struct file *file, int on)
>  {
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	int ret;
>  
>  	if ((ret = fasync_helper(fd, file, on, &tfile->fasync)) < 0)
> @@ -2165,7 +2174,8 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>  	tfile->sk.sk_write_space = tun_sock_write_space;
>  	tfile->sk.sk_sndbuf = INT_MAX;
>  
> -	file->private_data = tfile;
> +	file->private_data = &tfile->socket;
> +	file->private_data_is_socket = true;
>  	set_bit(SOCK_EXTERNALLY_ALLOCATED, &tfile->socket.flags);
>  	INIT_LIST_HEAD(&tfile->next);
>  
> @@ -2176,7 +2186,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>  
>  static int tun_chr_close(struct inode *inode, struct file *file)
>  {
> -	struct tun_file *tfile = file->private_data;
> +	struct tun_file *tfile = tun_file_from_file(file);
>  	struct net *net = tfile->net;
>  
>  	tun_detach(tfile, true);
> @@ -2335,7 +2345,7 @@ struct socket *tun_get_socket(struct file *file)
>  	struct tun_file *tfile;
>  	if (file->f_op != &tun_fops)
>  		return ERR_PTR(-EINVAL);
> -	tfile = file->private_data;
> +	tfile = tun_file_from_file(file);
>  	if (!tfile)
>  		return ERR_PTR(-EBADFD);
>  	return &tfile->socket;

Re: [PATCH net-next 2/2] tun: enable socket system calls

[Alex Gartrell]

Hello Jason,

Thanks for commenting.

On 12/26/14 4:43 AM, Jason Wang wrote:
>
> On 12/26/2014 02:50 PM, Alex Gartrell wrote:
>> By setting private_data to a socket and private_data_is_socket to true, we
>> can use the socket syscalls.  We also can't just blindly use private_data
>> anymore, so there's a __tun_file_get function that returns the container_of
>> private_data appropriately.
>
> So this in fact expose other socket syscalls to userspace. But some of
> proto_ops was not supported. E.g consider what happens if a bind() was
> called for tun socket?

Yeah, I erroneously assumed that NULL => sock_no_*, but a quick glance 
assures me that that's not the case.  In this case, I'd need to 
introduce another patch that sets all of the additional ops to sock_no_*.

>> +static struct tun_file *tun_file_from_file(struct file *file)
>> +{
>> +	struct socket *s = (struct socket *)file->private_data;
>> +
>> +	if (!s)
>
> Can s be NULL here? If yes, why tun_get() didn't check for NULL?

This check is just to ensure that tun_get_socket continues to work in 
the right way when passed a file with private_data set to NULL.

Thanks,
-- 
Alex Gartrell <agartrell@fb.com>