Fix `eventfd_link&#39; module leakages and races

Quoted 36 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Pavel Boldin
Sent: Tuesday, March 17, 2015 12:41 AM
To: [email protected]
Cc: Pavel Boldin
Subject: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races
From: Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>
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(-)
examples/vhost/eventfd_link/eventfd_link.c +1 -1
index 69470ba..9f1f8fb 100644
@@ -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)
 {

examples/vhost/eventfd_link/eventfd_link.c line 53

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +3 -3

 	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

Quoted 279 lines

examples/vhost/eventfd_link/eventfd_link.c +113 ~~-87~~

 	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

examples/vhost/eventfd_link/eventfd_link.c +6 -8

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

examples/vhost/eventfd_link/eventfd_link.c +6 -8

-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

A message that is not in the archive (sent off-list or not yet received): F52918179C57134FAEC9EA62FA2F962511A5E173-E2R4CRU6q/[email protected]

Pavel Boldin

author

2015-03-20 15:57 UTC #5 lore raw

Changchun,

Please review the updated patch.

Pavel

Quoted 349 lines · Changchun

Quoted 9 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Pavel Boldin
Sent: Tuesday, March 17, 2015 12:41 AM
To: [email protected]
Cc: Pavel Boldin
Subject: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races

Quoted 3 lines

From: Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>
The `eventfd_link' module provides an API to "steal" fd from another

process

Quoted 4 lines

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

Quoted 20 lines

reference counter.

Fix these bugs and refactor the module.

---
 examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++-----
--------
 1 file changed, 130 insertions(+), 106 deletions(-)

examples/vhost/eventfd_link/eventfd_link.c +1 -1

index 69470ba..9f1f8fb 100644
@@ -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)
 {

examples/vhost/eventfd_link/eventfd_link.c line 53

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +3 -3

      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

Quoted 50 lines

examples/vhost/eventfd_link/eventfd_link.c +37 -3

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

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task) · line 72

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +5 -1

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

Quoted 40 lines

examples/vhost/eventfd_link/eventfd_link.c +33 -4

+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;
+
+     /*

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +10 -0

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

Quoted 49 lines

examples/vhost/eventfd_link/eventfd_link.c +28 ~~-14~~

+
+     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;
-                     }
-

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +0 -9

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

Quoted 9 lines

examples/vhost/eventfd_link/eventfd_link.c +0 -8

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

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +0 -9

-
-                     if (file == NULL)
-                     {
-                             printk (KERN_DEBUG "Failed to get file from
 source pid\n");
-                             return 0;
-                     }
-
-                     /*
-                      * Release the existing eventfd in the source

process

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +0 ~~-10~~

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

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 12 lines

examples/vhost/eventfd_link/eventfd_link.c +0 ~~-11~~

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

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +0 -9

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

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +0 -9

-                     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

Quoted 10 lines

the

examples/vhost/eventfd_link/eventfd_link.c +2 -7

-                      * file desciptor of the source process,
-                      */
-
-                     fd_install (eventfd_copy.source_fd, file);
-
-                     return 0;
-
+                     ret = eventfd_link_ioctl_copy(arg);
+                     break;

examples/vhost/eventfd_link/eventfd_link.c get_files_struct (struct task_struct *task)

Quoted 10 lines

examples/vhost/eventfd_link/eventfd_link.c +4 -1

              default:
-                     return -ENOIOCTLCMD;
+                     ret = -ENOIOCTLCMD;
+                     break;
      }
