DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
@ 2015-03-16 16:40 Pavel Boldin
  2015-03-16 17:55 ` Neil Horman
                   ` (2 more replies)
  0 siblings, 3 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-03-16 16:40 UTC (permalink / raw)
  To: dev; +Cc: 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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
  2015-03-16 16:40 [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races Pavel Boldin
@ 2015-03-16 17:55 ` Neil Horman
  2015-03-17  1:29 ` Ouyang, Changchun
  2015-03-18 13:16 ` [dpdk-dev] [PATCH v2] " Pavel Boldin
  2 siblings, 0 replies; 58+ messages in thread
From: Neil Horman @ 2015-03-16 17:55 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
  2015-03-16 16:40 [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races Pavel Boldin
  2015-03-16 17:55 ` Neil Horman
@ 2015-03-17  1:29 ` Ouyang, Changchun
  2015-03-20 15:57   ` Pavel Boldin
  2015-03-18 13:16 ` [dpdk-dev] [PATCH v2] " Pavel Boldin
  2 siblings, 1 reply; 58+ messages in thread
From: Ouyang, Changchun @ 2015-03-17  1:29 UTC (permalink / raw)
  To: Pavel Boldin, dev



> -----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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races
  2015-03-16 16:40 [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races Pavel Boldin
  2015-03-16 17:55 ` Neil Horman
  2015-03-17  1:29 ` Ouyang, Changchun
@ 2015-03-18 13:16 ` Pavel Boldin
  2015-03-23 11:15   ` Thomas Monjalon
  2015-03-23 15:15   ` [dpdk-dev] [PATCH v3] vhost: Refactor module `eventfd_link' Pavel Boldin
  2 siblings, 2 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-03-18 13:16 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
  2015-03-17  1:29 ` Ouyang, Changchun
@ 2015-03-20 15:57   ` Pavel Boldin
  0 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-03-20 15:57 UTC (permalink / raw)
  To: Ouyang, Changchun; +Cc: dev

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
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races
  2015-03-18 13:16 ` [dpdk-dev] [PATCH v2] " Pavel Boldin
@ 2015-03-23 11:15   ` Thomas Monjalon
  2015-03-23 11:36     ` Pavel Boldin
  2015-03-23 15:15   ` [dpdk-dev] [PATCH v3] vhost: Refactor module `eventfd_link' Pavel Boldin
  1 sibling, 1 reply; 58+ messages in thread
From: Thomas Monjalon @ 2015-03-23 11:15 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races
  2015-03-23 11:15   ` Thomas Monjalon
@ 2015-03-23 11:36     ` Pavel Boldin
  2015-03-23 11:44       ` Thomas Monjalon
  0 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-03-23 11:36 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

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
>
>
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races
  2015-03-23 11:36     ` Pavel Boldin
@ 2015-03-23 11:44       ` Thomas Monjalon
  0 siblings, 0 replies; 58+ messages in thread
From: Thomas Monjalon @ 2015-03-23 11:44 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v3] vhost: Refactor module `eventfd_link'
  2015-03-18 13:16 ` [dpdk-dev] [PATCH v2] " Pavel Boldin
  2015-03-23 11:15   ` Thomas Monjalon
@ 2015-03-23 15:15   ` Pavel Boldin
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
  1 sibling, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-03-23 15:15 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v4 0/5] Refactor module `eventfd_link'
  2015-03-23 15:15   ` [dpdk-dev] [PATCH v3] vhost: Refactor module `eventfd_link' Pavel Boldin
@ 2015-04-02 17:01     ` Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
                         ` (5 more replies)
  0 siblings, 6 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-02 17:01 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
@ 2015-04-02 17:01       ` Pavel Boldin
  2015-05-07  7:57         ` Xie, Huawei
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 2/5] vhost: eventfd_link: add function fget_from_files Pavel Boldin
                         ` (4 subsequent siblings)
  5 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-04-02 17:01 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v4 2/5] vhost: eventfd_link: add function fget_from_files
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
@ 2015-04-02 17:01       ` Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 3/5] vhost: eventfd_link: fix ioctl return values Pavel Boldin
                         ` (3 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-02 17:01 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v4 3/5] vhost: eventfd_link: fix ioctl return values
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 2/5] vhost: eventfd_link: add function fget_from_files Pavel Boldin
@ 2015-04-02 17:01       ` Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
                         ` (2 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-02 17:01 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v4 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
                         ` (2 preceding siblings ...)
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 3/5] vhost: eventfd_link: fix ioctl return values Pavel Boldin
@ 2015-04-02 17:01       ` Pavel Boldin
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 5/5] vhost: eventfd_link: removing extra #includes Pavel Boldin
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-02 17:01 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v4 5/5] vhost: eventfd_link: removing extra #includes
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
                         ` (3 preceding siblings ...)
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
@ 2015-04-02 17:01       ` Pavel Boldin
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-02 17:01 UTC (permalink / raw)
  To: dev

---
 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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link'
  2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
                         ` (4 preceding siblings ...)
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 5/5] vhost: eventfd_link: removing extra #includes Pavel Boldin
@ 2015-04-16 11:48       ` Pavel Boldin
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
                           ` (5 more replies)
  5 siblings, 6 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-16 11:48 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
