[dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races

[dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races

[Pavel Boldin]

From: Pavel Boldin <pboldin@mirantis.com>

The `eventfd_link' module provides an API to "steal" fd from another
process had been written with a bug that leaks `struct file' because of the
extra reference counter increment and missing `fput' call.

The other bug is using another process' `task_struct' without incrementing
a reference counter.

Fix these bugs and refactor the module.
---
 examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++-------------
 1 file changed, 130 insertions(+), 106 deletions(-)

diff --git a/examples/vhost/eventfd_link/eventfd_link.c b/examples/vhost/eventfd_link/eventfd_link.c
index 69470ba..9f1f8fb 100644
--- a/examples/vhost/eventfd_link/eventfd_link.c
+++ b/examples/vhost/eventfd_link/eventfd_link.c
@@ -42,15 +42,15 @@
  * get_files_struct is copied from fs/file.c
  */
 struct files_struct *
-get_files_struct (struct task_struct *task)
+get_files_struct(struct task_struct *task)
 {
 	struct files_struct *files;
 
-	task_lock (task);
+	task_lock(task);
 	files = task->files;
 	if (files)
-		atomic_inc (&files->count);
-	task_unlock (task);
+		atomic_inc(&files->count);
+	task_unlock(task);
 
 	return files;
 }
@@ -59,117 +59,142 @@ get_files_struct (struct task_struct *task)
  * put_files_struct is extracted from fs/file.c
  */
 void
-put_files_struct (struct files_struct *files)
+put_files_struct(struct files_struct *files)
 {
-	if (atomic_dec_and_test (&files->count))
-	{
+	if (atomic_dec_and_test(&files->count))
 		BUG ();
+}
+
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
+
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+	{
+		if (file->f_mode & FMODE_PATH
+			|| !atomic_long_inc_not_zero(&file->f_count))
+		    file = NULL;
 	}
+	rcu_read_unlock();
+
+	return file;
+}
+
+static int
+close_fd(unsigned fd)
+{
+	struct file *file;
+	struct files_struct *files = current->files;
+	struct fdtable *fdt;
+
+	spin_lock(&files->file_lock);
+	fdt = files_fdtable(files);
+	if (fd >= fdt->max_fds)
+		goto out_unlock;
+	file = fdt->fd[fd];
+	if (!file)
+		goto out_unlock;
+	rcu_assign_pointer(fdt->fd[fd], NULL);
+	__clear_bit(fd, fdt->close_on_exec);
+	spin_unlock(&files->file_lock);
+	return filp_close(file, files);
+
+out_unlock:
+	spin_unlock(&files->file_lock);
+	return -EBADF;
 }
 
 
 static long
-eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long arg)
+eventfd_link_ioctl_copy(unsigned long arg)
 {
-	void __user *argp = (void __user *) arg;
+	long ret = -EFAULT;
 	struct task_struct *task_target = NULL;
-	struct file *file;
-	struct files_struct *files;
-	struct fdtable *fdt;
+	struct file *target_file = NULL;
+	struct files_struct *target_files = NULL;
 	struct eventfd_copy eventfd_copy;
+	struct pid *pid;
+
+	if (copy_from_user(&eventfd_copy, (void __user*)arg,
+			    sizeof(struct eventfd_copy)))
+		goto out;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	pid = find_vpid(eventfd_copy.target_pid);
+	if (pid == NULL) {
+		printk(KERN_INFO "Unable to find pid %d\n",
+			eventfd_copy.target_pid);
+		goto out;
+	}
+
+	task_target = get_pid_task(pid, PIDTYPE_PID);
+	if (task_target == NULL) {
+		printk(KERN_INFO "Failed to get task for pid %d\n",
+			eventfd_copy.target_pid);
+		goto out;
+	}
+
+	ret = close_fd(eventfd_copy.source_fd);
+	if (ret)
+		goto out_task;
+	ret = -EFAULT;
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	target_files = get_files_struct(task_target);
+	if (target_files == NULL) {
+		printk (KERN_INFO "Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
+
+	if (target_file == NULL) {
+		printk (KERN_INFO "Failed to get file from target pid\n");
+		goto out_target_files;
+	}
+
+
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
+
+	fd_install(eventfd_copy.source_fd, target_file);
+
+	ret = 0;
+
+out_target_files:
+	put_files_struct(target_files);
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
+
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret;
 
 	switch (ioctl)
 	{
 		case EVENTFD_COPY:
-			if (copy_from_user (&eventfd_copy, argp, sizeof (struct eventfd_copy)))
-				return -EFAULT;
-
-			/*
-			 * Find the task struct for the target pid
-			 */
-			task_target =
-				pid_task (find_vpid (eventfd_copy.target_pid), PIDTYPE_PID);
-			if (task_target == NULL)
-			{
-				printk (KERN_DEBUG "Failed to get mem ctx for target pid\n");
-				return -EFAULT;
-			}
-
-			files = get_files_struct (current);
-			if (files == NULL)
-			{
-				printk (KERN_DEBUG "Failed to get files struct\n");
-				return -EFAULT;
-			}
-
-			rcu_read_lock ();
-			file = fcheck_files (files, eventfd_copy.source_fd);
-			if (file)
-			{
-				if (file->f_mode & FMODE_PATH
-						|| !atomic_long_inc_not_zero (&file->f_count))
-					file = NULL;
-			}
-			rcu_read_unlock ();
-			put_files_struct (files);
-
-			if (file == NULL)
-			{
-				printk (KERN_DEBUG "Failed to get file from source pid\n");
-				return 0;
-			}
-
-			/*
-			 * Release the existing eventfd in the source process
-			 */
-			spin_lock (&files->file_lock);
-			fput(file);
-			filp_close (file, files);
-			fdt = files_fdtable (files);
-			fdt->fd[eventfd_copy.source_fd] = NULL;
-			spin_unlock (&files->file_lock);
-
-			/*
-			 * Find the file struct associated with the target fd.
-			 */
-
-			files = get_files_struct (task_target);
-			if (files == NULL)
-			{
-				printk (KERN_DEBUG "Failed to get files struct\n");
-				return -EFAULT;
-			}
-
-			rcu_read_lock ();
-			file = fcheck_files (files, eventfd_copy.target_fd);
-			if (file)
-			{
-				if (file->f_mode & FMODE_PATH
-						|| !atomic_long_inc_not_zero (&file->f_count))
-					file = NULL;
-			}
-			rcu_read_unlock ();
-			put_files_struct (files);
-
-			if (file == NULL)
-			{
-				printk (KERN_DEBUG "Failed to get file from target pid\n");
-				return 0;
-			}
-
-
-			/*
-			 * Install the file struct from the target process into the
-			 * file desciptor of the source process,
-			 */
-
-			fd_install (eventfd_copy.source_fd, file);
-
-			return 0;
-
+			ret = eventfd_link_ioctl_copy(arg);
+			break;
 		default:
-			return -ENOIOCTLCMD;
+			ret = -ENOIOCTLCMD;
+			break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
@@ -184,21 +209,20 @@ static struct miscdevice eventfd_link_misc = {
 };
 
 static int __init
-eventfd_link_init (void)
+eventfd_link_init(void)
 {
-	printk(KERN_INFO "eventfd_link is broken, use it at your risk\n");
-	return misc_register (&eventfd_link_misc);
+	return misc_register(&eventfd_link_misc);
 }
 
-module_init (eventfd_link_init);
+module_init(eventfd_link_init);
 
 static void __exit
-eventfd_link_exit (void)
+eventfd_link_exit(void)
 {
-	misc_deregister (&eventfd_link_misc);
+	misc_deregister(&eventfd_link_misc);
 }
 
-module_exit (eventfd_link_exit);
+module_exit(eventfd_link_exit);
 
 MODULE_VERSION ("0.0.1");
 MODULE_LICENSE ("GPL v2");
-- 
1.9.1

Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races

[Neil Horman]

On Mon, Mar 16, 2015 at 06:40:59PM +0200, Pavel Boldin wrote:
> From: Pavel Boldin <pboldin@mirantis.com>
> 
> The `eventfd_link' module provides an API to "steal" fd from another
> process had been written with a bug that leaks `struct file' because of the
> extra reference counter increment and missing `fput' call.
> 
> The other bug is using another process' `task_struct' without incrementing
> a reference counter.
> 
> Fix these bugs and refactor the module.
> ---
>  examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++-------------
>  1 file changed, 130 insertions(+), 106 deletions(-)
> 
> diff --git a/examples/vhost/eventfd_link/eventfd_link.c b/examples/vhost/eventfd_link/eventfd_link.c
> index 69470ba..9f1f8fb 100644
> --- a/examples/vhost/eventfd_link/eventfd_link.c
> +++ b/examples/vhost/eventfd_link/eventfd_link.c
> @@ -42,15 +42,15 @@
>   * get_files_struct is copied from fs/file.c
>   */
>  struct files_struct *
> -get_files_struct (struct task_struct *task)
> +get_files_struct(struct task_struct *task)
>  {
>  	struct files_struct *files;
>  
> -	task_lock (task);
> +	task_lock(task);
>  	files = task->files;
>  	if (files)
> -		atomic_inc (&files->count);
> -	task_unlock (task);
> +		atomic_inc(&files->count);
> +	task_unlock(task);
>  
>  	return files;
>  }
> @@ -59,117 +59,142 @@ get_files_struct (struct task_struct *task)
>   * put_files_struct is extracted from fs/file.c
>   */
>  void
> -put_files_struct (struct files_struct *files)
> +put_files_struct(struct files_struct *files)
>  {
> -	if (atomic_dec_and_test (&files->count))
> -	{
> +	if (atomic_dec_and_test(&files->count))
>  		BUG ();
> +}
> +
> +static struct file *
> +fget_from_files(struct files_struct *files, unsigned fd)
> +{
> +	struct file *file;
> +
> +	rcu_read_lock();
> +	file = fcheck_files(files, fd);
> +	if (file)
> +	{
> +		if (file->f_mode & FMODE_PATH
> +			|| !atomic_long_inc_not_zero(&file->f_count))
> +		    file = NULL;
>  	}
> +	rcu_read_unlock();
> +
> +	return file;
> +}
> +
> +static int
> +close_fd(unsigned fd)
> +{
> +	struct file *file;
> +	struct files_struct *files = current->files;
> +	struct fdtable *fdt;
> +
> +	spin_lock(&files->file_lock);
> +	fdt = files_fdtable(files);
> +	if (fd >= fdt->max_fds)
> +		goto out_unlock;
> +	file = fdt->fd[fd];
> +	if (!file)
> +		goto out_unlock;
> +	rcu_assign_pointer(fdt->fd[fd], NULL);
> +	__clear_bit(fd, fdt->close_on_exec);
> +	spin_unlock(&files->file_lock);
> +	return filp_close(file, files);
> +
> +out_unlock:
> +	spin_unlock(&files->file_lock);
> +	return -EBADF;
>  }
>  
>  
>  static long
> -eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long arg)
> +eventfd_link_ioctl_copy(unsigned long arg)
>  {
> -	void __user *argp = (void __user *) arg;
> +	long ret = -EFAULT;
>  	struct task_struct *task_target = NULL;
> -	struct file *file;
> -	struct files_struct *files;
> -	struct fdtable *fdt;
> +	struct file *target_file = NULL;
> +	struct files_struct *target_files = NULL;
>  	struct eventfd_copy eventfd_copy;
> +	struct pid *pid;
> +
> +	if (copy_from_user(&eventfd_copy, (void __user*)arg,
> +			    sizeof(struct eventfd_copy)))
> +		goto out;
> +
> +	/*
> +	 * Find the task struct for the target pid
> +	 */
> +	pid = find_vpid(eventfd_copy.target_pid);
> +	if (pid == NULL) {
> +		printk(KERN_INFO "Unable to find pid %d\n",
> +			eventfd_copy.target_pid);
> +		goto out;
> +	}
> +
> +	task_target = get_pid_task(pid, PIDTYPE_PID);
> +	if (task_target == NULL) {
> +		printk(KERN_INFO "Failed to get task for pid %d\n",
> +			eventfd_copy.target_pid);
> +		goto out;
> +	}
> +
> +	ret = close_fd(eventfd_copy.source_fd);
> +	if (ret)
> +		goto out_task;
> +	ret = -EFAULT;
> +
> +	/*
> +	 * Find the file struct associated with the target fd.
> +	 */
> +
> +	target_files = get_files_struct(task_target);
> +	if (target_files == NULL) {
> +		printk (KERN_INFO "Failed to get target files struct\n");
> +		goto out_task;
> +	}
> +
> +	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
> +
> +	if (target_file == NULL) {
> +		printk (KERN_INFO "Failed to get file from target pid\n");
> +		goto out_target_files;
> +	}
> +
> +
> +	/*
> +	 * Install the file struct from the target process into the
> +	 * file desciptor of the source process,
> +	 */
> +
> +	fd_install(eventfd_copy.source_fd, target_file);
> +
> +	ret = 0;
> +
> +out_target_files:
> +	put_files_struct(target_files);
> +out_task:
> +	put_task_struct(task_target);
> +out:
> +	return ret;
> +}
> +
> +static long
> +eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
> +{
> +	long ret;
>  
>  	switch (ioctl)
>  	{
>  		case EVENTFD_COPY:
> -			if (copy_from_user (&eventfd_copy, argp, sizeof (struct eventfd_copy)))
> -				return -EFAULT;
> -
> -			/*
> -			 * Find the task struct for the target pid
> -			 */
> -			task_target =
> -				pid_task (find_vpid (eventfd_copy.target_pid), PIDTYPE_PID);
> -			if (task_target == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get mem ctx for target pid\n");
> -				return -EFAULT;
> -			}
> -
> -			files = get_files_struct (current);
> -			if (files == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get files struct\n");
> -				return -EFAULT;
> -			}
> -
> -			rcu_read_lock ();
> -			file = fcheck_files (files, eventfd_copy.source_fd);
> -			if (file)
> -			{
> -				if (file->f_mode & FMODE_PATH
> -						|| !atomic_long_inc_not_zero (&file->f_count))
> -					file = NULL;
> -			}
> -			rcu_read_unlock ();
> -			put_files_struct (files);
> -
> -			if (file == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get file from source pid\n");
> -				return 0;
> -			}
> -
> -			/*
> -			 * Release the existing eventfd in the source process
> -			 */
> -			spin_lock (&files->file_lock);
> -			fput(file);
> -			filp_close (file, files);
> -			fdt = files_fdtable (files);
> -			fdt->fd[eventfd_copy.source_fd] = NULL;
> -			spin_unlock (&files->file_lock);
> -
> -			/*
> -			 * Find the file struct associated with the target fd.
> -			 */
> -
> -			files = get_files_struct (task_target);
> -			if (files == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get files struct\n");
> -				return -EFAULT;
> -			}
> -
> -			rcu_read_lock ();
> -			file = fcheck_files (files, eventfd_copy.target_fd);
> -			if (file)
> -			{
> -				if (file->f_mode & FMODE_PATH
> -						|| !atomic_long_inc_not_zero (&file->f_count))
> -					file = NULL;
> -			}
> -			rcu_read_unlock ();
> -			put_files_struct (files);
> -
> -			if (file == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get file from target pid\n");
> -				return 0;
> -			}
> -
> -
> -			/*
> -			 * Install the file struct from the target process into the
> -			 * file desciptor of the source process,
> -			 */
> -
> -			fd_install (eventfd_copy.source_fd, file);
> -
> -			return 0;
> -
> +			ret = eventfd_link_ioctl_copy(arg);
> +			break;
>  		default:
> -			return -ENOIOCTLCMD;
> +			ret = -ENOIOCTLCMD;
> +			break;
>  	}
> +
> +	return ret;
>  }
>  
>  static const struct file_operations eventfd_link_fops = {
> @@ -184,21 +209,20 @@ static struct miscdevice eventfd_link_misc = {
>  };
>  
>  static int __init
> -eventfd_link_init (void)
> +eventfd_link_init(void)
>  {
> -	printk(KERN_INFO "eventfd_link is broken, use it at your risk\n");
> -	return misc_register (&eventfd_link_misc);
> +	return misc_register(&eventfd_link_misc);
>  }
>  
> -module_init (eventfd_link_init);
> +module_init(eventfd_link_init);
>  
>  static void __exit
> -eventfd_link_exit (void)
> +eventfd_link_exit(void)
>  {
> -	misc_deregister (&eventfd_link_misc);
> +	misc_deregister(&eventfd_link_misc);
>  }
>  
> -module_exit (eventfd_link_exit);
> +module_exit(eventfd_link_exit);
>  
>  MODULE_VERSION ("0.0.1");
>  MODULE_LICENSE ("GPL v2");
> -- 
> 1.9.1
> 
> 
You need to respin this.  This library has since moved to another location in
the tree
Neil

Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pavel Boldin
> Sent: Tuesday, March 17, 2015 12:41 AM
> To: dev@dpdk.org
> Cc: Pavel Boldin
> Subject: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
> 
> From: Pavel Boldin <pboldin@mirantis.com>
> 
> The `eventfd_link' module provides an API to "steal" fd from another process
> had been written with a bug that leaks `struct file' because of the extra
> reference counter increment and missing `fput' call.
> 
> The other bug is using another process' `task_struct' without incrementing a
> reference counter.
> 
> Fix these bugs and refactor the module.
> ---
>  examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++-----
> --------
>  1 file changed, 130 insertions(+), 106 deletions(-)
> 
> diff --git a/examples/vhost/eventfd_link/eventfd_link.c
> b/examples/vhost/eventfd_link/eventfd_link.c
> index 69470ba..9f1f8fb 100644
> --- a/examples/vhost/eventfd_link/eventfd_link.c
> +++ b/examples/vhost/eventfd_link/eventfd_link.c
> @@ -42,15 +42,15 @@
>   * get_files_struct is copied from fs/file.c
>   */
>  struct files_struct *
> -get_files_struct (struct task_struct *task)
> +get_files_struct(struct task_struct *task)
>  {
>  	struct files_struct *files;
> 
> -	task_lock (task);
> +	task_lock(task);
>  	files = task->files;
>  	if (files)
> -		atomic_inc (&files->count);
> -	task_unlock (task);
> +		atomic_inc(&files->count);
> +	task_unlock(task);

I don't find any actual code change for above. 
If they are tab, space, indent refine,
I suggest it needs a separate patch named code cleanup,
Then it is easy to review.

Same for other places.

Done.
Changchun


> 
>  	return files;
>  }
> @@ -59,117 +59,142 @@ get_files_struct (struct task_struct *task)
>   * put_files_struct is extracted from fs/file.c
>   */
>  void
> -put_files_struct (struct files_struct *files)
> +put_files_struct(struct files_struct *files)
>  {
> -	if (atomic_dec_and_test (&files->count))
> -	{
> +	if (atomic_dec_and_test(&files->count))
>  		BUG ();
> +}
> +
> +static struct file *
> +fget_from_files(struct files_struct *files, unsigned fd) {
> +	struct file *file;
> +
> +	rcu_read_lock();
> +	file = fcheck_files(files, fd);
> +	if (file)
> +	{
> +		if (file->f_mode & FMODE_PATH
> +			|| !atomic_long_inc_not_zero(&file->f_count))
> +		    file = NULL;
>  	}
> +	rcu_read_unlock();
> +
> +	return file;
> +}
> +
> +static int
> +close_fd(unsigned fd)
> +{
> +	struct file *file;
> +	struct files_struct *files = current->files;
> +	struct fdtable *fdt;
> +
> +	spin_lock(&files->file_lock);
> +	fdt = files_fdtable(files);
> +	if (fd >= fdt->max_fds)
> +		goto out_unlock;
> +	file = fdt->fd[fd];
> +	if (!file)
> +		goto out_unlock;
> +	rcu_assign_pointer(fdt->fd[fd], NULL);
> +	__clear_bit(fd, fdt->close_on_exec);
> +	spin_unlock(&files->file_lock);
> +	return filp_close(file, files);
> +
> +out_unlock:
> +	spin_unlock(&files->file_lock);
> +	return -EBADF;
>  }
> 
> 
>  static long
> -eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long arg)
> +eventfd_link_ioctl_copy(unsigned long arg)
>  {
> -	void __user *argp = (void __user *) arg;
> +	long ret = -EFAULT;
>  	struct task_struct *task_target = NULL;
> -	struct file *file;
> -	struct files_struct *files;
> -	struct fdtable *fdt;
> +	struct file *target_file = NULL;
> +	struct files_struct *target_files = NULL;
>  	struct eventfd_copy eventfd_copy;
> +	struct pid *pid;
> +
> +	if (copy_from_user(&eventfd_copy, (void __user*)arg,
> +			    sizeof(struct eventfd_copy)))
> +		goto out;
> +
> +	/*
> +	 * Find the task struct for the target pid
> +	 */
> +	pid = find_vpid(eventfd_copy.target_pid);
> +	if (pid == NULL) {
> +		printk(KERN_INFO "Unable to find pid %d\n",
> +			eventfd_copy.target_pid);
> +		goto out;
> +	}
> +
> +	task_target = get_pid_task(pid, PIDTYPE_PID);
> +	if (task_target == NULL) {
> +		printk(KERN_INFO "Failed to get task for pid %d\n",
> +			eventfd_copy.target_pid);
> +		goto out;
> +	}
> +
> +	ret = close_fd(eventfd_copy.source_fd);
> +	if (ret)
> +		goto out_task;
> +	ret = -EFAULT;
> +
> +	/*
> +	 * Find the file struct associated with the target fd.
> +	 */
> +
> +	target_files = get_files_struct(task_target);
> +	if (target_files == NULL) {
> +		printk (KERN_INFO "Failed to get target files struct\n");
> +		goto out_task;
> +	}
> +
> +	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
> +
> +	if (target_file == NULL) {
> +		printk (KERN_INFO "Failed to get file from target pid\n");
> +		goto out_target_files;
> +	}
> +
> +
> +	/*
> +	 * Install the file struct from the target process into the
> +	 * file desciptor of the source process,
> +	 */
> +
> +	fd_install(eventfd_copy.source_fd, target_file);
> +
> +	ret = 0;
> +
> +out_target_files:
> +	put_files_struct(target_files);
> +out_task:
> +	put_task_struct(task_target);
> +out:
> +	return ret;
> +}
> +
> +static long
> +eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long
> +arg) {
> +	long ret;
> 
>  	switch (ioctl)
>  	{
>  		case EVENTFD_COPY:
> -			if (copy_from_user (&eventfd_copy, argp, sizeof
> (struct eventfd_copy)))
> -				return -EFAULT;
> -
> -			/*
> -			 * Find the task struct for the target pid
> -			 */
> -			task_target =
> -				pid_task (find_vpid
> (eventfd_copy.target_pid), PIDTYPE_PID);
> -			if (task_target == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get mem ctx
> for target pid\n");
> -				return -EFAULT;
> -			}
> -
> -			files = get_files_struct (current);
> -			if (files == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get files
> struct\n");
> -				return -EFAULT;
> -			}
> -
> -			rcu_read_lock ();
> -			file = fcheck_files (files, eventfd_copy.source_fd);
> -			if (file)
> -			{
> -				if (file->f_mode & FMODE_PATH
> -
> 	|| !atomic_long_inc_not_zero (&file->f_count))
> -					file = NULL;
> -			}
> -			rcu_read_unlock ();
> -			put_files_struct (files);
> -
> -			if (file == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get file from
> source pid\n");
> -				return 0;
> -			}
> -
> -			/*
> -			 * Release the existing eventfd in the source process
> -			 */
> -			spin_lock (&files->file_lock);
> -			fput(file);
> -			filp_close (file, files);
> -			fdt = files_fdtable (files);
> -			fdt->fd[eventfd_copy.source_fd] = NULL;
> -			spin_unlock (&files->file_lock);
> -
> -			/*
> -			 * Find the file struct associated with the target fd.
> -			 */
> -
> -			files = get_files_struct (task_target);
> -			if (files == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get files
> struct\n");
> -				return -EFAULT;
> -			}
> -
> -			rcu_read_lock ();
> -			file = fcheck_files (files, eventfd_copy.target_fd);
> -			if (file)
> -			{
> -				if (file->f_mode & FMODE_PATH
> -
> 	|| !atomic_long_inc_not_zero (&file->f_count))
> -					file = NULL;
> -			}
> -			rcu_read_unlock ();
> -			put_files_struct (files);
> -
> -			if (file == NULL)
> -			{
> -				printk (KERN_DEBUG "Failed to get file from
> target pid\n");
> -				return 0;
> -			}
> -
> -
> -			/*
> -			 * Install the file struct from the target process into
> the
> -			 * file desciptor of the source process,
> -			 */
> -
> -			fd_install (eventfd_copy.source_fd, file);
> -
> -			return 0;
> -
> +			ret = eventfd_link_ioctl_copy(arg);
> +			break;
>  		default:
> -			return -ENOIOCTLCMD;
> +			ret = -ENOIOCTLCMD;
> +			break;
>  	}
> +
> +	return ret;
>  }
> 
>  static const struct file_operations eventfd_link_fops = { @@ -184,21 +209,20
> @@ static struct miscdevice eventfd_link_misc = {  };
> 
>  static int __init
> -eventfd_link_init (void)
> +eventfd_link_init(void)
>  {
> -	printk(KERN_INFO "eventfd_link is broken, use it at your risk\n");
> -	return misc_register (&eventfd_link_misc);
> +	return misc_register(&eventfd_link_misc);
>  }
> 
> -module_init (eventfd_link_init);
> +module_init(eventfd_link_init);
> 
>  static void __exit
> -eventfd_link_exit (void)
> +eventfd_link_exit(void)
>  {
> -	misc_deregister (&eventfd_link_misc);
> +	misc_deregister(&eventfd_link_misc);
>  }
> 
> -module_exit (eventfd_link_exit);
> +module_exit(eventfd_link_exit);
> 
>  MODULE_VERSION ("0.0.1");
>  MODULE_LICENSE ("GPL v2");
> --
> 1.9.1

[dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races

[Pavel Boldin]

The `eventfd_link' module provides an API to "steal" fd from another
process had been written with a bug that leaks `struct file' because
of the extra reference counter increment and missing `fput' call.

The other bug is using another process' `task_struct' without incrementing
a reference counter.

Fix these bugs and refactor the module.
---

Changes since last submission:

* Rebased to the `master' version,
* Corrected error codes returned.

 lib/librte_vhost/eventfd_link/eventfd_link.c | 212 ++++++++++++++++-----------
 1 file changed, 125 insertions(+), 87 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 7755dd6..57b0a8a 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -65,100 +65,138 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
 
-static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+	{
+		if (file->f_mode & FMODE_PATH
+			|| !atomic_long_inc_not_zero(&file->f_count))
+		    file = NULL;
+	}
+	rcu_read_unlock();
+
+	return file;
+}
+
+static int
+close_fd(unsigned fd)
 {
-	void __user *argp = (void __user *) arg;
-	struct task_struct *task_target = NULL;
 	struct file *file;
-	struct files_struct *files;
+	struct files_struct *files = current->files;
 	struct fdtable *fdt;
+
+	spin_lock(&files->file_lock);
+	fdt = files_fdtable(files);
+	if (fd >= fdt->max_fds)
+		goto out_unlock;
+	file = fdt->fd[fd];
+	if (!file)
+		goto out_unlock;
+	rcu_assign_pointer(fdt->fd[fd], NULL);
+	__clear_bit(fd, fdt->close_on_exec);
+	spin_unlock(&files->file_lock);
+	return filp_close(file, files);
+
+out_unlock:
+	spin_unlock(&files->file_lock);
+	return -EBADF;
+}
+
+
+static long
+eventfd_link_ioctl_copy(unsigned long arg)
+{
+	long ret = -EFAULT;
+	struct task_struct *task_target = NULL;
+	struct file *target_file = NULL;
+	struct files_struct *target_files = NULL;
 	struct eventfd_copy eventfd_copy;
+	struct pid *pid;
+
+	if (copy_from_user(&eventfd_copy, (void __user*)arg,
+			    sizeof(struct eventfd_copy)))
+		goto out;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	ret = -ESRCH;
+
+	pid = find_vpid(eventfd_copy.target_pid);
+	if (pid == NULL) {
+		pr_info("Unable to find pid %d\n", eventfd_copy.target_pid);
+		goto out;
+	}
+
+	task_target = get_pid_task(pid, PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_info("Failed to get task for pid %d\n",
+			eventfd_copy.target_pid);
+		goto out;
+	}
+
+	ret = close_fd(eventfd_copy.source_fd);
+	if (ret)
+		goto out_task;
+	ret = -ESTALE;
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	target_files = get_files_struct(task_target);
+	if (target_files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
+
+	if (target_file == NULL) {
+		pr_info("Failed to get file from target pid\n");
+		goto out_target_files;
+	}
 
-	switch (ioctl) {
-	case EVENTFD_COPY:
-		if (copy_from_user(&eventfd_copy, argp,
-			sizeof(struct eventfd_copy)))
-			return -EFAULT;
-
-		/*
-		 * Find the task struct for the target pid
-		 */
-		task_target =
-			pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
-		if (task_target == NULL) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
-
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.source_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-				file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
-
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		filp_close(file, files);
-		fdt = files_fdtable(files);
-		fdt->fd[eventfd_copy.source_fd] = NULL;
-		spin_unlock(&files->file_lock);
-
-		/*
-		 * Find the file struct associated with the target fd.
-		 */
-
-		files = get_files_struct(task_target);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.target_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-					file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from target pid\n");
-			return 0;
-		}
-
-		/*
-		 * Install the file struct from the target process into the
-		 * file desciptor of the source process,
-		 */
-
-		fd_install(eventfd_copy.source_fd, file);
-
-		return 0;
-
-	default:
-		return -ENOIOCTLCMD;
+
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
+
+	fd_install(eventfd_copy.source_fd, target_file);
+
+	ret = 0;
+
+out_target_files:
+	put_files_struct(target_files);
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
+
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret;
+
+	switch (ioctl)
+	{
+		case EVENTFD_COPY:
+			ret = eventfd_link_ioctl_copy(arg);
+			break;
+		default:
+			ret = -ENOIOCTLCMD;
+			break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
-- 
1.9.1

Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races

[Pavel Boldin]

Changchun,

Please review the updated patch.

Pavel

On Tue, Mar 17, 2015 at 3:29 AM, Ouyang, Changchun <
changchun.ouyang@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pavel Boldin
> > Sent: Tuesday, March 17, 2015 12:41 AM
> > To: dev@dpdk.org
> > Cc: Pavel Boldin
> > Subject: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
> >
> > From: Pavel Boldin <pboldin@mirantis.com>
> >
> > The `eventfd_link' module provides an API to "steal" fd from another
> process
> > had been written with a bug that leaks `struct file' because of the extra
> > reference counter increment and missing `fput' call.
> >
> > The other bug is using another process' `task_struct' without
> incrementing a
> > reference counter.
> >
> > Fix these bugs and refactor the module.
> > ---
> >  examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++-----
> > --------
> >  1 file changed, 130 insertions(+), 106 deletions(-)
> >
> > diff --git a/examples/vhost/eventfd_link/eventfd_link.c
> > b/examples/vhost/eventfd_link/eventfd_link.c
> > index 69470ba..9f1f8fb 100644
> > --- a/examples/vhost/eventfd_link/eventfd_link.c
> > +++ b/examples/vhost/eventfd_link/eventfd_link.c
> > @@ -42,15 +42,15 @@
> >   * get_files_struct is copied from fs/file.c
> >   */
> >  struct files_struct *
> > -get_files_struct (struct task_struct *task)
> > +get_files_struct(struct task_struct *task)
> >  {
> >       struct files_struct *files;
> >
> > -     task_lock (task);
> > +     task_lock(task);
> >       files = task->files;
> >       if (files)
> > -             atomic_inc (&files->count);
> > -     task_unlock (task);
> > +             atomic_inc(&files->count);
> > +     task_unlock(task);
>
> I don't find any actual code change for above.
> If they are tab, space, indent refine,
> I suggest it needs a separate patch named code cleanup,
> Then it is easy to review.
>
> Same for other places.
>
> Done.
> Changchun
>
>
> >
> >       return files;
> >  }
> > @@ -59,117 +59,142 @@ get_files_struct (struct task_struct *task)
> >   * put_files_struct is extracted from fs/file.c
> >   */
> >  void
> > -put_files_struct (struct files_struct *files)
> > +put_files_struct(struct files_struct *files)
> >  {
> > -     if (atomic_dec_and_test (&files->count))
> > -     {
> > +     if (atomic_dec_and_test(&files->count))
> >               BUG ();
> > +}
> > +
> > +static struct file *
> > +fget_from_files(struct files_struct *files, unsigned fd) {
> > +     struct file *file;
> > +
> > +     rcu_read_lock();
> > +     file = fcheck_files(files, fd);
> > +     if (file)
> > +     {
> > +             if (file->f_mode & FMODE_PATH
> > +                     || !atomic_long_inc_not_zero(&file->f_count))
> > +                 file = NULL;
> >       }
> > +     rcu_read_unlock();
> > +
> > +     return file;
> > +}
> > +
> > +static int
> > +close_fd(unsigned fd)
> > +{
> > +     struct file *file;
> > +     struct files_struct *files = current->files;
> > +     struct fdtable *fdt;
> > +
> > +     spin_lock(&files->file_lock);
> > +     fdt = files_fdtable(files);
> > +     if (fd >= fdt->max_fds)
> > +             goto out_unlock;
> > +     file = fdt->fd[fd];
> > +     if (!file)
> > +             goto out_unlock;
> > +     rcu_assign_pointer(fdt->fd[fd], NULL);
> > +     __clear_bit(fd, fdt->close_on_exec);
> > +     spin_unlock(&files->file_lock);
> > +     return filp_close(file, files);
> > +
> > +out_unlock:
> > +     spin_unlock(&files->file_lock);
> > +     return -EBADF;
> >  }
> >
> >
> >  static long
> > -eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long
> arg)
> > +eventfd_link_ioctl_copy(unsigned long arg)
> >  {
> > -     void __user *argp = (void __user *) arg;
> > +     long ret = -EFAULT;
> >       struct task_struct *task_target = NULL;
> > -     struct file *file;
> > -     struct files_struct *files;
> > -     struct fdtable *fdt;
> > +     struct file *target_file = NULL;
> > +     struct files_struct *target_files = NULL;
> >       struct eventfd_copy eventfd_copy;
> > +     struct pid *pid;
> > +
> > +     if (copy_from_user(&eventfd_copy, (void __user*)arg,
> > +                         sizeof(struct eventfd_copy)))
> > +             goto out;
> > +
> > +     /*
> > +      * Find the task struct for the target pid
> > +      */
> > +     pid = find_vpid(eventfd_copy.target_pid);
> > +     if (pid == NULL) {
> > +             printk(KERN_INFO "Unable to find pid %d\n",
> > +                     eventfd_copy.target_pid);
> > +             goto out;
> > +     }
> > +
> > +     task_target = get_pid_task(pid, PIDTYPE_PID);
> > +     if (task_target == NULL) {
> > +             printk(KERN_INFO "Failed to get task for pid %d\n",
> > +                     eventfd_copy.target_pid);
> > +             goto out;
> > +     }
> > +
> > +     ret = close_fd(eventfd_copy.source_fd);
> > +     if (ret)
> > +             goto out_task;
> > +     ret = -EFAULT;
> > +
> > +     /*
> > +      * Find the file struct associated with the target fd.
> > +      */
> > +
> > +     target_files = get_files_struct(task_target);
> > +     if (target_files == NULL) {
> > +             printk (KERN_INFO "Failed to get target files struct\n");
> > +             goto out_task;
> > +     }
> > +
> > +     target_file = fget_from_files(target_files,
> eventfd_copy.target_fd);
> > +
> > +     if (target_file == NULL) {
> > +             printk (KERN_INFO "Failed to get file from target pid\n");
> > +             goto out_target_files;
> > +     }
> > +
> > +
> > +     /*
> > +      * Install the file struct from the target process into the
> > +      * file desciptor of the source process,
> > +      */
> > +
> > +     fd_install(eventfd_copy.source_fd, target_file);
> > +
> > +     ret = 0;
> > +
> > +out_target_files:
> > +     put_files_struct(target_files);
> > +out_task:
> > +     put_task_struct(task_target);
> > +out:
> > +     return ret;
> > +}
> > +
> > +static long
> > +eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long
> > +arg) {
> > +     long ret;
> >
> >       switch (ioctl)
> >       {
> >               case EVENTFD_COPY:
> > -                     if (copy_from_user (&eventfd_copy, argp, sizeof
> > (struct eventfd_copy)))
> > -                             return -EFAULT;
> > -
> > -                     /*
> > -                      * Find the task struct for the target pid
> > -                      */
> > -                     task_target =
> > -                             pid_task (find_vpid
> > (eventfd_copy.target_pid), PIDTYPE_PID);
> > -                     if (task_target == NULL)
> > -                     {
> > -                             printk (KERN_DEBUG "Failed to get mem ctx
> > for target pid\n");
> > -                             return -EFAULT;
> > -                     }
> > -
> > -                     files = get_files_struct (current);
> > -                     if (files == NULL)
> > -                     {
> > -                             printk (KERN_DEBUG "Failed to get files
> > struct\n");
> > -                             return -EFAULT;
> > -                     }
> > -
> > -                     rcu_read_lock ();
> > -                     file = fcheck_files (files,
> eventfd_copy.source_fd);
> > -                     if (file)
> > -                     {
> > -                             if (file->f_mode & FMODE_PATH
> > -
> >       || !atomic_long_inc_not_zero (&file->f_count))
> > -                                     file = NULL;
> > -                     }
> > -                     rcu_read_unlock ();
> > -                     put_files_struct (files);
> > -
> > -                     if (file == NULL)
> > -                     {
> > -                             printk (KERN_DEBUG "Failed to get file from
> > source pid\n");
> > -                             return 0;
> > -                     }
> > -
> > -                     /*
> > -                      * Release the existing eventfd in the source
> process
> > -                      */
> > -                     spin_lock (&files->file_lock);
> > -                     fput(file);
> > -                     filp_close (file, files);
> > -                     fdt = files_fdtable (files);
> > -                     fdt->fd[eventfd_copy.source_fd] = NULL;
> > -                     spin_unlock (&files->file_lock);
> > -
> > -                     /*
> > -                      * Find the file struct associated with the target
> fd.
> > -                      */
> > -
> > -                     files = get_files_struct (task_target);
> > -                     if (files == NULL)
> > -                     {
> > -                             printk (KERN_DEBUG "Failed to get files
> > struct\n");
> > -                             return -EFAULT;
> > -                     }
> > -
> > -                     rcu_read_lock ();
> > -                     file = fcheck_files (files,
> eventfd_copy.target_fd);
> > -                     if (file)
> > -                     {
> > -                             if (file->f_mode & FMODE_PATH
> > -
> >       || !atomic_long_inc_not_zero (&file->f_count))
> > -                                     file = NULL;
> > -                     }
> > -                     rcu_read_unlock ();
> > -                     put_files_struct (files);
> > -
> > -                     if (file == NULL)
> > -                     {
> > -                             printk (KERN_DEBUG "Failed to get file from
> > target pid\n");
> > -                             return 0;
> > -                     }
> > -
> > -
> > -                     /*
> > -                      * Install the file struct from the target process
> into
> > the
> > -                      * file desciptor of the source process,
> > -                      */
> > -
> > -                     fd_install (eventfd_copy.source_fd, file);
> > -
> > -                     return 0;
> > -
> > +                     ret = eventfd_link_ioctl_copy(arg);
> > +                     break;
> >               default:
> > -                     return -ENOIOCTLCMD;
> > +                     ret = -ENOIOCTLCMD;
> > +                     break;
> >       }
> > +
> > +     return ret;
> >  }
> >
> >  static const struct file_operations eventfd_link_fops = { @@ -184,21
> +209,20
> > @@ static struct miscdevice eventfd_link_misc = {  };
> >
> >  static int __init
> > -eventfd_link_init (void)
> > +eventfd_link_init(void)
> >  {
> > -     printk(KERN_INFO "eventfd_link is broken, use it at your risk\n");
> > -     return misc_register (&eventfd_link_misc);
> > +     return misc_register(&eventfd_link_misc);
> >  }
> >
> > -module_init (eventfd_link_init);
> > +module_init(eventfd_link_init);
> >
> >  static void __exit
> > -eventfd_link_exit (void)
> > +eventfd_link_exit(void)
> >  {
> > -     misc_deregister (&eventfd_link_misc);
> > +     misc_deregister(&eventfd_link_misc);
> >  }
> >
> > -module_exit (eventfd_link_exit);
> > +module_exit(eventfd_link_exit);
> >
> >  MODULE_VERSION ("0.0.1");
> >  MODULE_LICENSE ("GPL v2");
> > --
> > 1.9.1
>
>

Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races

[Thomas Monjalon]

Hi Pavel,

You forgot the Signed-off-by line.

Huawei, Changchun, any comment on this patch?

2015-03-18 15:16, Pavel Boldin:
> The `eventfd_link' module provides an API to "steal" fd from another
> process had been written with a bug that leaks `struct file' because
> of the extra reference counter increment and missing `fput' call.
> 
> The other bug is using another process' `task_struct' without incrementing
> a reference counter.

As there are 2 bugs, you should provide 2 patches.

> Fix these bugs and refactor the module.

Why refactoring along with the bug fixes?
You'd have more chances to have your fixes integrated in 2.0 without
refactoring.

Thanks

Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races

[Pavel Boldin]

Hi Thomas,

There is by far more than 2 issues, but these are the only ones that
appeared to us.

The list of the issues that motivated the refactoring:

   1. Only one error code for every fault (-EFAULT).
   2. Copy-paste code from the `fget' function.
   3. Ambiguous variable names. The `files' variable mean different things
   under the same block. This ruins readability of the code.
   4. Overall placing of the all the code inside one `switch/case'. Modern
   kernel code style suggests placing these in the inline functions.
   5. Usage of the copy-pasted `close_fd' function code. (I really should
   use `sys_close' here though).


I can rewrite the refactoring in a set of small patches if this will aid
reviewers.

The dangerous bug that definitely must be fixed before the next release is
the `struct file*' leakage. I can provide a simple patch for this as a
separate submission and continue working on the refactoring patches. Is
this plan OK for you?


Pavel

On Mon, Mar 23, 2015 at 1:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> Hi Pavel,
>
> You forgot the Signed-off-by line.
>
> Huawei, Changchun, any comment on this patch?
>
> 2015-03-18 15:16, Pavel Boldin:
> > The `eventfd_link' module provides an API to "steal" fd from another
> > process had been written with a bug that leaks `struct file' because
> > of the extra reference counter increment and missing `fput' call.
> >
> > The other bug is using another process' `task_struct' without
> incrementing
> > a reference counter.
>
> As there are 2 bugs, you should provide 2 patches.
>
> > Fix these bugs and refactor the module.
>
> Why refactoring along with the bug fixes?
> You'd have more chances to have your fixes integrated in 2.0 without
> refactoring.
>
> Thanks
>
>
>
>

Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races

[Thomas Monjalon]

2015-03-23 13:36, Pavel Boldin:
> Hi Thomas,
> 
> There is by far more than 2 issues, but these are the only ones that
> appeared to us.
> 
> The list of the issues that motivated the refactoring:
> 
>    1. Only one error code for every fault (-EFAULT).
>    2. Copy-paste code from the `fget' function.
>    3. Ambiguous variable names. The `files' variable mean different things
>    under the same block. This ruins readability of the code.
>    4. Overall placing of the all the code inside one `switch/case'. Modern
>    kernel code style suggests placing these in the inline functions.
>    5. Usage of the copy-pasted `close_fd' function code. (I really should
>    use `sys_close' here though).
> 
> 
> I can rewrite the refactoring in a set of small patches if this will aid
> reviewers.
> 
> The dangerous bug that definitely must be fixed before the next release is
> the `struct file*' leakage. I can provide a simple patch for this as a
> separate submission and continue working on the refactoring patches. Is
> this plan OK for you?

Yes, please separate important fixes and refactoring.
Then we'll be able to merge the fixes in 2.0.

PS: please answer below the question to ease thread reading.

Thanks


> On Mon, Mar 23, 2015 at 1:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > Hi Pavel,
> >
> > You forgot the Signed-off-by line.
> >
> > Huawei, Changchun, any comment on this patch?
> >
> > 2015-03-18 15:16, Pavel Boldin:
> > > The `eventfd_link' module provides an API to "steal" fd from another
> > > process had been written with a bug that leaks `struct file' because
> > > of the extra reference counter increment and missing `fput' call.
> > >
> > > The other bug is using another process' `task_struct' without
> > incrementing
> > > a reference counter.
> >
> > As there are 2 bugs, you should provide 2 patches.
> >
> > > Fix these bugs and refactor the module.
> >
> > Why refactoring along with the bug fixes?
> > You'd have more chances to have your fixes integrated in 2.0 without
> > refactoring.
> >
> > Thanks

[dpdk-dev] [PATCH v3] vhost: Refactor module `eventfd_link'

[Pavel Boldin]

Changes:
 * Remove unnecessary #include's.
 * Deindent by moving the code to an inline function.
 * Fix return codes. Use appropriate return code for each fault cause.
 * Remove copy-pasted `close_fd', call `sys_close' instead.
 * Use `get_pid_task' to correctly reference the `task_target'.

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
Changes since last submission:
 * Using `sys_close' to close fd.
 * Removing unnecessary #include's.

 lib/librte_vhost/eventfd_link/eventfd_link.c | 193 ++++++++++++++-------------
 1 file changed, 98 insertions(+), 95 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 7755dd6..d10e25f 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -22,18 +22,11 @@
  *   Intel Corporation
  */
 
-#include <linux/eventfd.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/rcupdate.h>
 #include <linux/file.h>
-#include <linux/slab.h>
-#include <linux/fs.h>
-#include <linux/mmu_context.h>
-#include <linux/sched.h>
-#include <asm/mmu_context.h>
 #include <linux/fdtable.h>
+#include <linux/syscalls.h>
 
 #include "eventfd_link.h"
 
@@ -65,100 +58,110 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
 
-static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+	{
+		if (file->f_mode & FMODE_PATH
+			|| !atomic_long_inc_not_zero(&file->f_count))
+		    file = NULL;
+	}
+	rcu_read_unlock();
+
+	return file;
+}
+
+static inline long
+eventfd_link_ioctl_copy(unsigned long arg)
 {
-	void __user *argp = (void __user *) arg;
+	long ret = -EFAULT;
 	struct task_struct *task_target = NULL;
-	struct file *file;
-	struct files_struct *files;
-	struct fdtable *fdt;
+	struct file *target_file = NULL;
+	struct files_struct *target_files = NULL;
 	struct eventfd_copy eventfd_copy;
+	struct pid *pid;
+
+	if (copy_from_user(&eventfd_copy, (void __user*)arg,
+			    sizeof(struct eventfd_copy)))
+		goto out;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	ret = -ESRCH;
+
+	pid = find_vpid(eventfd_copy.target_pid);
+	if (pid == NULL) {
+		pr_info("Unable to find pid %d\n", eventfd_copy.target_pid);
+		goto out;
+	}
+
+	task_target = get_pid_task(pid, PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_info("Failed to get task for pid %d\n",
+			eventfd_copy.target_pid);
+		goto out;
+	}
+
+	ret = sys_close(eventfd_copy.source_fd);
+	if (ret)
+		goto out_task;
+	ret = -ESTALE;
 
-	switch (ioctl) {
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	target_files = get_files_struct(task_target);
+	if (target_files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
+
+	if (target_file == NULL) {
+		pr_info("Failed to get file from target pid\n");
+		goto out_target_files;
+	}
+
+
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
+
+	fd_install(eventfd_copy.source_fd, target_file);
+
+	ret = 0;
+
+out_target_files:
+	put_files_struct(target_files);
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
+
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret = -ENOIOCTLCMD;
+
+	switch (ioctl)
+	{
 	case EVENTFD_COPY:
-		if (copy_from_user(&eventfd_copy, argp,
-			sizeof(struct eventfd_copy)))
-			return -EFAULT;
-
-		/*
-		 * Find the task struct for the target pid
-		 */
-		task_target =
-			pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
-		if (task_target == NULL) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
-
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.source_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-				file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
-
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		filp_close(file, files);
-		fdt = files_fdtable(files);
-		fdt->fd[eventfd_copy.source_fd] = NULL;
-		spin_unlock(&files->file_lock);
-
-		/*
-		 * Find the file struct associated with the target fd.
-		 */
-
-		files = get_files_struct(task_target);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.target_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-					file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from target pid\n");
-			return 0;
-		}
-
-		/*
-		 * Install the file struct from the target process into the
-		 * file desciptor of the source process,
-		 */
-
-		fd_install(eventfd_copy.source_fd, file);
-
-		return 0;
-
-	default:
-		return -ENOIOCTLCMD;
+		ret = eventfd_link_ioctl_copy(arg);
+		break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
-- 
1.9.1

[dpdk-dev] [PATCH v4 0/5] Refactor module `eventfd_link'

[Pavel Boldin]

This patchset contains refactoring steps for the `eventfd_link' module
of the DPDK's `librte_vhost' part.

Pavel Boldin (5):
  vhost: eventfd_link: moving ioctl to a function
  vhost: eventfd_link: add function fget_from_files
  vhost: eventfd_link: fix ioctl return values
  vhost: eventfd_link: replace copy-pasted sys_close
  vhost: eventfd_link: removing extra #includes

 lib/librte_vhost/eventfd_link/eventfd_link.c | 181 +++++++++++++--------------
 1 file changed, 87 insertions(+), 94 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function

[Pavel Boldin]

Move ioctl `EVENTFD_COPY' handler code to an inline function.
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 171 ++++++++++++++-------------
 1 file changed, 89 insertions(+), 82 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 62c45c8..d7cb81f 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -65,9 +65,8 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
-
-static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+static inline long
+eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
 	struct task_struct *task_target = NULL;
@@ -76,90 +75,98 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
 	struct fdtable *fdt;
 	struct eventfd_copy eventfd_copy;
 
-	switch (ioctl) {
-	case EVENTFD_COPY:
-		if (copy_from_user(&eventfd_copy, argp,
-			sizeof(struct eventfd_copy)))
-			return -EFAULT;
-
-		/*
-		 * Find the task struct for the target pid
-		 */
-		task_target =
-			pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
-		if (task_target == NULL) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
-
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.source_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
+	if (copy_from_user(&eventfd_copy, argp,
+		sizeof(struct eventfd_copy)))
+		return -EFAULT;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	task_target =
+		pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_debug("Failed to get mem ctx for target pid\n");
+		return -EFAULT;
+	}
+
+	files = get_files_struct(current);
+	if (files == NULL) {
+		pr_debug("Failed to get files struct\n");
+		return -EFAULT;
+	}
+
+	rcu_read_lock();
+	file = fcheck_files(files, eventfd_copy.source_fd);
+	if (file) {
+		if (file->f_mode & FMODE_PATH ||
+			!atomic_long_inc_not_zero(&file->f_count))
+			file = NULL;
+	}
+	rcu_read_unlock();
+	put_files_struct(files);
+
+	if (file == NULL) {
+		pr_debug("Failed to get file from source pid\n");
+		return 0;
+	}
+
+	/*
+	 * Release the existing eventfd in the source process
+	 */
+	spin_lock(&files->file_lock);
+	fput(file);
+	filp_close(file, files);
+	fdt = files_fdtable(files);
+	fdt->fd[eventfd_copy.source_fd] = NULL;
+	spin_unlock(&files->file_lock);
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	files = get_files_struct(task_target);
+	if (files == NULL) {
+		pr_debug("Failed to get files struct\n");
+		return -EFAULT;
+	}
+
+	rcu_read_lock();
+	file = fcheck_files(files, eventfd_copy.target_fd);
+	if (file) {
+		if (file->f_mode & FMODE_PATH ||
+			!atomic_long_inc_not_zero(&file->f_count))
 				file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
-
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		fput(file);
-		filp_close(file, files);
-		fdt = files_fdtable(files);
-		fdt->fd[eventfd_copy.source_fd] = NULL;
-		spin_unlock(&files->file_lock);
-
-		/*
-		 * Find the file struct associated with the target fd.
-		 */
-
-		files = get_files_struct(task_target);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.target_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-					file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from target pid\n");
-			return 0;
-		}
-
-		/*
-		 * Install the file struct from the target process into the
-		 * file desciptor of the source process,
-		 */
-
-		fd_install(eventfd_copy.source_fd, file);
+	}
+	rcu_read_unlock();
+	put_files_struct(files);
 
+	if (file == NULL) {
+		pr_debug("Failed to get file from target pid\n");
 		return 0;
+	}
+
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
+
+	fd_install(eventfd_copy.source_fd, file);
+
+	return 0;
+}
 
-	default:
-		return -ENOIOCTLCMD;
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret = -ENOIOCTLCMD;
+
+	switch (ioctl) {
+	case EVENTFD_COPY:
+		ret = eventfd_link_ioctl_copy(arg);
+		break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
-- 
1.9.1

[dpdk-dev] [PATCH v4 2/5] vhost: eventfd_link: add function fget_from_files

[Pavel Boldin]

Move copy-pasted code of `fget' for different `struct files' to
the added `fget_from_files' function.
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 36 +++++++++++++++-------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index d7cb81f..2a29999 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -65,6 +65,24 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
+
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+	{
+		if (file->f_mode & FMODE_PATH
+			|| !atomic_long_inc_not_zero(&file->f_count))
+		    file = NULL;
+	}
+	rcu_read_unlock();
+
+	return file;
+}
+
 static inline long
 eventfd_link_ioctl_copy(unsigned long arg)
 {
@@ -95,14 +113,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
 		return -EFAULT;
 	}
 
-	rcu_read_lock();
-	file = fcheck_files(files, eventfd_copy.source_fd);
-	if (file) {
-		if (file->f_mode & FMODE_PATH ||
-			!atomic_long_inc_not_zero(&file->f_count))
-			file = NULL;
-	}
-	rcu_read_unlock();
+	file = fget_from_files(files, eventfd_copy.source_fd);
 	put_files_struct(files);
 
 	if (file == NULL) {
@@ -130,14 +141,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
 		return -EFAULT;
 	}
 
-	rcu_read_lock();
-	file = fcheck_files(files, eventfd_copy.target_fd);
-	if (file) {
-		if (file->f_mode & FMODE_PATH ||
-			!atomic_long_inc_not_zero(&file->f_count))
-				file = NULL;
-	}
-	rcu_read_unlock();
+	file = fget_from_files(files, eventfd_copy.target_fd);
 	put_files_struct(files);
 
 	if (file == NULL) {
-- 
1.9.1

[dpdk-dev] [PATCH v4 3/5] vhost: eventfd_link: fix ioctl return values

[Pavel Boldin]

Fix ioctl return values:
 * `-EFAULT' when unable to fetch user supplied arguments,
 * `-ESRCH' when no target process is found,
 * `-ESTALE' when unable to get `struct files',
 * `-EBADF' when unable to get `struct file' for fd.
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 46 ++++++++++++++++++----------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 2a29999..0a06594 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -92,33 +92,38 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	struct files_struct *files;
 	struct fdtable *fdt;
 	struct eventfd_copy eventfd_copy;
+	long ret = -EFAULT;
 
-	if (copy_from_user(&eventfd_copy, argp,
-		sizeof(struct eventfd_copy)))
-		return -EFAULT;
+	if (copy_from_user(&eventfd_copy, argp, sizeof(struct eventfd_copy)))
+		goto out;
 
 	/*
 	 * Find the task struct for the target pid
 	 */
+	ret = -ESRCH;
+
 	task_target =
-		pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
+		get_pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
 	if (task_target == NULL) {
-		pr_debug("Failed to get mem ctx for target pid\n");
-		return -EFAULT;
+		pr_info("Unable to find pid %d\n", eventfd_copy.target_pid);
+		goto out;
 	}
 
+	ret = -ESTALE;
 	files = get_files_struct(current);
 	if (files == NULL) {
-		pr_debug("Failed to get files struct\n");
-		return -EFAULT;
+		pr_info("Failed to get current files struct\n");
+		goto out_task;
 	}
 
+	ret = -EBADF;
 	file = fget_from_files(files, eventfd_copy.source_fd);
-	put_files_struct(files);
 
 	if (file == NULL) {
-		pr_debug("Failed to get file from source pid\n");
-		return 0;
+		pr_info("Failed to get fd %d from source\n",
+			eventfd_copy.source_fd);
+		put_files_struct(files);
+		goto out_task;
 	}
 
 	/*
@@ -131,22 +136,27 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	fdt->fd[eventfd_copy.source_fd] = NULL;
 	spin_unlock(&files->file_lock);
 
+	put_files_struct(files);
+
 	/*
 	 * Find the file struct associated with the target fd.
 	 */
 
+	ret = -ESTALE;
 	files = get_files_struct(task_target);
 	if (files == NULL) {
-		pr_debug("Failed to get files struct\n");
-		return -EFAULT;
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
 	}
 
+	ret = -EBADF;
 	file = fget_from_files(files, eventfd_copy.target_fd);
 	put_files_struct(files);
 
 	if (file == NULL) {
-		pr_debug("Failed to get file from target pid\n");
-		return 0;
+		pr_info("Failed to get fd %d from target\n",
+			eventfd_copy.target_fd);
+		goto out_task;
 	}
 
 	/*
@@ -155,8 +165,12 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	 */
 
 	fd_install(eventfd_copy.source_fd, file);
+	ret = 0;
 
-	return 0;
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
 }
 
 static long
-- 
1.9.1

[dpdk-dev] [PATCH v4 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Pavel Boldin]

Replace copy-pasted `fget_from_files' -> `filp_close' with
a `sys_close' call.
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++---------------------
 1 file changed, 12 insertions(+), 37 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 0a06594..9bc52a3 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -88,9 +88,8 @@ eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
 	struct task_struct *task_target = NULL;
-	struct file *file;
-	struct files_struct *files;
-	struct fdtable *fdt;
+	struct file *target_file;
+	struct files_struct *target_files;
 	struct eventfd_copy eventfd_copy;
 	long ret = -EFAULT;
 
@@ -109,51 +108,27 @@ eventfd_link_ioctl_copy(unsigned long arg)
 		goto out;
 	}
 
-	ret = -ESTALE;
-	files = get_files_struct(current);
-	if (files == NULL) {
-		pr_info("Failed to get current files struct\n");
-		goto out_task;
-	}
-
-	ret = -EBADF;
-	file = fget_from_files(files, eventfd_copy.source_fd);
-
-	if (file == NULL) {
-		pr_info("Failed to get fd %d from source\n",
-			eventfd_copy.source_fd);
-		put_files_struct(files);
+	/* Closing the source_fd */
+	ret = sys_close(eventfd_copy.source_fd);
+	if (ret)
 		goto out_task;
-	}
-
-	/*
-	 * Release the existing eventfd in the source process
-	 */
-	spin_lock(&files->file_lock);
-	fput(file);
-	filp_close(file, files);
-	fdt = files_fdtable(files);
-	fdt->fd[eventfd_copy.source_fd] = NULL;
-	spin_unlock(&files->file_lock);
-
-	put_files_struct(files);
+	ret = -ESTALE;
 
 	/*
 	 * Find the file struct associated with the target fd.
 	 */
 
-	ret = -ESTALE;
-	files = get_files_struct(task_target);
-	if (files == NULL) {
+	target_files = get_files_struct(task_target);
+	if (target_files == NULL) {
 		pr_info("Failed to get target files struct\n");
 		goto out_task;
 	}
 
 	ret = -EBADF;
-	file = fget_from_files(files, eventfd_copy.target_fd);
-	put_files_struct(files);
+	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
+	put_files_struct(target_files);
 
-	if (file == NULL) {
+	if (target_file == NULL) {
 		pr_info("Failed to get fd %d from target\n",
 			eventfd_copy.target_fd);
 		goto out_task;
@@ -164,7 +139,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	 * file desciptor of the source process,
 	 */
 
-	fd_install(eventfd_copy.source_fd, file);
+	fd_install(eventfd_copy.source_fd, target_file);
 	ret = 0;
 
 out_task:
-- 
1.9.1

[dpdk-dev] [PATCH v4 5/5] vhost: eventfd_link: removing extra #includes

[Pavel Boldin]

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 9bc52a3..0ee7357 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -22,18 +22,11 @@
  *   Intel Corporation
  */
 
-#include <linux/eventfd.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/rcupdate.h>
 #include <linux/file.h>
-#include <linux/slab.h>
-#include <linux/fs.h>
-#include <linux/mmu_context.h>
-#include <linux/sched.h>
-#include <asm/mmu_context.h>
 #include <linux/fdtable.h>
+#include <linux/syscalls.h>
 
 #include "eventfd_link.h"
 
-- 
1.9.1

[dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link'

[Pavel Boldin]

This patchset contains refactoring steps for the `eventfd_link' module
of the DPDK's `librte_vhost' part.

The commit messages are updated to include `Signed-off-by'.

Pavel Boldin (5):
  vhost: eventfd_link: moving ioctl to a function
  vhost: eventfd_link: add function fget_from_files
  vhost: eventfd_link: fix ioctl return values
  vhost: eventfd_link: replace copy-pasted sys_close
  vhost: eventfd_link: removing extra #includes

 lib/librte_vhost/eventfd_link/eventfd_link.c | 181 +++++++++++++--------------
 1 file changed, 87 insertions(+), 94 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function

[Pavel Boldin]

Move ioctl `EVENTFD_COPY' handler code to an inline function.

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 171 ++++++++++++++-------------
 1 file changed, 89 insertions(+), 82 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 62c45c8..d7cb81f 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -65,9 +65,8 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
-
-static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+static inline long
+eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
 	struct task_struct *task_target = NULL;
@@ -76,90 +75,98 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
 	struct fdtable *fdt;
 	struct eventfd_copy eventfd_copy;
 
-	switch (ioctl) {
-	case EVENTFD_COPY:
-		if (copy_from_user(&eventfd_copy, argp,
-			sizeof(struct eventfd_copy)))
-			return -EFAULT;
-
-		/*
-		 * Find the task struct for the target pid
-		 */
-		task_target =
-			pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
-		if (task_target == NULL) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
-
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.source_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
+	if (copy_from_user(&eventfd_copy, argp,
+		sizeof(struct eventfd_copy)))
+		return -EFAULT;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	task_target =
+		pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_debug("Failed to get mem ctx for target pid\n");
+		return -EFAULT;
+	}
+
+	files = get_files_struct(current);
+	if (files == NULL) {
+		pr_debug("Failed to get files struct\n");
+		return -EFAULT;
+	}
+
+	rcu_read_lock();
+	file = fcheck_files(files, eventfd_copy.source_fd);
+	if (file) {
+		if (file->f_mode & FMODE_PATH ||
+			!atomic_long_inc_not_zero(&file->f_count))
+			file = NULL;
+	}
+	rcu_read_unlock();
+	put_files_struct(files);
+
+	if (file == NULL) {
+		pr_debug("Failed to get file from source pid\n");
+		return 0;
+	}
+
+	/*
+	 * Release the existing eventfd in the source process
+	 */
+	spin_lock(&files->file_lock);
+	fput(file);
+	filp_close(file, files);
+	fdt = files_fdtable(files);
+	fdt->fd[eventfd_copy.source_fd] = NULL;
+	spin_unlock(&files->file_lock);
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	files = get_files_struct(task_target);
+	if (files == NULL) {
+		pr_debug("Failed to get files struct\n");
+		return -EFAULT;
+	}
+
+	rcu_read_lock();
+	file = fcheck_files(files, eventfd_copy.target_fd);
+	if (file) {
+		if (file->f_mode & FMODE_PATH ||
+			!atomic_long_inc_not_zero(&file->f_count))
 				file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
-
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		fput(file);
-		filp_close(file, files);
-		fdt = files_fdtable(files);
-		fdt->fd[eventfd_copy.source_fd] = NULL;
-		spin_unlock(&files->file_lock);
-
-		/*
-		 * Find the file struct associated with the target fd.
-		 */
-
-		files = get_files_struct(task_target);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.target_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-					file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
-
-		if (file == NULL) {
-			pr_debug("Failed to get file from target pid\n");
-			return 0;
-		}
-
-		/*
-		 * Install the file struct from the target process into the
-		 * file desciptor of the source process,
-		 */
-
-		fd_install(eventfd_copy.source_fd, file);
+	}
+	rcu_read_unlock();
+	put_files_struct(files);
 
+	if (file == NULL) {
+		pr_debug("Failed to get file from target pid\n");
 		return 0;
+	}
+
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
+
+	fd_install(eventfd_copy.source_fd, file);
+
+	return 0;
+}
 
-	default:
-		return -ENOIOCTLCMD;
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret = -ENOIOCTLCMD;
+
+	switch (ioctl) {
+	case EVENTFD_COPY:
+		ret = eventfd_link_ioctl_copy(arg);
+		break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
-- 
1.9.1

[dpdk-dev] [PATCH v5 2/5] vhost: eventfd_link: add function fget_from_files

[Pavel Boldin]

Move copy-pasted code of `fget' for different `struct files' to
the added `fget_from_files' function.

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 36 +++++++++++++++-------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index d7cb81f..2a29999 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -65,6 +65,24 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
+
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+	{
+		if (file->f_mode & FMODE_PATH
+			|| !atomic_long_inc_not_zero(&file->f_count))
+		    file = NULL;
+	}
+	rcu_read_unlock();
+
+	return file;
+}
+
 static inline long
 eventfd_link_ioctl_copy(unsigned long arg)
 {
@@ -95,14 +113,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
 		return -EFAULT;
 	}
 
-	rcu_read_lock();
-	file = fcheck_files(files, eventfd_copy.source_fd);
-	if (file) {
-		if (file->f_mode & FMODE_PATH ||
-			!atomic_long_inc_not_zero(&file->f_count))
-			file = NULL;
-	}
-	rcu_read_unlock();
+	file = fget_from_files(files, eventfd_copy.source_fd);
 	put_files_struct(files);
 
 	if (file == NULL) {
@@ -130,14 +141,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
 		return -EFAULT;
 	}
 
-	rcu_read_lock();
-	file = fcheck_files(files, eventfd_copy.target_fd);
-	if (file) {
-		if (file->f_mode & FMODE_PATH ||
-			!atomic_long_inc_not_zero(&file->f_count))
-				file = NULL;
-	}
-	rcu_read_unlock();
+	file = fget_from_files(files, eventfd_copy.target_fd);
 	put_files_struct(files);
 
 	if (file == NULL) {
-- 
1.9.1

[dpdk-dev] [PATCH v5 3/5] vhost: eventfd_link: fix ioctl return values

[Pavel Boldin]

Fix ioctl return values:
 * `-EFAULT' when unable to fetch user supplied arguments,
 * `-ESRCH' when no target process is found,
 * `-ESTALE' when unable to get `struct files',
 * `-EBADF' when unable to get `struct file' for fd.

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 46 ++++++++++++++++++----------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 2a29999..0a06594 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -92,33 +92,38 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	struct files_struct *files;
 	struct fdtable *fdt;
 	struct eventfd_copy eventfd_copy;
+	long ret = -EFAULT;
 
-	if (copy_from_user(&eventfd_copy, argp,
-		sizeof(struct eventfd_copy)))
-		return -EFAULT;
+	if (copy_from_user(&eventfd_copy, argp, sizeof(struct eventfd_copy)))
+		goto out;
 
 	/*
 	 * Find the task struct for the target pid
 	 */
+	ret = -ESRCH;
+
 	task_target =
-		pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
+		get_pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
 	if (task_target == NULL) {
-		pr_debug("Failed to get mem ctx for target pid\n");
-		return -EFAULT;
+		pr_info("Unable to find pid %d\n", eventfd_copy.target_pid);
+		goto out;
 	}
 
+	ret = -ESTALE;
 	files = get_files_struct(current);
 	if (files == NULL) {
-		pr_debug("Failed to get files struct\n");
-		return -EFAULT;
+		pr_info("Failed to get current files struct\n");
+		goto out_task;
 	}
 
+	ret = -EBADF;
 	file = fget_from_files(files, eventfd_copy.source_fd);
-	put_files_struct(files);
 
 	if (file == NULL) {
-		pr_debug("Failed to get file from source pid\n");
-		return 0;
+		pr_info("Failed to get fd %d from source\n",
+			eventfd_copy.source_fd);
+		put_files_struct(files);
+		goto out_task;
 	}
 
 	/*
@@ -131,22 +136,27 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	fdt->fd[eventfd_copy.source_fd] = NULL;
 	spin_unlock(&files->file_lock);
 
+	put_files_struct(files);
+
 	/*
 	 * Find the file struct associated with the target fd.
 	 */
 
+	ret = -ESTALE;
 	files = get_files_struct(task_target);
 	if (files == NULL) {
-		pr_debug("Failed to get files struct\n");
-		return -EFAULT;
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
 	}
 
+	ret = -EBADF;
 	file = fget_from_files(files, eventfd_copy.target_fd);
 	put_files_struct(files);
 
 	if (file == NULL) {
-		pr_debug("Failed to get file from target pid\n");
-		return 0;
+		pr_info("Failed to get fd %d from target\n",
+			eventfd_copy.target_fd);
+		goto out_task;
 	}
 
 	/*
@@ -155,8 +165,12 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	 */
 
 	fd_install(eventfd_copy.source_fd, file);
+	ret = 0;
 
-	return 0;
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
 }
 
 static long
-- 
1.9.1

[dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Pavel Boldin]

Replace copy-pasted `fget_from_files' -> `filp_close' with
a `sys_close' call.

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++---------------------
 1 file changed, 12 insertions(+), 37 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 0a06594..9bc52a3 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -88,9 +88,8 @@ eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
 	struct task_struct *task_target = NULL;
-	struct file *file;
-	struct files_struct *files;
-	struct fdtable *fdt;
+	struct file *target_file;
+	struct files_struct *target_files;
 	struct eventfd_copy eventfd_copy;
 	long ret = -EFAULT;
 
@@ -109,51 +108,27 @@ eventfd_link_ioctl_copy(unsigned long arg)
 		goto out;
 	}
 
-	ret = -ESTALE;
-	files = get_files_struct(current);
-	if (files == NULL) {
-		pr_info("Failed to get current files struct\n");
-		goto out_task;
-	}
-
-	ret = -EBADF;
-	file = fget_from_files(files, eventfd_copy.source_fd);
-
-	if (file == NULL) {
-		pr_info("Failed to get fd %d from source\n",
-			eventfd_copy.source_fd);
-		put_files_struct(files);
+	/* Closing the source_fd */
+	ret = sys_close(eventfd_copy.source_fd);
+	if (ret)
 		goto out_task;
-	}
-
-	/*
-	 * Release the existing eventfd in the source process
-	 */
-	spin_lock(&files->file_lock);
-	fput(file);
-	filp_close(file, files);
-	fdt = files_fdtable(files);
-	fdt->fd[eventfd_copy.source_fd] = NULL;
-	spin_unlock(&files->file_lock);
-
-	put_files_struct(files);
+	ret = -ESTALE;
 
 	/*
 	 * Find the file struct associated with the target fd.
 	 */
 
-	ret = -ESTALE;
-	files = get_files_struct(task_target);
-	if (files == NULL) {
+	target_files = get_files_struct(task_target);
+	if (target_files == NULL) {
 		pr_info("Failed to get target files struct\n");
 		goto out_task;
 	}
 
 	ret = -EBADF;
-	file = fget_from_files(files, eventfd_copy.target_fd);
-	put_files_struct(files);
+	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
+	put_files_struct(target_files);
 
-	if (file == NULL) {
+	if (target_file == NULL) {
 		pr_info("Failed to get fd %d from target\n",
 			eventfd_copy.target_fd);
 		goto out_task;
@@ -164,7 +139,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
 	 * file desciptor of the source process,
 	 */
 
-	fd_install(eventfd_copy.source_fd, file);
+	fd_install(eventfd_copy.source_fd, target_file);
 	ret = 0;
 
 out_task:
-- 
1.9.1

[dpdk-dev] [PATCH v5 5/5] vhost: eventfd_link: removing extra #includes

[Pavel Boldin]

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 9bc52a3..0ee7357 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -22,18 +22,11 @@
  *   Intel Corporation
  */
 
-#include <linux/eventfd.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/rcupdate.h>
 #include <linux/file.h>
-#include <linux/slab.h>
-#include <linux/fs.h>
-#include <linux/mmu_context.h>
-#include <linux/sched.h>
-#include <asm/mmu_context.h>
 #include <linux/fdtable.h>
+#include <linux/syscalls.h>
 
 #include "eventfd_link.h"
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link'

[Thomas Monjalon]

Huawei, Changchun,
Any opinion about these patches?

2015-04-16 14:48, Pavel Boldin:
> This patchset contains refactoring steps for the `eventfd_link' module
> of the DPDK's `librte_vhost' part.
> 
> The commit messages are updated to include `Signed-off-by'.
> 
> Pavel Boldin (5):
>   vhost: eventfd_link: moving ioctl to a function
>   vhost: eventfd_link: add function fget_from_files
>   vhost: eventfd_link: fix ioctl return values
>   vhost: eventfd_link: replace copy-pasted sys_close
>   vhost: eventfd_link: removing extra #includes
> 
>  lib/librte_vhost/eventfd_link/eventfd_link.c | 181 +++++++++++++--------------
>  1 file changed, 87 insertions(+), 94 deletions(-)

Re: [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link'

[Xie, Huawei]

Thomas:
Would review this patch this week.

On 4/28/2015 10:36 PM, Thomas Monjalon wrote:
> Huawei, Changchun,
> Any opinion about these patches?
>
> 2015-04-16 14:48, Pavel Boldin:
>> This patchset contains refactoring steps for the `eventfd_link' module
>> of the DPDK's `librte_vhost' part.
>>
>> The commit messages are updated to include `Signed-off-by'.
>>
>> Pavel Boldin (5):
>>   vhost: eventfd_link: moving ioctl to a function
>>   vhost: eventfd_link: add function fget_from_files
>>   vhost: eventfd_link: fix ioctl return values
>>   vhost: eventfd_link: replace copy-pasted sys_close
>>   vhost: eventfd_link: removing extra #includes
>>
>>  lib/librte_vhost/eventfd_link/eventfd_link.c | 181 +++++++++++++--------------
>>  1 file changed, 87 insertions(+), 94 deletions(-)
>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Xie, Huawei]

On 4/16/2015 7:48 PM, Pavel Boldin wrote:
> Replace copy-pasted `fget_from_files' -> `filp_close' with
> a `sys_close' call.
>
> Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
> ---
>  lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++---------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
>
> diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
> index 0a06594..9bc52a3 100644
> --- a/lib/librte_vhost/eventfd_link/eventfd_link.c
> +++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
> @@ -88,9 +88,8 @@ eventfd_link_ioctl_copy(unsigned long arg)
>  {
>  
> +	/* Closing the source_fd */
> +	ret = sys_close(eventfd_copy.source_fd);
Pavel:
Here we close the fd and re-install a new file on this fd later. 
sys_close does all cleanup.
But, for instance, if we allocate new fd later, normally it will reuse
the just freed fds by sys_close, is there issue here? 

> +	if (ret)
>  		goto out_task;
> -	}
> -
> -	/*
> -	 * Release the existing eventfd in the source process
> -	 */
> -	spin_lock(&files->file_lock);
> -	fput(file);
> -	filp_close(file, files);
> -	fdt = files_fdtable(files);
> -	fdt->fd[eventfd_copy.source_fd] = NULL;
> -	spin_unlock(&files->file_lock);
> -
> -	put_files_struct(files);
> +	ret = -ESTALE;
>  
>  	/*
>  	 * Find the file struct associated with the target fd.
>  	 */
>  
> -	ret = -ESTALE;
> -	files = get_files_struct(task_target);
> -	if (files == NULL) {
> +	target_files = get_files_struct(task_target);
> +	if (target_files == NULL) {
>  		pr_info("Failed to get target files struct\n");
>  		goto out_task;
>  	}
>  
>  	ret = -EBADF;
> -	file = fget_from_files(files, eventfd_copy.target_fd);
> -	put_files_struct(files);
> +	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
> +	put_files_struct(target_files);
>  
> -	if (file == NULL) {
> +	if (target_file == NULL) {
>  		pr_info("Failed to get fd %d from target\n",
>  			eventfd_copy.target_fd);
>  		goto out_task;
> @@ -164,7 +139,7 @@ eventfd_link_ioctl_copy(unsigned long arg)
>  	 * file desciptor of the source process,
>  	 */
>  
> -	fd_install(eventfd_copy.source_fd, file);
> +	fd_install(eventfd_copy.source_fd, target_file);
>  	ret = 0;
>  
>  out_task:

Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function

[Xie, Huawei]

On 4/3/2015 1:02 AM, Pavel Boldin wrote:
> Move ioctl `EVENTFD_COPY' handler code to an inline function.
Pavel:
There is no necessity to inline this function.
/huawei

Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function

[Xie, Huawei]

On 5/7/2015 9:17 PM, Pavel Boldin wrote:


On Thu, May 7, 2015 at 10:57 AM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
On 4/3/2015 1:02 AM, Pavel Boldin wrote:
> Move ioctl `EVENTFD_COPY' handler code to an inline function.
Pavel:
There is no necessity to inline this function.
Xie, there is even no necessity to split this in a five piece patchseries. I did that solely for the purpose of clean reading.

There is no necessity to inline any single-used functions as long the compiler is decent. But I prefer to instruct compiler to do this explictly so there is no call/ret path in the generated code.

The purpose of inline or not is not for friendly reading. inline is for performance only.
Pavel


/huawei

Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function

[Pavel Boldin]

On Mon, May 18, 2015 at 9:06 AM, Xie, Huawei <huawei.xie@intel.com> wrote:

> On 5/7/2015 9:17 PM, Pavel Boldin wrote:
>
>
> On Thu, May 7, 2015 at 10:57 AM, Xie, Huawei <huawei.xie@intel.com<mailto:
> huawei.xie@intel.com>> wrote:
> On 4/3/2015 1:02 AM, Pavel Boldin wrote:
> > Move ioctl `EVENTFD_COPY' handler code to an inline function.
> Pavel:
> There is no necessity to inline this function.
> Xie, there is even no necessity to split this in a five piece patchseries.
> I did that solely for the purpose of clean reading.
>
> There is no necessity to inline any single-used functions as long the
> compiler is decent. But I prefer to instruct compiler to do this explictly
> so there is no call/ret path in the generated code.
>
> The purpose of inline or not is not for friendly reading. inline is for
> performance only.
>
Well, an optimizing compiler `inline's all the `static' functions that are
called only once in the file. So, this `inline' is purely for readability
of the code. This makes user aware that the function will be `inline'd
anyway.

Pavel


>
>
>
> /huawei
>
>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Thomas Monjalon]

2015-05-07 06:54, Xie, Huawei:
> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
> > +	/* Closing the source_fd */
> > +	ret = sys_close(eventfd_copy.source_fd);
> Pavel:
> Here we close the fd and re-install a new file on this fd later. 
> sys_close does all cleanup.
> But, for instance, if we allocate new fd later, normally it will reuse
> the just freed fds by sys_close, is there issue here? 

Pavel, Huawei,
Could we come to a conclusion on this patch series please?

Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function

[Xie, Huawei]

Two opens here, the trivial one is i think it is not good practice to
explicitly inline non performance critical functions in c file, even if
it will be done by compiler anyway.
The critical one i have concern is whether it will introduce
inconsistency if we call fd_install on a fd that is just closed by
sys_close, because that fd will be set to next-to-be-allocated fd. I
prefer to keep the original logic in patch 4/5 if we are not clear.

As we actually don't need to use the eventfd that is allocated in user
space at all, future patch would be: directly allocate a new fd in the
kernel and call fd_install on it.

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Thomas Monjalon]

Ping

2015-06-17 17:24, Thomas Monjalon:
> 2015-05-07 06:54, Xie, Huawei:
> > On 4/16/2015 7:48 PM, Pavel Boldin wrote:
> > > +	/* Closing the source_fd */
> > > +	ret = sys_close(eventfd_copy.source_fd);
> > Pavel:
> > Here we close the fd and re-install a new file on this fd later. 
> > sys_close does all cleanup.
> > But, for instance, if we allocate new fd later, normally it will reuse
> > the just freed fds by sys_close, is there issue here? 
> 
> Pavel, Huawei,
> Could we come to a conclusion on this patch series please?

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Xie, Huawei]

On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
> 2015-05-07 06:54, Xie, Huawei:
>> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
>>> +	/* Closing the source_fd */
>>> +	ret = sys_close(eventfd_copy.source_fd);
>> Pavel:
>> Here we close the fd and re-install a new file on this fd later. 
>> sys_close does all cleanup.
>> But, for instance, if we allocate new fd later, normally it will reuse
>> the just freed fds by sys_close, is there issue here? 
> Pavel, Huawei,
> Could we come to a conclusion on this patch series please?
For the previous 3 patches, i am OK except that i don't think inline is
needed explicitly for non-performance critical function.
For this patch, didn't check the fs code.

>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Pavel Boldin]

Xie,

Regarding the patches:
1. The replaced code in fourth patch is checked to be a copy-paste of the
`sys_close` syscall.
2. It is not uncommon for the applications to close FD making it allocated
for a different file. In our particular case the file is closed in the
*source* process and *added* to a target process, so matching fds should
not be the problem.
3. There is an implementation of the exact same thing in the SCM_RIGHTS [1]
that can be used as the reference code.

[1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248

Pavel

On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com> wrote:

> On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
> > 2015-05-07 06:54, Xie, Huawei:
> >> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
> >>> +   /* Closing the source_fd */
> >>> +   ret = sys_close(eventfd_copy.source_fd);
> >> Pavel:
> >> Here we close the fd and re-install a new file on this fd later.
> >> sys_close does all cleanup.
> >> But, for instance, if we allocate new fd later, normally it will reuse
> >> the just freed fds by sys_close, is there issue here?
> > Pavel, Huawei,
> > Could we come to a conclusion on this patch series please?
> For the previous 3 patches, i am OK except that i don't think inline is
> needed explicitly for non-performance critical function.
> For this patch, didn't check the fs code.
>
> >
> >
>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Xie, Huawei]

On 7/10/2015 10:50 PM, Pavel Boldin wrote:
Xie,

Regarding the patches:
1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall.
sys_close does extra cleanup than the replaced code. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd? Because it will be allocated starting from the next-to-be-allocated-fd. I think Kernel will do some check on that value, but not sure.

2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem.
Sure, that is what the old code does.
3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code.
I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i think is the clean fix.
Currently we allocate eventfd in user space, and install a new file onto it. Actually  we don't need to allocate eventfd in user space at all, what we should do is allocate a new fd, and install the file on the newly allocated fd.

new_fd = get_unused_fd_flags(...)
fd_install(new_fd, get_file(fp[i]));

/huawei


[1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248

Pavel

On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
> 2015-05-07 06:54, Xie, Huawei:
>> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
>>> +   /* Closing the source_fd */
>>> +   ret = sys_close(eventfd_copy.source_fd);
>> Pavel:
>> Here we close the fd and re-install a new file on this fd later.
>> sys_close does all cleanup.
>> But, for instance, if we allocate new fd later, normally it will reuse
>> the just freed fds by sys_close, is there issue here?
> Pavel, Huawei,
> Could we come to a conclusion on this patch series please?
For the previous 3 patches, i am OK except that i don't think inline is
needed explicitly for non-performance critical function.
For this patch, didn't check the fs code.

>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Xie, Huawei]

Don't know why previous mail get messed.

On 7/10/2015 10:50 PM, Pavel Boldin wrote:
Xie,

Regarding the patches:
1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall.

sys_close does extra cleanup than the replaced coe. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd, because it will be allocated starting from the value of next-to-be-allocted-fd? I think kernel willn't blindly use that value, but not sure.

2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem.

Yes, that is exactly what the old code does.
3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code.

I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i want to replace the current  code with.
Currently we allcoate eventfd in user space and install a new file onto it through fd_install. Actually we don't need to allocate the eventfd in user space at all, what we should do is allocate a new fd in kernel, and install the file onto it.

new_fd = get_unused_fd_flags(...)
fd_install(new_fd, get_file(fp[i])

/huawei

[1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248

Pavel

On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
> 2015-05-07 06:54, Xie, Huawei:
>> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
>>> +   /* Closing the source_fd */
>>> +   ret = sys_close(eventfd_copy.source_fd);
>> Pavel:
>> Here we close the fd and re-install a new file on this fd later.
>> sys_close does all cleanup.
>> But, for instance, if we allocate new fd later, normally it will reuse
>> the just freed fds by sys_close, is there issue here?
> Pavel, Huawei,
> Could we come to a conclusion on this patch series please?
For the previous 3 patches, i am OK except that i don't think inline is
needed explicitly for non-performance critical function.
For this patch, didn't check the fs code.

>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Thomas Monjalon]

2015-07-10 15:42, Xie, Huawei:
> Don't know why previous mail get messed.

Same problem with this one.
It's maybe because you send HTML mail which is wrongly translated in plain text.
 
> On 7/10/2015 10:50 PM, Pavel Boldin wrote:
> Xie,
> 
> Regarding the patches:
> 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall.
> 
> sys_close does extra cleanup than the replaced coe. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd, because it will be allocated starting from the value of next-to-be-allocted-fd? I think kernel willn't blindly use that value, but not sure.
> 
> 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem.
> 
> Yes, that is exactly what the old code does.
> 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code.
> 
> I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i want to replace the current  code with.
> Currently we allcoate eventfd in user space and install a new file onto it through fd_install. Actually we don't need to allocate the eventfd in user space at all, what we should do is allocate a new fd in kernel, and install the file onto it.
> 
> new_fd = get_unused_fd_flags(...)
> fd_install(new_fd, get_file(fp[i])
> 
> /huawei
> 
> [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248
> 
> Pavel
> 
> On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
> On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
> > 2015-05-07 06:54, Xie, Huawei:
> >> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
> >>> +   /* Closing the source_fd */
> >>> +   ret = sys_close(eventfd_copy.source_fd);
> >> Pavel:
> >> Here we close the fd and re-install a new file on this fd later.
> >> sys_close does all cleanup.
> >> But, for instance, if we allocate new fd later, normally it will reuse
> >> the just freed fds by sys_close, is there issue here?
> > Pavel, Huawei,
> > Could we come to a conclusion on this patch series please?
> For the previous 3 patches, i am OK except that i don't think inline is
> needed explicitly for non-performance critical function.
> For this patch, didn't check the fs code.
> 
> >
> >
> 
> 
> 

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Xie, Huawei]

On 7/10/2015 11:50 PM, Thomas Monjalon wrote:
> 2015-07-10 15:42, Xie, Huawei:
>> Don't know why previous mail get messed.
> Same problem with this one.
> It's maybe because you send HTML mail which is wrongly translated in plain text.
Have something to to with Pavel? :). I remember the format usually get
messed  when i reply to his mail.
Anyway, I changed delivery format in Thunderbird from "Auto detect" to
"Plain text only".
>  
>> On 7/10/2015 10:50 PM, Pavel Boldin wrote:
>> Xie,
>>
>> Regarding the patches:
>> 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall.
>>
>> sys_close does extra cleanup than the replaced coe. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd, because it will be allocated starting from the value of next-to-be-allocted-fd? I think kernel willn't blindly use that value, but not sure.
>>
>> 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem.
>>
>> Yes, that is exactly what the old code does.
>> 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code.
>>
>> I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i want to replace the current  code with.
>> Currently we allcoate eventfd in user space and install a new file onto it through fd_install. Actually we don't need to allocate the eventfd in user space at all, what we should do is allocate a new fd in kernel, and install the file onto it.
>>
>> new_fd = get_unused_fd_flags(...)
>> fd_install(new_fd, get_file(fp[i])
>>
>> /huawei
>>
>> [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248
>>
>> Pavel
>>
>> On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
>> On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
>>> 2015-05-07 06:54, Xie, Huawei:
>>>> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
>>>>> +   /* Closing the source_fd */
>>>>> +   ret = sys_close(eventfd_copy.source_fd);
>>>> Pavel:
>>>> Here we close the fd and re-install a new file on this fd later.
>>>> sys_close does all cleanup.
>>>> But, for instance, if we allocate new fd later, normally it will reuse
>>>> the just freed fds by sys_close, is there issue here?
>>> Pavel, Huawei,
>>> Could we come to a conclusion on this patch series please?
>> For the previous 3 patches, i am OK except that i don't think inline is
>> needed explicitly for non-performance critical function.
>> For this patch, didn't check the fs code.
>>
>>>
>>
>>
>
>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Pavel Boldin]

Xie, All,

Please find my comments intermixed below.

On Fri, Jul 10, 2015 at 6:42 PM, Xie, Huawei <huawei.xie@intel.com> wrote:

> Don't know why previous mail get messed.
>
> On 7/10/2015 10:50 PM, Pavel Boldin wrote:
> Xie,
>
> Regarding the patches:
> 1. The replaced code in fourth patch is checked to be a copy-paste of the
> `sys_close` syscall.
>
> sys_close does extra cleanup than the replaced coe. My concern is, for
> example, sys_close will mark the fd as next-to-be-allocated fd. Will there
> be issue when we allocate a new fd, because it will be allocated starting
> from the value of next-to-be-allocted-fd? I think kernel willn't blindly
> use that value, but not sure.
>

That is what applications do when call `close' libc function -- the freed
FD is ready to be allocated again and it is OK for applications to reuse
FDs.


>
> 2. It is not uncommon for the applications to close FD making it allocated
> for a different file. In our particular case the file is closed in the
> *source* process and *added* to a target process, so matching fds should
> not be the problem.
>
> Yes, that is exactly what the old code does.
> 3. There is an implementation of the exact same thing in the SCM_RIGHTS
> [1] that can be used as the reference code.
>
> I did a rough check. Maybe i miss something. I see it calls fd_install on
> a newly allocated fd. That is exactly what i want to replace the current
> code with.
> Currently we allcoate eventfd in user space and install a new file onto it
> through fd_install. Actually we don't need to allocate the eventfd in user
> space at all, what we should do is allocate a new fd in kernel, and install
> the file onto it.
>
> new_fd = get_unused_fd_flags(...)
> fd_install(new_fd, get_file(fp[i])
>

Well, this requires changes from the user-space side so I prefer not to do
it by myself at the moment, because I'm no expert in DPDK. I can provide
with the updated patches though but I will require a lab to check that it
works indeed.

No comments below this line.

Pavel


>
> /huawei
>
> [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248
>
> Pavel
>
> On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:
> huawei.xie@intel.com>> wrote:
> On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
> > 2015-05-07 06:54, Xie, Huawei:
> >> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
> >>> +   /* Closing the source_fd */
> >>> +   ret = sys_close(eventfd_copy.source_fd);
> >> Pavel:
> >> Here we close the fd and re-install a new file on this fd later.
> >> sys_close does all cleanup.
> >> But, for instance, if we allocate new fd later, normally it will reuse
> >> the just freed fds by sys_close, is there issue here?
> > Pavel, Huawei,
> > Could we come to a conclusion on this patch series please?
> For the previous 3 patches, i am OK except that i don't think inline is
> needed explicitly for non-performance critical function.
> For this patch, didn't check the fs code.
>
> >
> >
>
>
>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Xie, Huawei]

On 7/11/2015 11:08 PM, Pavel Boldin wrote:
> Xie, All,
>
> Please find my comments intermixed below.
>
> On Fri, Jul 10, 2015 at 6:42 PM, Xie, Huawei <huawei.xie@intel.com
> <mailto:huawei.xie@intel.com>> wrote:
>
>     Don't know why previous mail get messed.
>
>     On 7/10/2015 10:50 PM, Pavel Boldin wrote:
>     Xie,
>
>     Regarding the patches:
>     1. The replaced code in fourth patch is checked to be a copy-paste
>     of the `sys_close` syscall.
>
>     sys_close does extra cleanup than the replaced coe. My concern is,
>     for example, sys_close will mark the fd as next-to-be-allocated
>     fd. Will there be issue when we allocate a new fd, because it will
>     be allocated starting from the value of next-to-be-allocted-fd? I
>     think kernel willn't blindly use that value, but not sure.
>
>
> That is what applications do when call `close' libc function -- the
> freed FD is ready to be allocated again and it is OK for applications
> to reuse FDs.
>  
Pavel:
Maybe i don't make it clear.  It is ok we call sys_close to close a fd,
and then alloc_fd to reuse the previous fd, but the tricky thing for us
is after sys_close, we call fd_install to install a file on the previous
fd value.

Consider the following code flow:
1. sys_close->__close_fd: close the fd and mark the value of fd as the
next_fd
2. fd_install(fd, file): here we install the file onto just freed fd in
our event_fd module.
3.  In the later fd allocation call, 
    new_fd = alloc_fd(...),  fd = files->next_fd
Is it possible we get the previous fd value, while it is still being
used? If it is, then there is issue here.

so  both the following two  are straightforward.
1) sys_close->allocate_fd
2) As in our previous code, we did some internal cleanup on our old fd,
and call fd_install to install the fd with a new file.

     

>
>     2. It is not uncommon for the applications to close FD making it
>     allocated for a different file. In our particular case the file is
>     closed in the *source* process and *added* to a target process, so
>     matching fds should not be the problem.
>
>     Yes, that is exactly what the old code does.
>     3. There is an implementation of the exact same thing in the
>     SCM_RIGHTS [1] that can be used as the reference code.
>
>     I did a rough check. Maybe i miss something. I see it calls
>     fd_install on a newly allocated fd. That is exactly what i want to
>     replace the current  code with.
>     Currently we allcoate eventfd in user space and install a new file
>     onto it through fd_install. Actually we don't need to allocate the
>     eventfd in user space at all, what we should do is allocate a new
>     fd in kernel, and install the file onto it.
>
>     new_fd = get_unused_fd_flags(...)
>     fd_install(new_fd, get_file(fp[i])
>
>
> Well, this requires changes from the user-space side so I prefer not
> to do it by myself at the moment, because I'm no expert in DPDK. I can
> provide with the updated patches though but I will require a lab to
> check that it works indeed.
>
Thanks.
So if we aren't sure about this patch, is it ok we apply the previous
three patches first?

> No comments below this line.
>
> Pavel
>  
>
>
>     /huawei
>
>     [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248
>
>     Pavel
>
>     On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com
>     <mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com
>     <mailto:huawei.xie@intel.com>>> wrote:
>     On 6/17/2015 11:24 PM, Thomas Monjalon wrote:
>     > 2015-05-07 06:54, Xie, Huawei:
>     >> On 4/16/2015 7:48 PM, Pavel Boldin wrote:
>     >>> +   /* Closing the source_fd */
>     >>> +   ret = sys_close(eventfd_copy.source_fd);
>     >> Pavel:
>     >> Here we close the fd and re-install a new file on this fd later.
>     >> sys_close does all cleanup.
>     >> But, for instance, if we allocate new fd later, normally it
>     will reuse
>     >> the just freed fds by sys_close, is there issue here?
>     > Pavel, Huawei,
>     > Could we come to a conclusion on this patch series please?
>     For the previous 3 patches, i am OK except that i don't think
>     inline is
>     needed explicitly for non-performance critical function.
>     For this patch, didn't check the fs code.
>
>     >
>     >
>
>
>
>

Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close

[Pavel Boldin]

Xie,

I think it is OK to merge first three patches at the moment.

I'm going to implement a new scheme in a different ioctl soon.

Pavel

[dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler

[Pavel Boldin]

* Move ioctl `EVENTFD_COPY' code to a separate function
* Remove extra #includes
* Introduce function fget_from_files
* Fix ioctl return values

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 188 +++++++++++++++------------
 1 file changed, 103 insertions(+), 85 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 62c45c8..5ba1068 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -22,18 +22,11 @@
  *   Intel Corporation
  */
 
-#include <linux/eventfd.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/rcupdate.h>
 #include <linux/file.h>
-#include <linux/slab.h>
-#include <linux/fs.h>
-#include <linux/mmu_context.h>
-#include <linux/sched.h>
-#include <asm/mmu_context.h>
 #include <linux/fdtable.h>
+#include <linux/syscalls.h>
 
 #include "eventfd_link.h"
 
@@ -65,9 +58,26 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
+
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file)
+	{
+		if (file->f_mode & FMODE_PATH
+			|| !atomic_long_inc_not_zero(&file->f_count))
+		    file = NULL;
+	}
+	rcu_read_unlock();
+
+	return file;
+}
 
 static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
 	struct task_struct *task_target = NULL;
@@ -75,91 +85,99 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
 	struct files_struct *files;
 	struct fdtable *fdt;
 	struct eventfd_copy eventfd_copy;
+	long ret = -EFAULT;
 
-	switch (ioctl) {
-	case EVENTFD_COPY:
-		if (copy_from_user(&eventfd_copy, argp,
-			sizeof(struct eventfd_copy)))
-			return -EFAULT;
-
-		/*
-		 * Find the task struct for the target pid
-		 */
-		task_target =
-			pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
-		if (task_target == NULL) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
-
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.source_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-				file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
+	if (copy_from_user(&eventfd_copy, argp, sizeof(struct eventfd_copy)))
+		goto out;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	ret = -ESRCH;
+
+	task_target =
+		get_pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_info("Unable to find pid %d\n", eventfd_copy.target_pid);
+		goto out;
+	}
+
+	ret = -ESTALE;
+	files = get_files_struct(current);
+	if (files == NULL) {
+		pr_info("Failed to get current files struct\n");
+		goto out_task;
+	}
 
-		if (file == NULL) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
-
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		fput(file);
-		filp_close(file, files);
-		fdt = files_fdtable(files);
-		fdt->fd[eventfd_copy.source_fd] = NULL;
-		spin_unlock(&files->file_lock);
-
-		/*
-		 * Find the file struct associated with the target fd.
-		 */
-
-		files = get_files_struct(task_target);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
-
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.target_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-					file = NULL;
-		}
-		rcu_read_unlock();
+	ret = -EBADF;
+	file = fget_from_files(files, eventfd_copy.source_fd);
+
+	if (file == NULL) {
+		pr_info("Failed to get fd %d from source\n",
+			eventfd_copy.source_fd);
 		put_files_struct(files);
+		goto out_task;
+	}
+
+	/*
+	 * Release the existing eventfd in the source process
+	 */
+	spin_lock(&files->file_lock);
+	fput(file);
+	filp_close(file, files);
+	fdt = files_fdtable(files);
+	fdt->fd[eventfd_copy.source_fd] = NULL;
+	spin_unlock(&files->file_lock);
+
+	put_files_struct(files);
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	ret = -ESTALE;
+	files = get_files_struct(task_target);
+	if (files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	file = fget_from_files(files, eventfd_copy.target_fd);
+	put_files_struct(files);
+
+	if (file == NULL) {
+		pr_info("Failed to get fd %d from target\n",
+			eventfd_copy.target_fd);
+		goto out_task;
+	}
 
-		if (file == NULL) {
-			pr_debug("Failed to get file from target pid\n");
-			return 0;
-		}
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
 
-		/*
-		 * Install the file struct from the target process into the
-		 * file desciptor of the source process,
-		 */
+	fd_install(eventfd_copy.source_fd, file);
+	ret = 0;
 
-		fd_install(eventfd_copy.source_fd, file);
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
 
-		return 0;
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret = -ENOIOCTLCMD;
 
-	default:
-		return -ENOIOCTLCMD;
+	switch (ioctl) {
+	case EVENTFD_COPY:
+		ret = eventfd_link_ioctl_copy(arg);
+		break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
-- 
1.9.1

[dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl

[Pavel Boldin]

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 61 ++++++++++++++++++++++++++++
 lib/librte_vhost/eventfd_link/eventfd_link.h | 28 ++++++++++---
 2 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 5ba1068..0264365 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -77,6 +77,64 @@ fget_from_files(struct files_struct *files, unsigned fd)
 }
 
 static long
+eventfd_link_ioctl_copy2(unsigned long arg)
+{
+	void __user *argp = (void __user *) arg;
+	struct task_struct *task_target = NULL;
+	struct file *file;
+	struct files_struct *files;
+	struct eventfd_copy2 eventfd_copy2;
+	long ret = -EFAULT;
+
+	if (copy_from_user(&eventfd_copy2, argp, sizeof(struct eventfd_copy2)))
+		goto out;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	ret = -ESRCH;
+
+	task_target =
+		get_pid_task(find_vpid(eventfd_copy2.pid), PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_info("Unable to find pid %d\n", eventfd_copy2.pid);
+		goto out;
+	}
+
+	ret = -ESTALE;
+	files = get_files_struct(task_target);
+	if (files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	file = fget_from_files(files, eventfd_copy2.fd);
+	put_files_struct(files);
+
+	if (file == NULL) {
+		pr_info("Failed to get fd %d from target\n", eventfd_copy2.fd);
+		goto out_task;
+	}
+
+	/*
+	 * Install the file struct from the target process into the
+	 * newly allocated file desciptor of the source process.
+	 */
+	ret = get_unused_fd_flags(eventfd_copy2.flags);
+	if (ret < 0) {
+        fput(file);
+		goto out_task;
+    }
+	fd_install(ret, file);
+
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
+
+static long
 eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
@@ -175,6 +233,9 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
 	case EVENTFD_COPY:
 		ret = eventfd_link_ioctl_copy(arg);
 		break;
+	case EVENTFD_COPY2:
+		ret = eventfd_link_ioctl_copy2(arg);
+		break;
 	}
 
 	return ret;
diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.h b/lib/librte_vhost/eventfd_link/eventfd_link.h
index ea619ec..5ebc20b 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.h
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.h
@@ -61,11 +61,6 @@
 #define _EVENTFD_LINK_H_
 
 /*
- * ioctl to copy an fd entry in calling process to an fd in a target process
- */
-#define EVENTFD_COPY 1
-
-/*
  * arguements for the EVENTFD_COPY ioctl
  */
 struct eventfd_copy {
@@ -73,4 +68,27 @@ struct eventfd_copy {
 	unsigned source_fd; /* fd in the calling pid */
 	pid_t target_pid; /* pid of the target pid */
 };
+
+/*
+ * ioctl to copy an fd entry in calling process to an fd in a target process
+ * NOTE: this one should be
+ * #define EVENTFD_COPY _IOWR('D', 1, struct eventfd_copy) actually
+ */
+#define EVENTFD_COPY 1
+
+/*
+ * arguments for the EVENTFD_COPY2 ioctl
+ */
+struct eventfd_copy2 {
+	unsigned fd; /* fd to steal */
+	pid_t pid; /* pid of the process to steal from */
+	unsigned flags; /* flags to allocate new fd with */
+};
+
+/*
+ * ioctl to copy an fd entry from the target process into newly allocated
+ * fd in the calling process
+ */
+#define EVENTFD_COPY2 _IOW('D', 2, struct eventfd_copy2)
+
 #endif /* _EVENTFD_LINK_H_ */
-- 
1.9.1

[dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2

[Pavel Boldin]

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/vhost_cuse/eventfd_copy.c   | 54 ++++++++++++++++++----------
 lib/librte_vhost/vhost_cuse/eventfd_copy.h   |  6 ++++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  3 ++
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/lib/librte_vhost/vhost_cuse/eventfd_copy.c b/lib/librte_vhost/vhost_cuse/eventfd_copy.c
index 4d697a2..1625163 100644
--- a/lib/librte_vhost/vhost_cuse/eventfd_copy.c
+++ b/lib/librte_vhost/vhost_cuse/eventfd_copy.c
@@ -46,6 +46,32 @@
 
 static const char eventfd_cdev[] = "/dev/eventfd-link";
 
+static int eventfd_link = -1;
+
+int
+eventfd_init(void)
+{
+	if (eventfd_link > 0)
+		return 0;
+
+	eventfd_link = open(eventfd_cdev, O_RDWR);
+	if (eventfd_link < 0) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"eventfd_link module is not loaded\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+int
+eventfd_free(void)
+{
+	if (eventfd_link > 0)
+		close(eventfd_link);
+	return 0;
+}
+
 /*
  * This function uses the eventfd_link kernel module to copy an eventfd file
  * descriptor provided by QEMU in to our process space.
@@ -53,36 +79,26 @@ static const char eventfd_cdev[] = "/dev/eventfd-link";
 int
 eventfd_copy(int target_fd, int target_pid)
 {
-	int eventfd_link, ret;
-	struct eventfd_copy eventfd_copy;
-	int fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	int ret;
+	struct eventfd_copy2 eventfd_copy2;
 
-	if (fd == -1)
-		return -1;
 
 	/* Open the character device to the kernel module. */
 	/* TODO: check this earlier rather than fail until VM boots! */
-	eventfd_link = open(eventfd_cdev, O_RDWR);
-	if (eventfd_link < 0) {
-		RTE_LOG(ERR, VHOST_CONFIG,
-			"eventfd_link module is not loaded\n");
-		close(fd);
+	if (eventfd_init() < 0)
 		return -1;
-	}
 
-	eventfd_copy.source_fd = fd;
-	eventfd_copy.target_fd = target_fd;
-	eventfd_copy.target_pid = target_pid;
+	eventfd_copy2.fd = target_fd;
+	eventfd_copy2.pid = target_pid;
+	eventfd_copy2.flags = O_NONBLOCK | O_CLOEXEC;
 	/* Call the IOCTL to copy the eventfd. */
-	ret = ioctl(eventfd_link, EVENTFD_COPY, &eventfd_copy);
-	close(eventfd_link);
+	ret = ioctl(eventfd_link, EVENTFD_COPY2, &eventfd_copy2);
 
 	if (ret < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"EVENTFD_COPY ioctl failed\n");
-		close(fd);
+			"EVENTFD_COPY2 ioctl failed\n");
 		return -1;
 	}
 
-	return fd;
+	return ret;
 }
diff --git a/lib/librte_vhost/vhost_cuse/eventfd_copy.h b/lib/librte_vhost/vhost_cuse/eventfd_copy.h
index 19ae30d..5f446ca 100644
--- a/lib/librte_vhost/vhost_cuse/eventfd_copy.h
+++ b/lib/librte_vhost/vhost_cuse/eventfd_copy.h
@@ -34,6 +34,12 @@
 #define _EVENTFD_H
 
 int
+eventfd_init(void);
+
+int
+eventfd_free(void);
+
+int
 eventfd_copy(int target_fd, int target_pid);
 
 #endif
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 1ae7c49..ae7ad8d 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -373,6 +373,9 @@ rte_vhost_driver_register(const char *dev_name)
 		return -1;
 	}
 
+	if (eventfd_init() < 0)
+		return -1;
+
 	/*
 	 * The device name is created. This is passed to QEMU so that it can
 	 * register the device with our application.
-- 
1.9.1

[dpdk-dev] [PATCH v5 4/4] DO NOT MERGE: Tests for new eventfd_link module

[Pavel Boldin]

To use:
1. Compile and load the new eventfd_link module (as root):
 # (cd lib/librte_vhost/eventfd_link; make; insmod ./eventfd_link.ko)
2. Compile the test program:
 $ make -C test_eventfd_copy
3. Run it as root:
 # sudo ./test_eventfd_copy/test_eventfd_copy --check
 Stealing FD OK
---
 test_eventfd_copy/Makefile            |   8 ++
 test_eventfd_copy/rte_log.h           |   2 +
 test_eventfd_copy/test_eventfd_copy.c | 248 ++++++++++++++++++++++++++++++++++
 test_eventfd_copy/vhost-net.h         |   0
 4 files changed, 258 insertions(+)
 create mode 100644 test_eventfd_copy/Makefile
 create mode 100644 test_eventfd_copy/rte_log.h
 create mode 100644 test_eventfd_copy/test_eventfd_copy.c
 create mode 100644 test_eventfd_copy/vhost-net.h

diff --git a/test_eventfd_copy/Makefile b/test_eventfd_copy/Makefile
new file mode 100644
index 0000000..99b3ae3
--- /dev/null
+++ b/test_eventfd_copy/Makefile
@@ -0,0 +1,8 @@
+
+
+test_eventfd_copy: test_eventfd_copy.c \
+                   ../lib/librte_vhost/vhost_cuse/eventfd_copy.c
+	gcc -o $@ $^ \
+	    -I . \
+	    -I ../lib/librte_vhost/ \
+            -I ../lib/librte_vhost/vhost_cuse/
diff --git a/test_eventfd_copy/rte_log.h b/test_eventfd_copy/rte_log.h
new file mode 100644
index 0000000..ffc64c9
--- /dev/null
+++ b/test_eventfd_copy/rte_log.h
@@ -0,0 +1,2 @@
+
+#define RTE_LOG(...)
diff --git a/test_eventfd_copy/test_eventfd_copy.c b/test_eventfd_copy/test_eventfd_copy.c
new file mode 100644
index 0000000..9dfa414
--- /dev/null
+++ b/test_eventfd_copy/test_eventfd_copy.c
@@ -0,0 +1,248 @@
+/*-
+ *   BSD LICENSE
+ *   Copyright(c) 2015 Mirantis Inc. All rights reserved.
+ *   Based on `eventfd_copy.c` from the Intel's DPDK
+
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <unistd.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/eventfd.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "eventfd_copy.h"
+
+static void
+usage()
+{
+	fprintf(stderr,
+"test_eventfd_copy (--file-nr [number] | --check)\n"
+"\t--file-nr\tchecks that there is no `struct file' leakage by ensuring that\n"
+"\t\t\t `/proc/sys/fs/file-nr' value is not growing while `eventfd_copy'ing\n"
+"\t\t\t number times (default is 100000)\n"
+"\t--check\t\tchecks that the `fd' is moved correctly\n");
+}
+
+static int
+read_proc_file_nr()
+{
+	int fd, ret;
+	char buf[1024];
+	fd = open("/proc/sys/fs/file-nr", O_RDONLY);
+	if (fd < 0) {
+		perror("open file-nr");
+		return -1;
+	}
+
+	if (read(fd, buf, 1024) < 0) {
+		perror("read");
+		close(fd);
+		return -1;
+	}
+
+	ret = atoi(buf);
+
+	close(fd);
+
+	return ret;
+}
+
+static int
+check_file_nr(int count)
+{
+        pid_t pid;
+	int i, fd;
+
+	pid = getpid();
+
+	fprintf(stderr, "dummy eventfd_copy check for %d file(s)\n", count);
+	fprintf(stdout, "file-nr before: %d\n", read_proc_file_nr());
+        for (i = 0; i < count; ++i) {
+                fd = eventfd_copy(2, pid);
+                if (fd < 0)
+			return 1;
+                close(fd);
+        }
+	fprintf(stdout, "file-nr after: %d\n", read_proc_file_nr());
+
+	return 0;
+}
+
+
+#define FD_TO_LINK 42
+#define SALT	"The Life, the Universe and everything"
+
+/* checks link:
+   0. opens a pipe
+   1. forks
+   2. child opens tempfile
+   3. child dups tempfile fd to fd=42
+   4. child writes salt to the tempfile
+   5. child notifies parent by writing to a pipe
+   6. parent steals child fd
+   7. parent kills child
+   8. parent checks file content
+   9. parent removes tempfile
+ */
+static int
+check_link(void)
+{
+	pid_t cpid;
+	int pipefd[2];
+	char buf;
+
+	if (pipe(pipefd) < 0) {
+		perror("pipe");
+		return 1;
+	}
+
+	cpid = fork();
+	if (cpid == -1) {
+		perror("fork");
+		return 1;
+	}
+
+	if (cpid) {
+		int stolen_fd, ret = 1;
+		char buf[1024], tmpfname[1024] = "";
+
+		close(pipefd[1]);
+
+		if ((ret = read(pipefd[0], tmpfname, 1024)) < 0) {
+			perror("read pipefd");
+			goto parent_out;
+		}
+		close(pipefd[0]);
+
+		stolen_fd = eventfd_copy(FD_TO_LINK, cpid);
+		if (stolen_fd < 0) {
+			goto parent_out;
+		}
+
+		if (lseek(stolen_fd, 0, SEEK_SET) < 0) {
+			perror("lseek");
+			goto parent_out;
+		}
+
+		if (read(stolen_fd, buf, 1024) < 0) {
+			perror("read stolen salt");
+			goto parent_out;
+		}
+
+		if (strcmp(buf, SALT)) {
+			fprintf(stdout, "Stealing FD failed\n");
+		}
+		else {
+			fprintf(stdout, "Stealing FD OK\n");
+		}
+
+		close(stolen_fd);
+
+		ret = 0;
+parent_out:
+		if (tmpfname[0])
+			unlink(tmpfname);
+		kill(cpid, SIGKILL);
+		wait(NULL);
+		return ret;
+	}
+
+	if (cpid == 0) {
+		int fd;
+		char fname[] = "/tmp/linkXXXXXX";
+
+		close(pipefd[0]);
+
+		fd = mkstemp(fname);
+		if (fd < 0) {
+			perror("mkstemp");
+			return 1;
+		}
+
+		if (dup2(fd, FD_TO_LINK) < 0) {
+			perror("dup2");
+			close(fd);
+			return 1;
+		}
+
+		close(fd);
+
+		if (write(FD_TO_LINK, (void*)SALT, strlen(SALT)) < 0) {
+			perror("write salt");
+			return 1;
+		}
+
+		fsync(FD_TO_LINK);
+
+		if (write(pipefd[1], fname, strlen(fname)) < 0) {
+			perror("write pipe");
+			return 1;
+		}
+
+		close(pipefd[1]);
+
+		while (1) {
+			sleep(1);
+		}
+
+		return 0;
+	}
+}
+
+int
+main(int argc, const char** argv)
+{
+	if (argc < 2) {
+		usage();
+		return 0;
+	}
+
+	if (!strcmp(argv[1], "--file-nr")) {
+		int count = 100000;
+		if (argc >= 3)
+			count = atoi(argv[2]);
+		return check_file_nr(count);
+	}
+
+	if (!strcmp(argv[1], "--check")) {
+		return check_link();
+	}
+
+err:
+	usage();
+	return 1;
+}
diff --git a/test_eventfd_copy/vhost-net.h b/test_eventfd_copy/vhost-net.h
new file mode 100644
index 0000000..e69de29
-- 
1.9.1

Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler

[Pavel Boldin]

Ping.

On Fri, Aug 28, 2015 at 9:51 PM, Pavel Boldin <pboldin@mirantis.com> wrote:

> * Move ioctl `EVENTFD_COPY' code to a separate function
> * Remove extra #includes
> * Introduce function fget_from_files
> * Fix ioctl return values
>
> Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
> ---
>  lib/librte_vhost/eventfd_link/eventfd_link.c | 188
> +++++++++++++++------------
>  1 file changed, 103 insertions(+), 85 deletions(-)
>
> diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c
> b/lib/librte_vhost/eventfd_link/eventfd_link.c
> index 62c45c8..5ba1068 100644
> --- a/lib/librte_vhost/eventfd_link/eventfd_link.c
> +++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
> @@ -22,18 +22,11 @@
>   *   Intel Corporation
>   */
>
> -#include <linux/eventfd.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
> -#include <linux/moduleparam.h>
> -#include <linux/rcupdate.h>
>  #include <linux/file.h>
> -#include <linux/slab.h>
> -#include <linux/fs.h>
> -#include <linux/mmu_context.h>
> -#include <linux/sched.h>
> -#include <asm/mmu_context.h>
>  #include <linux/fdtable.h>
> +#include <linux/syscalls.h>
>
>  #include "eventfd_link.h"
>
> @@ -65,9 +58,26 @@ put_files_struct(struct files_struct *files)
>                 BUG();
>  }
>
> +static struct file *
> +fget_from_files(struct files_struct *files, unsigned fd)
> +{
> +       struct file *file;
> +
> +       rcu_read_lock();
> +       file = fcheck_files(files, fd);
> +       if (file)
> +       {
> +               if (file->f_mode & FMODE_PATH
> +                       || !atomic_long_inc_not_zero(&file->f_count))
> +                   file = NULL;
> +       }
> +       rcu_read_unlock();
> +
> +       return file;
> +}
>
>  static long
> -eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
> +eventfd_link_ioctl_copy(unsigned long arg)
>  {
>         void __user *argp = (void __user *) arg;
>         struct task_struct *task_target = NULL;
> @@ -75,91 +85,99 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl,
> unsigned long arg)
>         struct files_struct *files;
>         struct fdtable *fdt;
>         struct eventfd_copy eventfd_copy;
> +       long ret = -EFAULT;
>
> -       switch (ioctl) {
> -       case EVENTFD_COPY:
> -               if (copy_from_user(&eventfd_copy, argp,
> -                       sizeof(struct eventfd_copy)))
> -                       return -EFAULT;
> -
> -               /*
> -                * Find the task struct for the target pid
> -                */
> -               task_target =
> -                       pid_task(find_vpid(eventfd_copy.target_pid),
> PIDTYPE_PID);
> -               if (task_target == NULL) {
> -                       pr_debug("Failed to get mem ctx for target pid\n");
> -                       return -EFAULT;
> -               }
> -
> -               files = get_files_struct(current);
> -               if (files == NULL) {
> -                       pr_debug("Failed to get files struct\n");
> -                       return -EFAULT;
> -               }
> -
> -               rcu_read_lock();
> -               file = fcheck_files(files, eventfd_copy.source_fd);
> -               if (file) {
> -                       if (file->f_mode & FMODE_PATH ||
> -                               !atomic_long_inc_not_zero(&file->f_count))
> -                               file = NULL;
> -               }
> -               rcu_read_unlock();
> -               put_files_struct(files);
> +       if (copy_from_user(&eventfd_copy, argp, sizeof(struct
> eventfd_copy)))
> +               goto out;
> +
> +       /*
> +        * Find the task struct for the target pid
> +        */
> +       ret = -ESRCH;
> +
> +       task_target =
> +               get_pid_task(find_vpid(eventfd_copy.target_pid),
> PIDTYPE_PID);
> +       if (task_target == NULL) {
> +               pr_info("Unable to find pid %d\n",
> eventfd_copy.target_pid);
> +               goto out;
> +       }
> +
> +       ret = -ESTALE;
> +       files = get_files_struct(current);
> +       if (files == NULL) {
> +               pr_info("Failed to get current files struct\n");
> +               goto out_task;
> +       }
>
> -               if (file == NULL) {
> -                       pr_debug("Failed to get file from source pid\n");
> -                       return 0;
> -               }
> -
> -               /*
> -                * Release the existing eventfd in the source process
> -                */
> -               spin_lock(&files->file_lock);
> -               fput(file);
> -               filp_close(file, files);
> -               fdt = files_fdtable(files);
> -               fdt->fd[eventfd_copy.source_fd] = NULL;
> -               spin_unlock(&files->file_lock);
> -
> -               /*
> -                * Find the file struct associated with the target fd.
> -                */
> -
> -               files = get_files_struct(task_target);
> -               if (files == NULL) {
> -                       pr_debug("Failed to get files struct\n");
> -                       return -EFAULT;
> -               }
> -
> -               rcu_read_lock();
> -               file = fcheck_files(files, eventfd_copy.target_fd);
> -               if (file) {
> -                       if (file->f_mode & FMODE_PATH ||
> -                               !atomic_long_inc_not_zero(&file->f_count))
> -                                       file = NULL;
> -               }
> -               rcu_read_unlock();
> +       ret = -EBADF;
> +       file = fget_from_files(files, eventfd_copy.source_fd);
> +
> +       if (file == NULL) {
> +               pr_info("Failed to get fd %d from source\n",
> +                       eventfd_copy.source_fd);
>                 put_files_struct(files);
> +               goto out_task;
> +       }
> +
> +       /*
> +        * Release the existing eventfd in the source process
> +        */
> +       spin_lock(&files->file_lock);
> +       fput(file);
> +       filp_close(file, files);
> +       fdt = files_fdtable(files);
> +       fdt->fd[eventfd_copy.source_fd] = NULL;
> +       spin_unlock(&files->file_lock);
> +
> +       put_files_struct(files);
> +
> +       /*
> +        * Find the file struct associated with the target fd.
> +        */
> +
> +       ret = -ESTALE;
> +       files = get_files_struct(task_target);
> +       if (files == NULL) {
> +               pr_info("Failed to get target files struct\n");
> +               goto out_task;
> +       }
> +
> +       ret = -EBADF;
> +       file = fget_from_files(files, eventfd_copy.target_fd);
> +       put_files_struct(files);
> +
> +       if (file == NULL) {
> +               pr_info("Failed to get fd %d from target\n",
> +                       eventfd_copy.target_fd);
> +               goto out_task;
> +       }
>
> -               if (file == NULL) {
> -                       pr_debug("Failed to get file from target pid\n");
> -                       return 0;
> -               }
> +       /*
> +        * Install the file struct from the target process into the
> +        * file desciptor of the source process,
> +        */
>
> -               /*
> -                * Install the file struct from the target process into the
> -                * file desciptor of the source process,
> -                */
> +       fd_install(eventfd_copy.source_fd, file);
> +       ret = 0;
>
> -               fd_install(eventfd_copy.source_fd, file);
> +out_task:
> +       put_task_struct(task_target);
> +out:
> +       return ret;
> +}
>
> -               return 0;
> +static long
> +eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
> +{
> +       long ret = -ENOIOCTLCMD;
>
> -       default:
> -               return -ENOIOCTLCMD;
> +       switch (ioctl) {
> +       case EVENTFD_COPY:
> +               ret = eventfd_link_ioctl_copy(arg);
> +               break;
>         }
> +
> +       return ret;
>  }
>
>  static const struct file_operations eventfd_link_fops = {
> --
> 1.9.1
>
>

Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler

[Thomas Monjalon]

Hi Pavel,

2015-09-23 23:25, Pavel Boldin:
> Ping.

Are you requesting a review?
I think you should set Huawei Xie in the recipient list.

Other note: it is not easy to follow the evolution of your patches because
there is no cover letter and the numbering (v5) is not incremented.

These little details may make review happening ;)

Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler

[Pavel Boldin]

Hi Thomas,

On Tue, Sep 29, 2015 at 10:42 PM, Thomas Monjalon <thomas.monjalon@6wind.com
> wrote:

> Hi Pavel,
>
> 2015-09-23 23:25, Pavel Boldin:
> > Ping.
>
> Are you requesting a review?
>
Yes.


> I think you should set Huawei Xie in the recipient list.
>
Huawei Xie CCed


>
> Other note: it is not easy to follow the evolution of your patches because
> there is no cover letter and the numbering (v5) is not incremented.
>
Should I reupload them then?

Pavel

Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler

[Xie, Huawei]

On 8/29/2015 2:51 AM, Pavel Boldin wrote:
> * Move ioctl `EVENTFD_COPY' code to a separate function
> * Remove extra #includes
> * Introduce function fget_from_files
> * Fix ioctl return values
>
> Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
Basically OK with this patch since we have reviewed before.
Could you split the patch, one change each patch? Thomas will require
you to do this too. :). I see you have also changed when to call
put_files_struct. It is better put it in a separate patch with the reason.
Another thing is code style. Run checkpatch.pl against your patch. There
are some issues.

Re: [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl

[Xie, Huawei]

On 8/29/2015 2:51 AM, Pavel Boldin wrote:
> Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
> ---
>  
OK with the patch. Please run checkpatch.pl against your patch.

Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2

[Xie, Huawei]

Thanks Pavel for this work.
This is what we think is the better implementation for eventfd proxy, in
our last review.
Could you add an additional patch to remove the old implementation?

Again, please run checkpatch.pl against your patch.
On 8/29/2015 2:51 AM, Pavel Boldin wrote:

[...]
> +
> +int
> +eventfd_init(void)
> +{
> +	if (eventfd_link > 0)
0 could be valid fd. Change it to:
    if (eventfd_link >= 0)
Change elsewhere if i miss it.
> +int
> +eventfd_free(void)
> +{
> +	if (eventfd_link > 0)
same as above:
    if (eventfd_link >= 0)

[...]

Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2

[Pavel Boldin]

Xie,

Please find my comments intermixed below.

On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <huawei.xie@intel.com> wrote:

> Thanks Pavel for this work.
> This is what we think is the better implementation for eventfd proxy, in
> our last review.
> Could you add an additional patch to remove the old implementation?
>
I'm not really sure if we should do it -- imagine upgrading from one
version of DPDK to another.
Given the current implementation there is a backward compatibility.


>
> Again, please run checkpatch.pl against your patch.
>
Oops. Thanks for pointing out.


> On 8/29/2015 2:51 AM, Pavel Boldin wrote:
>
> [...]
> > +
> > +int
> > +eventfd_init(void)
> > +{
> > +     if (eventfd_link > 0)
> 0 could be valid fd. Change it to:
>
Got it. Thanks.


>     if (eventfd_link >= 0)
> Change elsewhere if i miss it.
> > +int
> > +eventfd_free(void)
> > +{
> > +     if (eventfd_link > 0)
> same as above:
>     if (eventfd_link >= 0)
>
> [...]
>

--
Sincerely,
 Pavel

Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2

[Xie, Huawei]

On 10/21/2015 8:16 PM, Pavel Boldin wrote:
> Xie,
>
> Please find my comments intermixed below.
>
> On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <huawei.xie@intel.com
> <mailto:huawei.xie@intel.com>> wrote:
>
>     Thanks Pavel for this work.
>     This is what we think is the better implementation for eventfd
>     proxy, in
>     our last review.
>     Could you add an additional patch to remove the old implementation?
>
> I'm not really sure if we should do it -- imagine upgrading from one
> version of DPDK to another.
> Given the current implementation there is a backward compatibility.
I couldn't image the case any one would run old dpdk app with the new
dpdk module. However I am ok you leave it here, :), we could remove this
in next release.
Could you finish rebasing the patch before end of next week, otherwise
it will lose chance of being merged.
>  
>
>
>     Again, please run checkpatch.pl <http://checkpatch.pl> against
>     your patch.
>
> Oops. Thanks for pointing out.
>  
>
>     On 8/29/2015 2:51 AM, Pavel Boldin wrote:
>
>     [...]
>     > +
>     > +int
>     > +eventfd_init(void)
>     > +{
>     > +     if (eventfd_link > 0)
>     0 could be valid fd. Change it to:
>
> Got it. Thanks.
>  
>
>         if (eventfd_link >= 0)
>     Change elsewhere if i miss it.
>     > +int
>     > +eventfd_free(void)
>     > +{
>     > +     if (eventfd_link > 0)
>     same as above:
>         if (eventfd_link >= 0)
>
>     [...]
>
>
> --
> Sincerely,
>  Pavel

[dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring

[Pavel Boldin]

The patchset contains an attempt at the refactoring the `eventfd_link`
kernel module that is used to steal an FD in DPDK.

The first patch refactors old EVENTFD_COPY handler fixing the codepath
and errors returned from the kernel space. This patch is retained
for the backward compatibility.

The next one introduces a new more clean implementation of the
EVENTFD_COPY2 ioctl that allocates a new fd for the `struct file'
being stolen.

The last patch uses this new mechanism in the DPDK userspace.

Pavel Boldin (3):
  vhost: eventfd_link: refactoring EVENTFD_COPY handler
  vhost: add EVENTFD_COPY2 ioctl
  vhost: using EVENTFD_COPY2

 lib/librte_vhost/eventfd_link/eventfd_link.c | 249 ++++++++++++++++++---------
 lib/librte_vhost/eventfd_link/eventfd_link.h |  28 ++-
 lib/librte_vhost/vhost_cuse/eventfd_copy.c   |  54 ++++--
 lib/librte_vhost/vhost_cuse/eventfd_copy.h   |   6 +
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |   3 +
 5 files changed, 231 insertions(+), 109 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH v6 1/3] vhost: eventfd_link: refactoring EVENTFD_COPY handler

[Pavel Boldin]

* Move ioctl `EVENTFD_COPY' code to a separate function
* Remove extra #includes
* Introduce function fget_from_files
* Fix ioctl return values

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 181 +++++++++++++++------------
 1 file changed, 100 insertions(+), 81 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 62c45c8..7cbebd4 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -22,18 +22,11 @@
  *   Intel Corporation
  */
 
-#include <linux/eventfd.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/rcupdate.h>
 #include <linux/file.h>
-#include <linux/slab.h>
-#include <linux/fs.h>
-#include <linux/mmu_context.h>
-#include <linux/sched.h>
-#include <asm/mmu_context.h>
 #include <linux/fdtable.h>
+#include <linux/syscalls.h>
 
 #include "eventfd_link.h"
 
@@ -65,9 +58,27 @@ put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
+
+	rcu_read_lock();
+	file = fcheck_files(files, fd);
+	if (file) {
+		if (file->f_mode & FMODE_PATH ||
+			!atomic_long_inc_not_zero(&file->f_count)) {
+
+			file = NULL;
+		}
+	}
+	rcu_read_unlock();
+
+	return file;
+}
 
 static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
 	struct task_struct *task_target = NULL;
@@ -75,91 +86,99 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
 	struct files_struct *files;
 	struct fdtable *fdt;
 	struct eventfd_copy eventfd_copy;
+	long ret = -EFAULT;
 
-	switch (ioctl) {
-	case EVENTFD_COPY:
-		if (copy_from_user(&eventfd_copy, argp,
-			sizeof(struct eventfd_copy)))
-			return -EFAULT;
-
-		/*
-		 * Find the task struct for the target pid
-		 */
-		task_target =
-			pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
-		if (task_target == NULL) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
+	if (copy_from_user(&eventfd_copy, argp, sizeof(struct eventfd_copy)))
+		goto out;
 
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
+	/*
+	 * Find the task struct for the target pid
+	 */
+	ret = -ESRCH;
 
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.source_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-				file = NULL;
-		}
-		rcu_read_unlock();
-		put_files_struct(files);
+	task_target =
+		get_pid_task(find_vpid(eventfd_copy.target_pid), PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_info("Unable to find pid %d\n", eventfd_copy.target_pid);
+		goto out;
+	}
 
-		if (file == NULL) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
+	ret = -ESTALE;
+	files = get_files_struct(current);
+	if (files == NULL) {
+		pr_info("Failed to get current files struct\n");
+		goto out_task;
+	}
 
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		fput(file);
-		filp_close(file, files);
-		fdt = files_fdtable(files);
-		fdt->fd[eventfd_copy.source_fd] = NULL;
-		spin_unlock(&files->file_lock);
-
-		/*
-		 * Find the file struct associated with the target fd.
-		 */
-
-		files = get_files_struct(task_target);
-		if (files == NULL) {
-			pr_debug("Failed to get files struct\n");
-			return -EFAULT;
-		}
+	ret = -EBADF;
+	file = fget_from_files(files, eventfd_copy.source_fd);
 
-		rcu_read_lock();
-		file = fcheck_files(files, eventfd_copy.target_fd);
-		if (file) {
-			if (file->f_mode & FMODE_PATH ||
-				!atomic_long_inc_not_zero(&file->f_count))
-					file = NULL;
-		}
-		rcu_read_unlock();
+	if (file == NULL) {
+		pr_info("Failed to get fd %d from source\n",
+			eventfd_copy.source_fd);
 		put_files_struct(files);
+		goto out_task;
+	}
 
-		if (file == NULL) {
-			pr_debug("Failed to get file from target pid\n");
-			return 0;
-		}
+	/*
+	 * Release the existing eventfd in the source process
+	 */
+	spin_lock(&files->file_lock);
+	fput(file);
+	filp_close(file, files);
+	fdt = files_fdtable(files);
+	fdt->fd[eventfd_copy.source_fd] = NULL;
+	spin_unlock(&files->file_lock);
+
+	put_files_struct(files);
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	ret = -ESTALE;
+	files = get_files_struct(task_target);
+	if (files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	file = fget_from_files(files, eventfd_copy.target_fd);
+	put_files_struct(files);
 
-		/*
-		 * Install the file struct from the target process into the
-		 * file desciptor of the source process,
-		 */
+	if (file == NULL) {
+		pr_info("Failed to get fd %d from target\n",
+			eventfd_copy.target_fd);
+		goto out_task;
+	}
 
-		fd_install(eventfd_copy.source_fd, file);
+	/*
+	 * Install the file struct from the target process into the
+	 * file desciptor of the source process,
+	 */
 
-		return 0;
+	fd_install(eventfd_copy.source_fd, file);
+	ret = 0;
 
-	default:
-		return -ENOIOCTLCMD;
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
+
+static long
+eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+{
+	long ret = -ENOIOCTLCMD;
+
+	switch (ioctl) {
+	case EVENTFD_COPY:
+		ret = eventfd_link_ioctl_copy(arg);
+		break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {
-- 
1.9.1

[dpdk-dev] [PATCH v6 2/3] vhost: add EVENTFD_COPY2 ioctl

[Pavel Boldin]

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 61 ++++++++++++++++++++++++++++
 lib/librte_vhost/eventfd_link/eventfd_link.h | 28 ++++++++++---
 2 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 7cbebd4..c54a938 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -78,6 +78,64 @@ fget_from_files(struct files_struct *files, unsigned fd)
 }
 
 static long
+eventfd_link_ioctl_copy2(unsigned long arg)
+{
+	void __user *argp = (void __user *) arg;
+	struct task_struct *task_target = NULL;
+	struct file *file;
+	struct files_struct *files;
+	struct eventfd_copy2 eventfd_copy2;
+	long ret = -EFAULT;
+
+	if (copy_from_user(&eventfd_copy2, argp, sizeof(struct eventfd_copy2)))
+		goto out;
+
+	/*
+	 * Find the task struct for the target pid
+	 */
+	ret = -ESRCH;
+
+	task_target =
+		get_pid_task(find_vpid(eventfd_copy2.pid), PIDTYPE_PID);
+	if (task_target == NULL) {
+		pr_info("Unable to find pid %d\n", eventfd_copy2.pid);
+		goto out;
+	}
+
+	ret = -ESTALE;
+	files = get_files_struct(task_target);
+	if (files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	file = fget_from_files(files, eventfd_copy2.fd);
+	put_files_struct(files);
+
+	if (file == NULL) {
+		pr_info("Failed to get fd %d from target\n", eventfd_copy2.fd);
+		goto out_task;
+	}
+
+	/*
+	 * Install the file struct from the target process into the
+	 * newly allocated file desciptor of the source process.
+	 */
+	ret = get_unused_fd_flags(eventfd_copy2.flags);
+	if (ret < 0) {
+		fput(file);
+		goto out_task;
+	}
+	fd_install(ret, file);
+
+out_task:
+	put_task_struct(task_target);
+out:
+	return ret;
+}
+
+static long
 eventfd_link_ioctl_copy(unsigned long arg)
 {
 	void __user *argp = (void __user *) arg;
@@ -176,6 +234,9 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
 	case EVENTFD_COPY:
 		ret = eventfd_link_ioctl_copy(arg);
 		break;
+	case EVENTFD_COPY2:
+		ret = eventfd_link_ioctl_copy2(arg);
+		break;
 	}
 
 	return ret;
diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.h b/lib/librte_vhost/eventfd_link/eventfd_link.h
index ea619ec..5ebc20b 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.h
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.h
@@ -61,11 +61,6 @@
 #define _EVENTFD_LINK_H_
 
 /*
- * ioctl to copy an fd entry in calling process to an fd in a target process
- */
-#define EVENTFD_COPY 1
-
-/*
  * arguements for the EVENTFD_COPY ioctl
  */
 struct eventfd_copy {
@@ -73,4 +68,27 @@ struct eventfd_copy {
 	unsigned source_fd; /* fd in the calling pid */
 	pid_t target_pid; /* pid of the target pid */
 };
+
+/*
+ * ioctl to copy an fd entry in calling process to an fd in a target process
+ * NOTE: this one should be
+ * #define EVENTFD_COPY _IOWR('D', 1, struct eventfd_copy) actually
+ */
+#define EVENTFD_COPY 1
+
+/*
+ * arguments for the EVENTFD_COPY2 ioctl
+ */
+struct eventfd_copy2 {
+	unsigned fd; /* fd to steal */
+	pid_t pid; /* pid of the process to steal from */
+	unsigned flags; /* flags to allocate new fd with */
+};
+
+/*
+ * ioctl to copy an fd entry from the target process into newly allocated
+ * fd in the calling process
+ */
+#define EVENTFD_COPY2 _IOW('D', 2, struct eventfd_copy2)
+
 #endif /* _EVENTFD_LINK_H_ */
-- 
1.9.1

[dpdk-dev] [PATCH v6 3/3] vhost: using EVENTFD_COPY2

[Pavel Boldin]

Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
 lib/librte_vhost/vhost_cuse/eventfd_copy.c   | 54 ++++++++++++++++++----------
 lib/librte_vhost/vhost_cuse/eventfd_copy.h   |  6 ++++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  3 ++
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/lib/librte_vhost/vhost_cuse/eventfd_copy.c b/lib/librte_vhost/vhost_cuse/eventfd_copy.c
index 4d697a2..154b32a 100644
--- a/lib/librte_vhost/vhost_cuse/eventfd_copy.c
+++ b/lib/librte_vhost/vhost_cuse/eventfd_copy.c
@@ -46,6 +46,32 @@
 
 static const char eventfd_cdev[] = "/dev/eventfd-link";
 
+static int eventfd_link = -1;
+
+int
+eventfd_init(void)
+{
+	if (eventfd_link >= 0)
+		return 0;
+
+	eventfd_link = open(eventfd_cdev, O_RDWR);
+	if (eventfd_link < 0) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"eventfd_link module is not loaded\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+int
+eventfd_free(void)
+{
+	if (eventfd_link >= 0)
+		close(eventfd_link);
+	return 0;
+}
+
 /*
  * This function uses the eventfd_link kernel module to copy an eventfd file
  * descriptor provided by QEMU in to our process space.
@@ -53,36 +79,26 @@ static const char eventfd_cdev[] = "/dev/eventfd-link";
 int
 eventfd_copy(int target_fd, int target_pid)
 {
-	int eventfd_link, ret;
-	struct eventfd_copy eventfd_copy;
-	int fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	int ret;
+	struct eventfd_copy2 eventfd_copy2;
 
-	if (fd == -1)
-		return -1;
 
 	/* Open the character device to the kernel module. */
 	/* TODO: check this earlier rather than fail until VM boots! */
-	eventfd_link = open(eventfd_cdev, O_RDWR);
-	if (eventfd_link < 0) {
-		RTE_LOG(ERR, VHOST_CONFIG,
-			"eventfd_link module is not loaded\n");
-		close(fd);
+	if (eventfd_init() < 0)
 		return -1;
-	}
 
-	eventfd_copy.source_fd = fd;
-	eventfd_copy.target_fd = target_fd;
-	eventfd_copy.target_pid = target_pid;
+	eventfd_copy2.fd = target_fd;
+	eventfd_copy2.pid = target_pid;
+	eventfd_copy2.flags = O_NONBLOCK | O_CLOEXEC;
 	/* Call the IOCTL to copy the eventfd. */
-	ret = ioctl(eventfd_link, EVENTFD_COPY, &eventfd_copy);
-	close(eventfd_link);
+	ret = ioctl(eventfd_link, EVENTFD_COPY2, &eventfd_copy2);
 
 	if (ret < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"EVENTFD_COPY ioctl failed\n");
-		close(fd);
+			"EVENTFD_COPY2 ioctl failed\n");
 		return -1;
 	}
 
-	return fd;
+	return ret;
 }
diff --git a/lib/librte_vhost/vhost_cuse/eventfd_copy.h b/lib/librte_vhost/vhost_cuse/eventfd_copy.h
index 19ae30d..5f446ca 100644
--- a/lib/librte_vhost/vhost_cuse/eventfd_copy.h
+++ b/lib/librte_vhost/vhost_cuse/eventfd_copy.h
@@ -34,6 +34,12 @@
 #define _EVENTFD_H
 
 int
+eventfd_init(void);
+
+int
+eventfd_free(void);
+
+int
 eventfd_copy(int target_fd, int target_pid);
 
 #endif
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 1ae7c49..ae7ad8d 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -373,6 +373,9 @@ rte_vhost_driver_register(const char *dev_name)
 		return -1;
 	}
 
+	if (eventfd_init() < 0)
+		return -1;
+
 	/*
 	 * The device name is created. This is passed to QEMU so that it can
 	 * register the device with our application.
-- 
1.9.1

Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2

[Pavel Boldin]

Huawei, Thomas,

Please find an updated patchset in the appropriate mail thread.

With best regards,
Pavel

On Mon, Oct 26, 2015 at 3:45 AM, Xie, Huawei <huawei.xie@intel.com> wrote:

> On 10/21/2015 8:16 PM, Pavel Boldin wrote:
> > Xie,
> >
> > Please find my comments intermixed below.
> >
> > On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <huawei.xie@intel.com
> > <mailto:huawei.xie@intel.com>> wrote:
> >
> >     Thanks Pavel for this work.
> >     This is what we think is the better implementation for eventfd
> >     proxy, in
> >     our last review.
> >     Could you add an additional patch to remove the old implementation?
> >
> > I'm not really sure if we should do it -- imagine upgrading from one
> > version of DPDK to another.
> > Given the current implementation there is a backward compatibility.
> I couldn't image the case any one would run old dpdk app with the new
> dpdk module. However I am ok you leave it here, :), we could remove this
> in next release.
> Could you finish rebasing the patch before end of next week, otherwise
> it will lose chance of being merged.
> >
> >
> >
> >     Again, please run checkpatch.pl <http://checkpatch.pl> against
> >     your patch.
> >
> > Oops. Thanks for pointing out.
> >
> >
> >     On 8/29/2015 2:51 AM, Pavel Boldin wrote:
> >
> >     [...]
> >     > +
> >     > +int
> >     > +eventfd_init(void)
> >     > +{
> >     > +     if (eventfd_link > 0)
> >     0 could be valid fd. Change it to:
> >
> > Got it. Thanks.
> >
> >
> >         if (eventfd_link >= 0)
> >     Change elsewhere if i miss it.
> >     > +int
> >     > +eventfd_free(void)
> >     > +{
> >     > +     if (eventfd_link > 0)
> >     same as above:
> >         if (eventfd_link >= 0)
> >
> >     [...]
> >
> >
> > --
> > Sincerely,
> >  Pavel
>
>

Re: [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring

[Xie, Huawei]

On 10/29/2015 2:34 AM, Pavel Boldin wrote:
> The patchset contains an attempt at the refactoring the `eventfd_link`
> kernel module that is used to steal an FD in DPDK.
>
> The first patch refactors old EVENTFD_COPY handler fixing the codepath
> and errors returned from the kernel space. This patch is retained
> for the backward compatibility.
>
> The next one introduces a new more clean implementation of the
> EVENTFD_COPY2 ioctl that allocates a new fd for the `struct file'
> being stolen.
>
> The last patch uses this new mechanism in the DPDK userspace.
Pavel:
Don't forget signoff and version change message next time.
Or if you have time today, could you send a new version?

Acked-by: Huawei Xie <huawei.xie@intel.com>

> Pavel Boldin (3):
>   vhost: eventfd_link: refactoring EVENTFD_COPY handler
>   vhost: add EVENTFD_COPY2 ioctl
>   vhost: using EVENTFD_COPY2
>
>  lib/librte_vhost/eventfd_link/eventfd_link.c | 249 ++++++++++++++++++---------
>  lib/librte_vhost/eventfd_link/eventfd_link.h |  28 ++-
>  lib/librte_vhost/vhost_cuse/eventfd_copy.c   |  54 ++++--
>  lib/librte_vhost/vhost_cuse/eventfd_copy.h   |   6 +
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |   3 +
>  5 files changed, 231 insertions(+), 109 deletions(-)
>

Re: [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring

[Thomas Monjalon]

2015-10-29 18:33, Xie, Huawei:
> On 10/29/2015 2:34 AM, Pavel Boldin wrote:
> > The patchset contains an attempt at the refactoring the `eventfd_link`
> > kernel module that is used to steal an FD in DPDK.
> >
> > The first patch refactors old EVENTFD_COPY handler fixing the codepath
> > and errors returned from the kernel space. This patch is retained
> > for the backward compatibility.
> >
> > The next one introduces a new more clean implementation of the
> > EVENTFD_COPY2 ioctl that allocates a new fd for the `struct file'
> > being stolen.
> >
> > The last patch uses this new mechanism in the DPDK userspace.
> Pavel:
> Don't forget signoff and version change message next time.
> Or if you have time today, could you send a new version?
> 
> Acked-by: Huawei Xie <huawei.xie@intel.com>

Applied, thanks