+
+     return ret;
 }
 
 static const struct file_operations eventfd_link_fops = { @@ -184,21

+209,20

Quoted 29 lines

@@ static struct miscdevice eventfd_link_misc = { };

static int __init

examples/vhost/eventfd_link/eventfd_link.c +6 -8

-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

Pavel Boldin

author

2015-03-18 13:16 UTC #6 lore raw

PATCH v2 1/1 Fix `eventfd_link' module leakages and races

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

lib/librte_vhost/eventfd_link/eventfd_link.c +125 ~~-87~~

index 7755dd6..57b0a8a 100644
@@ -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 = {

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): 1426684571-14782-1-git-send-email-pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]

Thomas Monjalon

2015-03-23 11:15 UTC #8 lore raw

Hi Pavel,

You forgot the Signed-off-by line.

Huawei, Changchun, any comment on this patch?

2015-03-18 15:16, Pavel Boldin:

Quoted 6 lines

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.

Quoted 1 line

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

Pavel Boldin

author

2015-03-23 11:36 UTC #9 lore raw

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

Quoted 28 lines · Thomas Monjalon

Hi Pavel,
You forgot the Signed-off-by line.
Huawei, Changchun, any comment on this patch?
2015-03-18 15:16, Pavel Boldin:
Quoted 5 lines
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
Quoted 1 line
a reference counter.
As there are 2 bugs, you should provide 2 patches.
Quoted 1 line
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

A message that is not in the archive (sent off-list or not yet received): CACf4_B8buRSAPCFO+fWJjQ8it5n0cgN789jSrsB6Bc-cJGOyHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Thomas Monjalon

2015-03-23 11:44 UTC #11 lore raw

2015-03-23 13:36, Pavel Boldin:

Quoted 17 lines

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

Quoted 7 lines

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

Quoted 27 lines

Quoted 24 lines · Thomas Monjalon
Hi Pavel,
You forgot the Signed-off-by line.
Huawei, Changchun, any comment on this patch?
2015-03-18 15:16, Pavel Boldin:
Quoted 5 lines
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
Quoted 1 line
a reference counter.
As there are 2 bugs, you should provide 2 patches.
Quoted 1 line
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

Pavel Boldin

author

2015-03-23 15:15 UTC #12 lore raw

PATCH v3 1/1 vhost: Refactor module `eventfd_link'

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-nYU0QVwCCFFWk0Htik3J/[email protected]>

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

lib/librte_vhost/eventfd_link/eventfd_link.c +98 ~~-95~~

index 7755dd6..d10e25f 100644
@@ -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 = {

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): 1427123731-15654-1-git-send-email-pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]

Pavel Boldin

author

2015-04-02 17:01 UTC #14 lore raw

v4 cover Refactor module `eventfd_link'

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

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): 1427994080-10163-1-git-send-email-pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]

Pavel Boldin

author

2015-04-02 17:01 UTC #16 lore raw

PATCH v4 1/5 vhost: eventfd_link: moving ioctl to a function

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

lib/librte_vhost/eventfd_link/eventfd_link.c +89 ~~-82~~

index 62c45c8..d7cb81f 100644
@@ -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 = {

Signature · 1 line

1.9.1

Xie, Huawei

2015-05-07 07:57 UTC #17 lore raw

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

Quoted 1 line · Pavel Boldin

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

Pavel:
There is no necessity to inline this function.
/huawei

A message that is not in the archive (sent off-list or not yet received): C37D651A908B024F974696C65296B57B0F476887-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org

Pavel Boldin

author

2015-05-07 13:16 UTC #19 lore raw

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

Quoted 5 lines · Huawei

Quoted 1 line · Pavel Boldin
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.

Pavel

Quoted 2 lines

/huawei

Xie, Huawei

2015-05-18 06:07 UTC #20 lore raw

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

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

On Thu, May 7, 2015 at 10:57 AM, Xie, Huawei <[email protected]<mailto:[email protected]>> wrote:

Quoted 1 line · Pavel Boldin

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

Pavel Boldin

2015-05-18 09:16 UTC #21 lore raw

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

Quoted 12 lines · Huawei

On 5/7/2015 9:17 PM, Pavel Boldin wrote:
On Thu, May 7, 2015 at 10:57 AM, Xie, Huawei <[email protected]<mailto:
[email protected]>> wrote:
Quoted 1 line · Pavel Boldin
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.

Quoted 7 lines

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

Quoted 7 lines

/huawei

Xie, Huawei

2015-06-18 03:08 UTC #22 lore raw

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

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.

Pavel Boldin

author

2015-04-02 17:01 UTC #23 lore raw

PATCH v4 2/5 vhost: eventfd_link: add function fget_from_files

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

lib/librte_vhost/eventfd_link/eventfd_link.c +20 ~~-16~~

index d7cb81f..2a29999 100644
@@ -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) {

Signature · 1 line

1.9.1

Pavel Boldin

author

2015-04-02 17:01 UTC #24 lore raw

PATCH v4 3/5 vhost: eventfd_link: fix ioctl return values

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

lib/librte_vhost/eventfd_link/eventfd_link.c +30 ~~-16~~

index 2a29999..0a06594 100644
@@ -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

Signature · 1 line

1.9.1

Pavel Boldin

author

2015-04-02 17:01 UTC #25 lore raw

PATCH v4 4/5 vhost: eventfd_link: replace copy-pasted sys_close

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

lib/librte_vhost/eventfd_link/eventfd_link.c +12 ~~-37~~

index 0a06594..9bc52a3 100644
@@ -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:

Signature · 1 line

1.9.1

Pavel Boldin

author

2015-04-02 17:01 UTC #26 lore raw

PATCH v4 5/5 vhost: eventfd_link: removing extra #includes

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

lib/librte_vhost/eventfd_link/eventfd_link.c +1 -8

index 9bc52a3..0ee7357 100644
@@ -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"

Signature · 1 line

1.9.1

Pavel Boldin

author

2015-04-16 11:48 UTC #27 lore raw

v5 cover Refactor module `eventfd_link'

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

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

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): 1429184910-30186-1-git-send-email-pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]

