From 1e5bfa623aa93b918566e8dc0e2f9c1a1037f45e Mon Sep 17 00:00:00 2001
From: Xiang Xiao <xiaoxiang@xiaomi.com>
Date: Sun, 3 Jan 2021 00:53:46 +0800
Subject: [PATCH] fs: file_dup2 shouldn't hold the file list lock

the argument passed to file_dup2 doesn't always come from task file list
so it doesn't make sense to hold the file list lock and then it is better
to do the protection in the new function files_dupfd2

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Change-Id: Ibf02cea9b0b275e7472f9c04fd66b9242285b957
---
 fs/inode/fs_files.c | 93 +++++++++++++++++++++++++--------------------
 fs/inode/inode.h    | 14 +++++++
 fs/vfs/fs_dupfd2.c  | 29 +-------------
 3 files changed, 66 insertions(+), 70 deletions(-)

diff --git a/fs/inode/fs_files.c b/fs/inode/fs_files.c
index 31117cba39..52980187c7 100644
--- a/fs/inode/fs_files.c
+++ b/fs/inode/fs_files.c
@@ -135,7 +135,6 @@ void files_releaselist(FAR struct filelist *list)
 
 int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
 {
-  FAR struct filelist *list;
   FAR struct inode *inode;
   struct file temp;
   int ret;
@@ -150,33 +149,13 @@ int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
       return OK;
     }
 
-  list = nxsched_get_files();
-
-  /* The file list can be NULL under two cases:  (1) One is an obscure
-   * cornercase:  When memory management debug output is enabled.  Then
-   * there may be attempts to write to stdout from malloc before the group
-   * data has been allocated.  The other other is (2) if this is a kernel
-   * thread.  Kernel threads have no allocated file descriptors.
-   */
-
-  if (list != NULL)
-    {
-      ret = _files_semtake(list);
-      if (ret < 0)
-        {
-          /* Probably canceled */
-
-          return ret;
-        }
-    }
-
   /* Increment the reference count on the contained inode */
 
   inode = filep1->f_inode;
   ret   = inode_addref(inode);
   if (ret < 0)
     {
-      goto errout_with_sem;
+      return ret;
     }
 
   /* Then clone the file structure */
@@ -211,7 +190,8 @@ int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
 
       if (ret < 0)
         {
-          goto errout_with_inode;
+          inode_release(inode);
+          return ret;
         }
     }
 
@@ -225,26 +205,7 @@ int file_dup2(FAR struct file *filep1, FAR struct file *filep2)
   /* Return the file structure */
 
   memcpy(filep2, &temp, sizeof(temp));
-
-  if (list != NULL)
-    {
-      _files_semgive(list);
-    }
-
   return OK;
-
-  /* Handle various error conditions */
-
-errout_with_inode:
-  inode_release(inode);
-
-errout_with_sem:
-  if (list != NULL)
-    {
-      _files_semgive(list);
-    }
-
-  return ret;
 }
 
 /****************************************************************************
@@ -293,6 +254,54 @@ int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
   return -EMFILE;
 }
 
+/****************************************************************************
+ * Name: files_dupfd2
+ *
+ * Description:
+ *   Clone a file descriptor to a specific descriptor number.
+ *
+ * Returned Value:
+ *   fd2 is returned on success; a negated errno value is return on
+ *   any failure.
+ *
+ ****************************************************************************/
+
+int files_dupfd2(int fd1, int fd2)
+{
+  FAR struct filelist *list;
+  int ret;
+
+  if (fd1 < 0 || fd1 >= CONFIG_NFILE_DESCRIPTORS ||
+      fd2 < 0 || fd2 >= CONFIG_NFILE_DESCRIPTORS)
+    {
+      return -EBADF;
+    }
+
+  /* Get the file descriptor list.  It should not be NULL in this context. */
+
+  list = nxsched_get_files();
+  DEBUGASSERT(list != NULL);
+
+  ret = _files_semtake(list);
+  if (ret < 0)
+    {
+      /* Probably canceled */
+
+      return ret;
+    }
+
+  /* Perform the dup2 operation */
+
+  ret = file_dup2(&list->fl_files[fd1], &list->fl_files[fd2]);
+  _files_semgive(list);
+  if (ret < 0)
+    {
+      return ret;
+    }
+
+  return fd2;
+}
+
 /****************************************************************************
  * Name: files_close
  *
diff --git a/fs/inode/inode.h b/fs/inode/inode.h
index dea5753491..f2c696e626 100644
--- a/fs/inode/inode.h
+++ b/fs/inode/inode.h
@@ -389,6 +389,20 @@ void weak_function files_initialize(void);
 int files_allocate(FAR struct inode *inode, int oflags, off_t pos,
                    FAR void *priv, int minfd);
 
+/****************************************************************************
+ * Name: files_dupfd2
+ *
+ * Description:
+ *   Clone a file descriptor to a specific descriptor number.
+ *
+ * Returned Value:
+ *   fd2 is returned on success; a negated errno value is return on
+ *   any failure.
+ *
+ ****************************************************************************/
+
+int files_dupfd2(int fd1, int fd2);
+
 /****************************************************************************
  * Name: files_close
  *
diff --git a/fs/vfs/fs_dupfd2.c b/fs/vfs/fs_dupfd2.c
index d74ccb44e2..68fcc544e7 100644
--- a/fs/vfs/fs_dupfd2.c
+++ b/fs/vfs/fs_dupfd2.c
@@ -64,32 +64,5 @@
 
 int fs_dupfd2(int fd1, int fd2)
 {
-  FAR struct file *filep1;
-  FAR struct file *filep2 = NULL;
-  int ret;
-
-  /* Get the file structures corresponding to the file descriptors. */
-
-  ret = fs_getfilep(fd1, &filep1);
-  if (ret >= 0)
-    {
-      ret = fs_getfilep(fd2, &filep2);
-    }
-
-  if (ret < 0)
-    {
-      return ret;
-    }
-
-  DEBUGASSERT(filep1 != NULL && filep2 != NULL);
-
-  /* Perform the dup2 operation */
-
-  ret = file_dup2(filep1, filep2);
-  if (ret < 0)
-    {
-      return ret;
-    }
-
-  return fd2;
+  return files_dupfd2(fd1, fd2);
 }