@ 2015-04-16 11:48         ` Pavel Boldin
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
                             ` (3 more replies)
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 2/5] vhost: eventfd_link: add function fget_from_files Pavel Boldin
                           ` (4 subsequent siblings)
  5 siblings, 4 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-16 11:48 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 2/5] vhost: eventfd_link: add function fget_from_files
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
@ 2015-04-16 11:48         ` Pavel Boldin
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 3/5] vhost: eventfd_link: fix ioctl return values Pavel Boldin
                           ` (3 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-16 11:48 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 3/5] vhost: eventfd_link: fix ioctl return values
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 2/5] vhost: eventfd_link: add function fget_from_files Pavel Boldin
@ 2015-04-16 11:48         ` Pavel Boldin
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
                           ` (2 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-16 11:48 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
                           ` (2 preceding siblings ...)
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 3/5] vhost: eventfd_link: fix ioctl return values Pavel Boldin
@ 2015-04-16 11:48         ` Pavel Boldin
  2015-05-07  6:54           ` Xie, Huawei
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 5/5] vhost: eventfd_link: removing extra #includes Pavel Boldin
  2015-04-28 14:35         ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Thomas Monjalon
  5 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-04-16 11:48 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 5/5] vhost: eventfd_link: removing extra #includes
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
                           ` (3 preceding siblings ...)
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
@ 2015-04-16 11:48         ` Pavel Boldin
  2015-04-28 14:35         ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Thomas Monjalon
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-04-16 11:48 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link'
  2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
                           ` (4 preceding siblings ...)
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 5/5] vhost: eventfd_link: removing extra #includes Pavel Boldin
@ 2015-04-28 14:35         ` Thomas Monjalon
  2015-05-04  5:29           ` Xie, Huawei
  5 siblings, 1 reply; 58+ messages in thread
From: Thomas Monjalon @ 2015-04-28 14:35 UTC (permalink / raw)
  To: Changchun Ouyang, Huawei Xie; +Cc: dev

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(-)

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link'
  2015-04-28 14:35         ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Thomas Monjalon
@ 2015-05-04  5:29           ` Xie, Huawei
  0 siblings, 0 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-05-04  5:29 UTC (permalink / raw)
  To: Thomas Monjalon, Ouyang, Changchun; +Cc: dev

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(-)
>
>


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
@ 2015-05-07  6:54           ` Xie, Huawei
  2015-06-17 15:24             ` Thomas Monjalon
  0 siblings, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-05-07  6:54 UTC (permalink / raw)
  To: Pavel Boldin, dev

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:


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function
  2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
@ 2015-05-07  7:57         ` Xie, Huawei
       [not found]           ` <CACf4_B8MDfkaEk5jMjMbEw9F8LsBVLA9TUWwOBU=SCAxzFmSFw@mail.gmail.com>
  0 siblings, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-05-07  7:57 UTC (permalink / raw)
  To: Pavel Boldin, dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function
       [not found]           ` <CACf4_B8MDfkaEk5jMjMbEw9F8LsBVLA9TUWwOBU=SCAxzFmSFw@mail.gmail.com>
@ 2015-05-18  6:06             ` Xie, Huawei
  2015-05-18  9:16               ` Pavel Boldin
  2015-06-18  3:08             ` Xie, Huawei
  1 sibling, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-05-18  6:06 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function
  2015-05-18  6:06             ` Xie, Huawei
@ 2015-05-18  9:16               ` Pavel Boldin
  0 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-05-18  9:16 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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
>
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-05-07  6:54           ` Xie, Huawei
@ 2015-06-17 15:24             ` Thomas Monjalon
  2015-07-09  0:59               ` Thomas Monjalon
  2015-07-10 14:27               ` Xie, Huawei
  0 siblings, 2 replies; 58+ messages in thread
