Properly close files when destroying procs
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 1 Nov 2013 20:54:54 +0000 (13:54 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 16 Jan 2014 19:25:46 +0000 (11:25 -0800)
I'm not 100% happy with this, but we're locking anyways, so we might as
well use it to prevent concurrent additions/modifications when we're
destroying the process.

This prevents us from sharing fgrps, which I'm okay with.

There are lots of races involved, check the comments for details.

I left in the notes about where to deal with plan9/inferno chans.

kern/include/vfs.h
kern/src/process.c
kern/src/vfs.c

index 06bbfba..5d2e378 100644 (file)
@@ -380,6 +380,7 @@ struct file_desc {
 /* All open files for a process */
 struct files_struct {
        spinlock_t                                      lock;
+       bool                                            closed;
        int                                                     max_files;              /* max files ptd to by fd */
        int                                                     max_fdset;              /* max of the current fd_set */
        int                                                     next_fd;                /* next number available */
index bd43d75..eee96e7 100644 (file)
@@ -360,6 +360,7 @@ static void __proc_free(struct kref *kref)
        // All parts of the kernel should have decref'd before __proc_free is called
        assert(kref_refcnt(&p->p_kref) == 0);
 
+       /* close plan9 dot and slash and free fgrp fd and fgrp */
        kref_put(&p->fs_env.root->d_kref);
        kref_put(&p->fs_env.pwd->d_kref);
        destroy_vmrs(p);
@@ -758,12 +759,13 @@ void proc_destroy(struct proc *p)
        spin_unlock(&p->proc_lock);
        /* Wake any of our kthreads waiting on children, so they can abort */
        cv_broadcast(&p->child_wait);
-       /* This prevents processes from accessing their old files while dying, and
-        * will help if these files (or similar objects in the future) hold
-        * references to p (preventing a __proc_free()).  Need to unlock before
-        * doing this - the proclock doesn't protect the files (not proc state), and
-        * closing these might block (can't block while spinning). */
-       /* TODO: might need some sync protection */
+       /* we need to close files here, and not in free, since we could have a
+        * refcnt indirectly related to one of our files.  specifically, if we have
+        * a parent sleeping on our pipe, that parent won't wake up to decref until
+        * the pipe closes.  And if the parent doesnt decref, we don't free.
+        * alternatively, we could send a SIGCHILD to the parent, but that would
+        * require parent's to never ignore that signal (or risk never reaping) */
+       //close_9ns_files(p);
        close_all_files(&p->open_files, FALSE);
        /* Tell the ksched about our death, and which cores we freed up */
        __sched_proc_destroy(p, pc_arr, nr_cores_revoked);
index 7b4c298..b08655f 100644 (file)
@@ -2018,6 +2018,10 @@ struct file *get_file_from_fd(struct files_struct *open_files, int file_desc)
        if (file_desc < 0)
                return 0;
        spin_lock(&open_files->lock);
+       if (open_files->closed) {
+               spin_unlock(&open_files->lock);
+               return 0;
+       }
        if (file_desc < open_files->max_fdset) {
                if (GET_BITMASK_BIT(open_files->open_fds->fds_bits, file_desc)) {
                        /* while max_files and max_fdset might not line up, we should never
@@ -2041,6 +2045,10 @@ struct file *put_file_from_fd(struct files_struct *open_files, int file_desc)
        if (file_desc < 0)
                return 0;
        spin_lock(&open_files->lock);
+       if (open_files->closed) {
+               spin_unlock(&open_files->lock);
+               return 0;
+       }
        if (file_desc < open_files->max_fdset) {
                if (GET_BITMASK_BIT(open_files->open_fds->fds_bits, file_desc)) {
                        /* while max_files and max_fdset might not line up, we should never
@@ -2064,6 +2072,10 @@ int insert_file(struct files_struct *open_files, struct file *file, int low_fd)
        if ((low_fd < 0) || (low_fd > NR_FILE_DESC_MAX))
                return -EINVAL;
        spin_lock(&open_files->lock);
+       if (open_files->closed) {
+               spin_unlock(&open_files->lock);
+               return -EINVAL; /* won't matter, they are dying */
+       }
        for (int i = low_fd; i < open_files->max_fdset; i++) {
                if (GET_BITMASK_BIT(open_files->open_fds->fds_bits, i))
                        continue;
@@ -2090,6 +2102,12 @@ void close_all_files(struct files_struct *open_files, bool cloexec)
 {
        struct file *file;
        spin_lock(&open_files->lock);
+       if (open_files->closed) {
+               spin_unlock(&open_files->lock);
+               return;
+       }
+       if (!cloexec)
+               open_files->closed = TRUE;
        for (int i = 0; i < open_files->max_fdset; i++) {
                if (GET_BITMASK_BIT(open_files->open_fds->fds_bits, i)) {
                        /* while max_files and max_fdset might not line up, we should never
@@ -2113,7 +2131,17 @@ void clone_files(struct files_struct *src, struct files_struct *dst)
 {
        struct file *file;
        spin_lock(&src->lock);
+       if (src->closed) {
+               spin_unlock(&src->lock);
+               return;
+       }
        spin_lock(&dst->lock);
+       if (dst->closed) {
+               warn("Destination closed before it opened");
+               spin_unlock(&dst->lock);
+               spin_unlock(&src->lock);
+               return;
+       }
        for (int i = 0; i < src->max_fdset; i++) {
                if (GET_BITMASK_BIT(src->open_fds->fds_bits, i)) {
                        /* while max_files and max_fdset might not line up, we should never