vfs: Fix race in fdrop

There is a race window between f_count reaches 0 and we set f_count to
INT_MIN. This race would result in multiple call to fp->close() and
rcu_dispose(fp) if we call them in fdrop().

To fix, we move fp->stop_polls and fp->close() from fdrop() into in
fdset() and fdclose(). This will eliminate the multipe call to
fp->close(), as there can be only one call to fdclose() or fdset().

Also, we set f_count to INT_MIN using CAS, this way, we make sure
rcu_dispose(fp) won't be called twice.

Fixes cloudius-systems#293.

Signed-off-by: Asias He <[email protected]>

core/mmu.cc

@@ -1681,12 +1681,16 @@ int shm_file::stat(struct stat* buf)
 }
 
 int shm_file::close()
+{
+    return 0;
+}
+
+shm_file::~shm_file()
 {
     for (auto& i : _pages) {
         memory::free_huge_page(i.second, huge_page_size);
     }
     _pages.clear();
-    return 0;
 }
 
 void linear_map(void* _virt, phys addr, size_t size, size_t slop)

fs/vfs/kern_descrip.cc

@@ -90,6 +90,8 @@ int fdclose(int fd)
         gfdt[fd].assign(nullptr);
     }
 
+    fp->stop_polls();
+    fp->close();
     fdrop(fp);
 
     return 0;
@@ -114,8 +116,11 @@ int fdset(int fd, struct file *fp)
         gfdt[fd].assign(fp);
     }
 
-    if (orig)
+    if (orig) {
+        orig->stop_polls();
+        orig->close();
         fdrop(orig);
+    }
 
     return 0;
 }
@@ -185,18 +190,25 @@ void fhold(struct file* fp)
 
 int fdrop(struct file *fp)
 {
-    if (__sync_fetch_and_sub(&fp->f_count, 1) != 1)
-        return 0;
+    bool do_free = false;
+    int o = fp->f_count, n;
+    do {
+        n = o - 1;
+        if (n == 0) {
+            /* We are about to free this file structure, but we still do things with it
+             * so set the refcount to INT_MIN, fhold/fdrop may get called again
+             * and we don't want to reach this point more than once.
+             * INT_MIN is also safe against fget() seeing this file.
+             */
+            n = INT_MIN;
+            do_free = true;
+        }
+    } while (!__atomic_compare_exchange_n(&fp->f_count, &o, n, true,
+                __ATOMIC_RELAXED, __ATOMIC_RELAXED));
 
-    /* We are about to free this file structure, but we still do things with it
-     * so set the refcount to INT_MIN, fhold/fdrop may get called again
-     * and we don't want to reach this point more than once.
-     * INT_MIN is also safe against fget() seeing this file.
-     */
+    if (!do_free)
+        return 0;
 
-    fp->f_count = INT_MIN;
-    fp->stop_polls();
-    fp->close();
     rcu_dispose(fp);
     return 1;
 }

fs/vfs/vfs_fops.cc

@@ -33,7 +33,6 @@ int vfs_file::close()
 	if (error)
 		return error;
 
-	fp->f_dentry.reset();
 	return 0;
 }
 

include/osv/mmu.hh

@@ -147,6 +147,7 @@ class shm_file final : public special_file {
     size_t _size;
     std::unordered_map<uintptr_t, void*> _pages;
     void* page(uintptr_t hp_off);
+    ~shm_file();
 public:
     shm_file(size_t size, int flags);
     virtual int stat(struct stat* buf) override;

libc/pipe.cc

@@ -36,8 +36,8 @@ class pipe_file final : public special_file {
     virtual int poll(int events) override;
     virtual int close() override;
 private:
-    pipe_writer* writer = nullptr;
-    pipe_reader* reader = nullptr;
+    std::unique_ptr<pipe_writer> writer;
+    std::unique_ptr<pipe_reader> reader;
 };
 
 pipe_file::pipe_file(std::unique_ptr<pipe_writer>&& s)
@@ -82,13 +82,6 @@ int pipe_file::poll(int events)
 
 int pipe_file::close()
 {
-    if (f_flags & FWRITE) {
-        delete writer;
-        writer = nullptr;
-    } else {
-        delete reader;
-        reader = nullptr;
-    }
     return 0;
 }