From: Thomas Monjalon @ 2015-06-17 15:24 UTC (permalink / raw)
  To: Xie, Huawei, Pavel Boldin; +Cc: dev

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?

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function
       [not found]           ` <CACf4_B8MDfkaEk5jMjMbEw9F8LsBVLA9TUWwOBU=SCAxzFmSFw@mail.gmail.com>
  2015-05-18  6:06             ` Xie, Huawei
@ 2015-06-18  3:08             ` Xie, Huawei
  1 sibling, 0 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-06-18  3:08 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-06-17 15:24             ` Thomas Monjalon
@ 2015-07-09  0:59               ` Thomas Monjalon
  2015-07-10 14:27               ` Xie, Huawei
  1 sibling, 0 replies; 58+ messages in thread
From: Thomas Monjalon @ 2015-07-09  0:59 UTC (permalink / raw)
  To: Xie, Huawei, changchun.ouyang; +Cc: dev, Pavel Boldin

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?

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-06-17 15:24             ` Thomas Monjalon
  2015-07-09  0:59               ` Thomas Monjalon
@ 2015-07-10 14:27               ` Xie, Huawei
  2015-07-10 14:50                 ` Pavel Boldin
  1 sibling, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-07-10 14:27 UTC (permalink / raw)
  To: Thomas Monjalon, Pavel Boldin; +Cc: dev

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.

>
>


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-10 14:27               ` Xie, Huawei
@ 2015-07-10 14:50                 ` Pavel Boldin
  2015-07-10 15:32                   ` Xie, Huawei
  2015-07-10 15:42                   ` Xie, Huawei
  0 siblings, 2 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-07-10 14:50 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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.
>
> >
> >
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-10 14:50                 ` Pavel Boldin
@ 2015-07-10 15:32                   ` Xie, Huawei
  2015-07-10 15:42                   ` Xie, Huawei
  1 sibling, 0 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-07-10 15:32 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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.

>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-10 14:50                 ` Pavel Boldin
  2015-07-10 15:32                   ` Xie, Huawei
@ 2015-07-10 15:42                   ` Xie, Huawei
  2015-07-10 15:49                     ` Thomas Monjalon
  2015-07-11 15:08                     ` Pavel Boldin
  1 sibling, 2 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-07-10 15:42 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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.

>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-10 15:42                   ` Xie, Huawei
@ 2015-07-10 15:49                     ` Thomas Monjalon
  2015-07-10 16:06                       ` Xie, Huawei
  2015-07-11 15:08                     ` Pavel Boldin
  1 sibling, 1 reply; 58+ messages in thread
From: Thomas Monjalon @ 2015-07-10 15:49 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev, Pavel Boldin

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.
> 
> >
> >
> 
> 
> 

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-10 15:49                     ` Thomas Monjalon
@ 2015-07-10 16:06                       ` Xie, Huawei
  0 siblings, 0 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-07-10 16:06 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Pavel Boldin

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.
>>
>>>
>>
>>
>
>
>


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-10 15:42                   ` Xie, Huawei
  2015-07-10 15:49                     ` Thomas Monjalon
@ 2015-07-11 15:08                     ` Pavel Boldin
  2015-07-13  1:59                       ` Xie, Huawei
  1 sibling, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-07-11 15:08 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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.
>
> >
> >
>
>
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-11 15:08                     ` Pavel Boldin
@ 2015-07-13  1:59                       ` Xie, Huawei
  2015-07-19 12:39                         ` Pavel Boldin
  0 siblings, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-07-13  1:59 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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.
>
>     >
>     >
>
>
>
>


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close
  2015-07-13  1:59                       ` Xie, Huawei
@ 2015-07-19 12:39                         ` Pavel Boldin
  0 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-07-19 12:39 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
@ 2015-08-28 18:51           ` Pavel Boldin
  2015-09-23 20:25             ` Pavel Boldin
                               ` (5 more replies)
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
                             ` (2 subsequent siblings)
  3 siblings, 6 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-08-28 18:51 UTC (permalink / raw)
  To: dev

* 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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
@ 2015-08-28 18:51           ` Pavel Boldin
  2015-10-20  9:43             ` Xie, Huawei
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2 Pavel Boldin
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 4/4] DO NOT MERGE: Tests for new eventfd_link module Pavel Boldin
  3 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-08-28 18:51 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
@ 2015-08-28 18:51           ` Pavel Boldin
  2015-10-20  9:52             ` Xie, Huawei
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 4/4] DO NOT MERGE: Tests for new eventfd_link module Pavel Boldin
  3 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-08-28 18:51 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v5 4/4] DO NOT MERGE: Tests for new eventfd_link module
  2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
                             ` (2 preceding siblings ...)
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2 Pavel Boldin
@ 2015-08-28 18:51           ` Pavel Boldin
  3 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-08-28 18:51 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
@ 2015-09-23 20:25             ` Pavel Boldin
  2015-09-29 19:42               ` Thomas Monjalon
  2015-10-20  9:06             ` Xie, Huawei
                               ` (4 subsequent siblings)
  5 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-09-23 20:25 UTC (permalink / raw)
  To: dev

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
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler
  2015-09-23 20:25             ` Pavel Boldin