Pavel Boldin

author

2015-04-16 11:48 UTC #29 lore raw

PATCH v5 1/5 vhost: eventfd_link: moving ioctl to a function

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

Signed-off-by Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 171 ++++++++++++++-------------
 1 file changed, 89 insertions(+), 82 deletions(-)

lib/librte_vhost/eventfd_link/eventfd_link.c +89 ~~-82~~

index 62c45c8..d7cb81f 100644
@@ -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 = {

Signature · 1 line

1.9.1

Pavel Boldin

2015-08-28 18:51 UTC #30 lore raw

PATCH v5 1/4 vhost: eventfd_link: refactoring EVENTFD_COPY handler

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

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 188 +++++++++++++++------------
 1 file changed, 103 insertions(+), 85 deletions(-)

lib/librte_vhost/eventfd_link/eventfd_link.c +103 ~~-85~~

index 62c45c8..5ba1068 100644
@@ -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 = {

Signature · 1 line

1.9.1

Pavel Boldin

2015-08-28 18:51 UTC #31 lore raw

PATCH v5 2/4 vhost: add EVENTFD_COPY2 ioctl

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

lib/librte_vhost/eventfd_link/eventfd_link.c +61 -0

index 5ba1068..0264365 100644
@@ -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;

lib/librte_vhost/eventfd_link/eventfd_link.h +23 -5

index ea619ec..5ebc20b 100644
@@ -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_ */

Signature · 1 line

1.9.1

Xie, Huawei

2015-10-20 09:43 UTC #32 lore raw

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

Quoted 3 lines · Pavel Boldin

Signed-off-by Pavel Boldin <[email protected]>
---

OK with the patch. Please run checkpatch.pl against your patch.

Pavel Boldin

2015-08-28 18:51 UTC #33 lore raw

PATCH v5 3/4 vhost: using EVENTFD_COPY2

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

lib/librte_vhost/vhost_cuse/eventfd_copy.c +35 ~~-19~~

index 4d697a2..1625163 100644
@@ -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;
 }

lib/librte_vhost/vhost_cuse/eventfd_copy.h +6 -0

index 19ae30d..5f446ca 100644
@@ -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

lib/librte_vhost/vhost_cuse/vhost-net-cdev.c +3 -0

index 1ae7c49..ae7ad8d 100644
@@ -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.

Signature · 1 line

1.9.1

Xie, Huawei

2015-10-20 09:52 UTC #34 lore raw

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

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:

[...]

lib/librte_vhost/vhost_cuse/eventfd_copy.c line 54

Quoted 5 lines

lib/librte_vhost/vhost_cuse/eventfd_copy.c +5 -0
+
+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.

lib/librte_vhost/vhost_cuse/eventfd_copy.c line 70

Quoted 4 lines

lib/librte_vhost/vhost_cuse/eventfd_copy.c +4 -0
+int
+eventfd_free(void)
+{
+	if (eventfd_link > 0)

same as above:

    if (eventfd_link >= 0)

[...]

Pavel Boldin

2015-10-21 12:16 UTC #35 lore raw

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

Xie,

Please find my comments intermixed below.

Quoted 5 lines · Huawei

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

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.

Quoted 3 lines

Again, please run checkpatch.pl against your patch.

Oops. Thanks for pointing out.

Quoted 10 lines · Pavel Boldin

[...]
lib/librte_vhost/vhost_cuse/eventfd_copy.c line 54
Quoted 5 lines
lib/librte_vhost/vhost_cuse/eventfd_copy.c +5 -0
+
+int
+eventfd_init(void)
+{
+     if (eventfd_link > 0)
0 could be valid fd. Change it to:

Got it. Thanks.

Quoted 11 lines

    if (eventfd_link >= 0)
Change elsewhere if i miss it.
lib/librte_vhost/vhost_cuse/eventfd_copy.c line 70
Quoted 4 lines
lib/librte_vhost/vhost_cuse/eventfd_copy.c +4 -0
+int
+eventfd_free(void)
+{
+     if (eventfd_link > 0)
same as above:
    if (eventfd_link >= 0)
[...]

--
Sincerely,
Pavel

Xie, Huawei

2015-10-26 01:45 UTC #36 lore raw

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

Quoted 13 lines · Pavel Boldin

Xie,
Please find my comments intermixed below.
On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <[email protected] <mailto:[email protected]>> 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?

Quoted 3 lines

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.

Quoted 37 lines

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

Pavel Boldin

2015-10-28 18:35 UTC #37 lore raw

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

Huawei, Thomas,

Please find an updated patchset in the appropriate mail thread.

With best regards,
Pavel

Quoted 61 lines · Huawei

Quoted 13 lines · Pavel Boldin
Xie,
Please find my comments intermixed below.
On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <[email protected] <mailto:[email protected]>> 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?
Quoted 3 lines
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.
Quoted 37 lines
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

Pavel Boldin

2015-08-28 18:51 UTC #38 lore raw

PATCH v5 4/4 DO NOT MERGE: Tests for new eventfd_link module

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

test_eventfd_copy/Makefile new file +8 -0

new file mode 100644
index 0000000..99b3ae3
@@ -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/

test_eventfd_copy/rte_log.h new file +2 -0

new file mode 100644
index 0000000..ffc64c9
@@ -0,0 +1,2 @@
+
+#define RTE_LOG(...)

test_eventfd_copy/test_eventfd_copy.c new file +248 -0

248 lines not on this page · raw message

test_eventfd_copy/vhost-net.h new file +0 -0

new file mode 100644
index 0000000..e69de29

Signature · 1 line

1.9.1

Pavel Boldin

2015-09-23 20:25 UTC #39 lore raw

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

Ping.

Quoted 249 lines · Pavel Boldin

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

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 188
+++++++++++++++------------
 1 file changed, 103 insertions(+), 85 deletions(-)

lib/librte_vhost/eventfd_link/eventfd_link.c +103 ~~-86~~

index 62c45c8..5ba1068 100644
@@ -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

Thomas Monjalon

2015-09-29 23:18 UTC #40 lore raw

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

Hi Pavel,

2015-09-23 23:25, Pavel Boldin:

Quoted 1 line

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

Pavel Boldin

2015-09-29 23:29 UTC #41 lore raw

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

Hi Thomas,

On Tue, Sep 29, 2015 at 10:42 PM, Thomas Monjalon <[email protected]

Quoted 9 lines

wrote:
Hi Pavel,
2015-09-23 23:25, Pavel Boldin:
Quoted 1 line
Ping.
Are you requesting a review?

Yes.

Quoted 2 lines

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

Huawei Xie CCed

Quoted 4 lines

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

Xie, Huawei

2015-10-20 09:06 UTC #42 lore raw

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

Quoted 6 lines · Pavel Boldin

* Move ioctl `EVENTFD_COPY' code to a separate function
* Remove extra #includes
* Introduce function fget_from_files
* Fix ioctl return values
Signed-off-by Pavel Boldin <[email protected]>

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.

Pavel Boldin

2015-10-28 18:33 UTC #43 lore raw

v6 cover vhost: eventfd_link refactoring

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

Signature · 1 line

1.9.1

Pavel Boldin

2015-10-28 18:34 UTC #44 lore raw

PATCH v6 1/3 vhost: eventfd_link: refactoring EVENTFD_COPY handler

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

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

lib/librte_vhost/eventfd_link/eventfd_link.c +100 ~~-81~~

index 62c45c8..7cbebd4 100644
@@ -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 = {

Signature · 1 line

1.9.1

Pavel Boldin

2015-10-28 18:34 UTC #45 lore raw

PATCH v6 2/3 vhost: add EVENTFD_COPY2 ioctl

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

lib/librte_vhost/eventfd_link/eventfd_link.c +61 -0

index 7cbebd4..c54a938 100644
@@ -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;

lib/librte_vhost/eventfd_link/eventfd_link.h +23 -5

index ea619ec..5ebc20b 100644
@@ -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_ */

Signature · 1 line

1.9.1

Pavel Boldin

2015-10-28 18:34 UTC #46 lore raw

PATCH v6 3/3 vhost: using EVENTFD_COPY2

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

lib/librte_vhost/vhost_cuse/eventfd_copy.c +35 ~~-19~~

index 4d697a2..154b32a 100644
@@ -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;
 }

lib/librte_vhost/vhost_cuse/eventfd_copy.h +6 -0

index 19ae30d..5f446ca 100644
@@ -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

lib/librte_vhost/vhost_cuse/vhost-net-cdev.c +3 -0

index 1ae7c49..ae7ad8d 100644
@@ -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.

Signature · 1 line

1.9.1

Xie, Huawei

Acked-by on the v6 series

2015-10-29 18:34 UTC #47 lore raw

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

Quoted 7 lines · Pavel Boldin

The patchset contains an attempt at the refactoring the eventfd_link kernel module that is used to steal an FD in DPDK.
The first patch refactors old EVENTFD_COPY handler fixing the codepath and errors returned from the kernel space. This patch is retained for the backward compatibility.

Quoted 5 lines

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 <[email protected]>

Quoted 12 lines

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

Thomas Monjalon

Applied

2015-10-30 19:11 UTC #48 lore raw

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

2015-10-29 18:33, Xie, Huawei:

Quoted 12 lines · Pavel Boldin

Quoted 11 lines
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.

Quoted 6 lines

Quoted 1 line
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 <[email protected]>

Applied, thanks

Pavel Boldin

author

2015-04-16 11:48 UTC #49 lore raw

PATCH v5 2/5 vhost: eventfd_link: add function fget_from_files

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

Signed-off-by Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 36 +++++++++++++++-------------
 1 file changed, 20 insertions(+), 16 deletions(-)

lib/librte_vhost/eventfd_link/eventfd_link.c +20 ~~-16~~

index d7cb81f..2a29999 100644
@@ -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) {

Signature · 1 line

1.9.1

Pavel Boldin

author

2015-04-16 11:48 UTC #50 lore raw

PATCH v5 3/5 vhost: eventfd_link: fix ioctl return values

Signed-off-by Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 46 ++++++++++++++++++----------
 1 file changed, 30 insertions(+), 16 deletions(-)

lib/librte_vhost/eventfd_link/eventfd_link.c +30 ~~-16~~

index 2a29999..0a06594 100644
@@ -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

Signature · 1 line

1.9.1

Pavel Boldin

author

2015-04-16 11:48 UTC #51 lore raw

PATCH v5 4/5 vhost: eventfd_link: replace copy-pasted sys_close

In reply to Thomas Monjalon

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

Signed-off-by Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>

---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++---------------------
 1 file changed, 12 insertions(+), 37 deletions(-)

lib/librte_vhost/eventfd_link/eventfd_link.c +12 ~~-37~~

index 0a06594..9bc52a3 100644
@@ -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:

Signature · 1 line

1.9.1

Xie, Huawei

2015-05-07 06:54 UTC #52 lore raw

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

Quoted 13 lines · Pavel Boldin

Replace copy-pasted fget_from_files' -> filp_close' with a `sys_close' call.
Signed-off-by Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>
---
 lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++---------------------
 1 file changed, 12 insertions(+), 37 deletions(-)
lib/librte_vhost/eventfd_link/eventfd_link.c +0 -0
index 0a06594..9bc52a3 100644

lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 91

Quoted 4 lines

lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0

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

Quoted 50 lines

lib/librte_vhost/eventfd_link/eventfd_link.c +8 ~~-20~~

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

Thomas Monjalon

2015-06-17 15:25 UTC #53 lore raw

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

2015-05-07 06:54, Xie, Huawei:

Quoted 8 lines · Pavel Boldin

lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+	/* 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?

Thomas Monjalon

2015-07-09 01:00 UTC #54 lore raw

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

Ping

2015-06-17 17:24, Thomas Monjalon:

Quoted 12 lines

2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+	/* 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?

Xie, Huawei

2015-07-10 14:27 UTC #55 lore raw

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

Quoted 11 lines · Thomas Monjalon

2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+	/* 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.

Quoted 2 lines

Pavel Boldin

2015-07-10 14:50 UTC #56 lore raw

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

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

Quoted 20 lines · Huawei

Quoted 11 lines · Thomas Monjalon
2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+   /* 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.
Quoted 2 lines

Xie, Huawei

2015-07-10 15:32 UTC #57 lore raw

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

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 <[email protected]<mailto:[email protected]>> wrote:

Quoted 11 lines · Thomas Monjalon

2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+   /* 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.

Quoted 2 lines

Xie, Huawei

2015-07-10 15:42 UTC #58 lore raw

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

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

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.

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

Pavel

On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <[email protected]<mailto:[email protected]>> wrote:

Quoted 11 lines · Thomas Monjalon

2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+   /* 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.

Quoted 2 lines

Thomas Monjalon

2015-07-10 15:50 UTC #59 lore raw

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

2015-07-10 15:42, Xie, Huawei:

Quoted 1 line

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.

Quoted 47 lines · Pavel Boldin

Xie,
Regarding the patches:
1. The replaced code in fourth patch is checked to be a copy-paste of the sys_close syscall.
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 <[email protected]<mailto:[email protected]>> wrote:
Quoted 11 lines · Thomas Monjalon
2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+   /* 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.
Quoted 2 lines

Xie, Huawei

2015-07-10 16:06 UTC #60 lore raw

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

Quoted 4 lines · Thomas Monjalon

2015-07-10 15:42, Xie, Huawei:
Quoted 1 line
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".

Quoted 49 lines

Quoted 46 lines · Pavel Boldin
Xie,
Regarding the patches:
1. The replaced code in fourth patch is checked to be a copy-paste of the sys_close syscall.
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 <[email protected]<mailto:[email protected]>> wrote:
Quoted 11 lines · Thomas Monjalon
2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
Review
Quoted 2 lines
(quoted diff) +2 -0
+   /* 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.
Quoted 1 line

Pavel Boldin

2015-07-11 15:08 UTC #61 lore raw

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

In reply to Xie, Huawei

Xie, All,

Please find my comments intermixed below.

Quoted 9 lines · Huawei

Don't know why previous mail get messed.
On 7/10/2015 10:50 PM, Pavel Boldin wrote:
Xie,
Regarding the patches:
1. The replaced code in fourth patch is checked to be a copy-paste of the sys_close syscall.

Quoted 6 lines

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.

Quoted 18 lines

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.

Quoted 3 lines

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

Quoted 31 lines

/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 <[email protected]<mailto:
[email protected]>> wrote:
Quoted 11 lines · Thomas Monjalon
2015-05-07 06:54, Xie, Huawei:
Quoted 8 lines · Pavel Boldin
lib/librte_vhost/eventfd_link/eventfd_link.c eventfd_link_ioctl_copy(unsigned long arg) · line 112
Quoted 2 lines
lib/librte_vhost/eventfd_link/eventfd_link.c +2 -0
+   /* 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.
Quoted 2 lines

Xie, Huawei

2015-07-13 02:00 UTC #62 lore raw

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

Quoted 23 lines · Pavel Boldin

Xie, All,
Please find my comments intermixed below.
On Fri, Jul 10, 2015 at 6:42 PM, Xie, Huawei <[email protected] <mailto:[email protected]>> 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.

Quoted 4 lines

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.

Quoted 18 lines

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.

Quoted 9 lines

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?

Quoted 39 lines

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 <[email protected]
    <mailto:[email protected]><mailto:[email protected]
    <mailto:[email protected]>>> 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.

    >
    >

Pavel Boldin

2015-07-19 12:39 UTC #63 lore raw

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

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

Pavel Boldin

author

2015-04-16 11:48 UTC #64 lore raw

PATCH v5 5/5 vhost: eventfd_link: removing extra #includes

Signed-off-by Pavel Boldin <pboldin-nYU0QVwCCFFWk0Htik3J/[email protected]>

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

lib/librte_vhost/eventfd_link/eventfd_link.c +1 -8

index 9bc52a3..0ee7357 100644
@@ -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"

Signature · 1 line

1.9.1

Thomas Monjalon

2015-04-28 14:35 UTC #65 lore raw

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