@ 2015-09-29 19:42               ` Thomas Monjalon
  2015-09-29 23:29                 ` Pavel Boldin
  0 siblings, 1 reply; 58+ messages in thread
From: Thomas Monjalon @ 2015-09-29 19:42 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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 ;)

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler
  2015-09-29 19:42               ` Thomas Monjalon
@ 2015-09-29 23:29                 ` Pavel Boldin
  0 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-09-29 23:29 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
  2015-09-23 20:25             ` Pavel Boldin
@ 2015-10-20  9:06             ` Xie, Huawei
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring Pavel Boldin
                               ` (3 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-10-20  9:06 UTC (permalink / raw)
  To: Pavel Boldin, dev

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.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
@ 2015-10-20  9:43             ` Xie, Huawei
  0 siblings, 0 replies; 58+ messages in thread
From: Xie, Huawei @ 2015-10-20  9:43 UTC (permalink / raw)
  To: Pavel Boldin, dev


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.



^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2 Pavel Boldin
@ 2015-10-20  9:52             ` Xie, Huawei
  2015-10-21 12:16               ` Pavel Boldin
  0 siblings, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-10-20  9:52 UTC (permalink / raw)
  To: Pavel Boldin, dev, Long, Thomas

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)

[...]

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2
  2015-10-20  9:52             ` Xie, Huawei
@ 2015-10-21 12:16               ` Pavel Boldin
  2015-10-26  1:45                 ` Xie, Huawei
  0 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-10-21 12:16 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2
  2015-10-21 12:16               ` Pavel Boldin
@ 2015-10-26  1:45                 ` Xie, Huawei
  2015-10-28 18:35                   ` Pavel Boldin
  0 siblings, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-10-26  1:45 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
  2015-09-23 20:25             ` Pavel Boldin
  2015-10-20  9:06             ` Xie, Huawei
@ 2015-10-28 18:33             ` Pavel Boldin
  2015-10-29 18:33               ` Xie, Huawei
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 1/3] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
                               ` (2 subsequent siblings)
  5 siblings, 1 reply; 58+ messages in thread
From: Pavel Boldin @ 2015-10-28 18:33 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v6 1/3] vhost: eventfd_link: refactoring EVENTFD_COPY handler
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
                               ` (2 preceding siblings ...)
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring Pavel Boldin
@ 2015-10-28 18:33             ` Pavel Boldin
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 2/3] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 3/3] vhost: using EVENTFD_COPY2 Pavel Boldin
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-10-28 18:33 UTC (permalink / raw)
  To: dev

* 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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v6 2/3] vhost: add EVENTFD_COPY2 ioctl
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
                               ` (3 preceding siblings ...)
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 1/3] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
@ 2015-10-28 18:33             ` Pavel Boldin
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 3/3] vhost: using EVENTFD_COPY2 Pavel Boldin
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-10-28 18:33 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [dpdk-dev] [PATCH v6 3/3] vhost: using EVENTFD_COPY2
  2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
                               ` (4 preceding siblings ...)
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 2/3] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
@ 2015-10-28 18:33             ` Pavel Boldin
  5 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-10-28 18:33 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2
  2015-10-26  1:45                 ` Xie, Huawei
@ 2015-10-28 18:35                   ` Pavel Boldin
  0 siblings, 0 replies; 58+ messages in thread
From: Pavel Boldin @ 2015-10-28 18:35 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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
>
>

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring
  2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring Pavel Boldin
@ 2015-10-29 18:33               ` Xie, Huawei
  2015-10-30 19:10                 ` Thomas Monjalon
  0 siblings, 1 reply; 58+ messages in thread
From: Xie, Huawei @ 2015-10-29 18:33 UTC (permalink / raw)
  To: Pavel Boldin, dev

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(-)
>


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring
  2015-10-29 18:33               ` Xie, Huawei
@ 2015-10-30 19:10                 ` Thomas Monjalon
  0 siblings, 0 replies; 58+ messages in thread
From: Thomas Monjalon @ 2015-10-30 19:10 UTC (permalink / raw)
  To: dev, Pavel Boldin

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

^ permalink raw reply	[flat|nested] 58+ messages in thread

end of thread, other threads:[~2015-10-30 19:11 UTC | newest]

Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-16 16:40 [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races Pavel Boldin
2015-03-16 17:55 ` Neil Horman
2015-03-17  1:29 ` Ouyang, Changchun
2015-03-20 15:57   ` Pavel Boldin
2015-03-18 13:16 ` [dpdk-dev] [PATCH v2] " Pavel Boldin
2015-03-23 11:15   ` Thomas Monjalon
2015-03-23 11:36     ` Pavel Boldin
2015-03-23 11:44       ` Thomas Monjalon
2015-03-23 15:15   ` [dpdk-dev] [PATCH v3] vhost: Refactor module `eventfd_link' Pavel Boldin
2015-04-02 17:01     ` [dpdk-dev] [PATCH v4 0/5] " Pavel Boldin
2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
2015-05-07  7:57         ` Xie, Huawei
     [not found]           ` <CACf4_B8MDfkaEk5jMjMbEw9F8LsBVLA9TUWwOBU=SCAxzFmSFw@mail.gmail.com>
2015-05-18  6:06             ` Xie, Huawei
2015-05-18  9:16               ` Pavel Boldin
2015-06-18  3:08             ` Xie, Huawei
2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 2/5] vhost: eventfd_link: add function fget_from_files Pavel Boldin
2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 3/5] vhost: eventfd_link: fix ioctl return values Pavel Boldin
2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
2015-04-02 17:01       ` [dpdk-dev] [PATCH v4 5/5] vhost: eventfd_link: removing extra #includes Pavel Boldin
2015-04-16 11:48       ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Pavel Boldin
2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 1/5] vhost: eventfd_link: moving ioctl to a function Pavel Boldin
2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 1/4] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
2015-09-23 20:25             ` Pavel Boldin
2015-09-29 19:42               ` Thomas Monjalon
2015-09-29 23:29                 ` Pavel Boldin
2015-10-20  9:06             ` Xie, Huawei
2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 0/3] vhost: eventfd_link refactoring Pavel Boldin
2015-10-29 18:33               ` Xie, Huawei
2015-10-30 19:10                 ` Thomas Monjalon
2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 1/3] vhost: eventfd_link: refactoring EVENTFD_COPY handler Pavel Boldin
2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 2/3] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
2015-10-28 18:33             ` [dpdk-dev] [PATCH v6 3/3] vhost: using EVENTFD_COPY2 Pavel Boldin
2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 2/4] vhost: add EVENTFD_COPY2 ioctl Pavel Boldin
2015-10-20  9:43             ` Xie, Huawei
2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 3/4] vhost: using EVENTFD_COPY2 Pavel Boldin
2015-10-20  9:52             ` Xie, Huawei
2015-10-21 12:16               ` Pavel Boldin
2015-10-26  1:45                 ` Xie, Huawei
2015-10-28 18:35                   ` Pavel Boldin
2015-08-28 18:51           ` [dpdk-dev] [PATCH v5 4/4] DO NOT MERGE: Tests for new eventfd_link module Pavel Boldin
2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 2/5] vhost: eventfd_link: add function fget_from_files Pavel Boldin
2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 3/5] vhost: eventfd_link: fix ioctl return values Pavel Boldin
2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close Pavel Boldin
2015-05-07  6:54           ` Xie, Huawei
2015-06-17 15:24             ` Thomas Monjalon
2015-07-09  0:59               ` Thomas Monjalon
2015-07-10 14:27               ` Xie, Huawei
2015-07-10 14:50                 ` Pavel Boldin
2015-07-10 15:32                   ` Xie, Huawei
2015-07-10 15:42                   ` Xie, Huawei
2015-07-10 15:49                     ` Thomas Monjalon
2015-07-10 16:06                       ` Xie, Huawei
2015-07-11 15:08                     ` Pavel Boldin
2015-07-13  1:59                       ` Xie, Huawei
2015-07-19 12:39                         ` Pavel Boldin
2015-04-16 11:48         ` [dpdk-dev] [PATCH v5 5/5] vhost: eventfd_link: removing extra #includes Pavel Boldin
2015-04-28 14:35         ` [dpdk-dev] [PATCH v5 0/5] Refactor module `eventfd_link' Thomas Monjalon
2015-05-04  5:29           ` Xie, Huawei

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git