Fixes O_CLOEXEC
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 19 Jan 2015 23:38:01 +0000 (18:38 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 19 Jan 2015 23:38:01 +0000 (18:38 -0500)
O_CLOEXEC, the open flag from glibc (sometimes called the mode in 9ns)
was being saved as a struct file f_flag, instead of on the file
descriptor.  Likewise, fcntl() was doing the same.

To deal with the races that O_CLOEXEC was made for (and which make the
fcntl SETFD option arguably useless) we need to set FD_CLOEXEC
atomically when the FD is inserted into the FD table.  This required
work with both VFS and 9ns.

I'm not sure how inferno/9ns actually handled CLOEXEC, since newfd()
doesn't take any flags.

kern/include/ns.h
kern/include/vfs.h
kern/src/ns/sysfile.c
kern/src/process.c
kern/src/syscall.c
kern/src/vfs.c

index 2c57b98..797d895 100644 (file)
@@ -977,7 +977,7 @@ void cmderror(struct cmdbuf *cb, char *s);
 struct cmdtab *lookupcmd(struct cmdbuf *cb, struct cmdtab *ctab, int nctab);
 
 /* kern/src/ns/sysfile.c */
-int newfd(struct chan *c);
+int newfd(struct chan *c, bool oflags);
 struct chan *fdtochan(struct fgrp *f, int fd, int mode, int chkmnt, int iref);
 long kchanio(void *vc, void *buf, int n, int mode);
 int openmode(uint32_t o);
index 8815f98..fcfbef6 100644 (file)
@@ -486,7 +486,7 @@ void file_release(struct kref *kref);
 struct file *get_file_from_fd(struct files_struct *open_files, int fd);
 struct file *put_file_from_fd(struct files_struct *open_files, int file_desc);
 int insert_file(struct files_struct *open_files, struct file *file, int low_fd,
-                bool must);
+                bool must, bool cloexec);
 void close_all_files(struct files_struct *open_files, bool cloexec);
 void clone_files(struct files_struct *src, struct files_struct *dst);
 int do_chdir(struct fs_struct *fs_env, char *path);
@@ -500,7 +500,7 @@ extern struct inode_operations dummy_i_op;
 extern struct dentry_operations dummy_d_op;
 
 int put_fd(struct files_struct *open_files, int file_desc);
-int get_fd(struct files_struct *open_files, int low_fd);
+int get_fd(struct files_struct *open_files, int low_fd, int cloexec);
 int claim_fd(struct files_struct *open_files, int file_desc);
 
 #endif /* ROS_KERN_VFS_H */
index 74aa619..17e7d8c 100644 (file)
@@ -53,7 +53,7 @@ static int growfd(struct fgrp *f, int fd)
        return 0;
 }
 
-int newfd(struct chan *c)
+int newfd(struct chan *c, bool oflags)
 {
        int i;
        struct fgrp *f = current->fgrp;
@@ -68,7 +68,7 @@ int newfd(struct chan *c)
         * know if we closed anything.  Since we share the FD numbers with the VFS,
         * there is no way to know that. */
 #if 1  // VFS hack
-       i = get_fd(&current->open_files, 0);
+       i = get_fd(&current->open_files, 0, oflags & O_CLOEXEC);
 #else  // 9ns style
        /* TODO: use a unique integer allocator */
        for (i = f->minfd; i < f->nfd; i++)
@@ -278,7 +278,7 @@ int syscreate(char *path, int mode, uint32_t perm)
                cclose(c);
                nexterror();
        }
-       fd = newfd(c);
+       fd = newfd(c, mode);    /* 9ns mode is the O_FLAGS and perm is glibc mode */
        if (fd < 0)
                error(Enofd);
        poperror();
@@ -339,7 +339,7 @@ int sysdup(int old, int new)
                        cclose(c);
                        nexterror();
                }
-               fd = newfd(c);
+               fd = newfd(c, 0);
                if (fd < 0)
                        error(Enofd);
                poperror();
@@ -449,7 +449,7 @@ int sysfauth(int fd, char *aname)
                nexterror();
        }
 
-       fd = newfd(ac);
+       fd = newfd(ac, 0);
        if (fd < 0)
                error(Enofd);
        poperror();     /* ac */
@@ -529,10 +529,10 @@ int syspipe(int fd[2])
                error(Egreg);
        c[0] = d->open(c[0], ORDWR);
        c[1] = d->open(c[1], ORDWR);
-       fd[0] = newfd(c[0]);
+       fd[0] = newfd(c[0], 0);
        if (fd[0] < 0)
                error(Enofd);
-       fd[1] = newfd(c[1]);
+       fd[1] = newfd(c[1], 0);
        if (fd[1] < 0)
                error(Enofd);
        poperror();
@@ -705,7 +705,7 @@ int sysopen(char *path, int vfs_flags)
                cclose(c);
                nexterror();
        }
-       fd = newfd(c);
+       fd = newfd(c, vfs_flags);
        if (fd < 0)
                error(Enofd);
        poperror();
index 68a42e6..6094254 100644 (file)
@@ -388,9 +388,13 @@ error_t proc_alloc(struct proc **pp, struct proc *parent, int flags)
                        clone_files(&parent->open_files, &p->open_files);
        } else {
                /* no parent, we're created from the kernel */
-               assert(insert_file(&p->open_files, dev_stdin,  0, TRUE) == 0);
-               assert(insert_file(&p->open_files, dev_stdout, 1, TRUE) == 1);
-               assert(insert_file(&p->open_files, dev_stderr, 2, TRUE) == 2);
+               int fd;
+               fd = insert_file(&p->open_files, dev_stdin,  0, TRUE, FALSE);
+               assert(fd == 0);
+               fd = insert_file(&p->open_files, dev_stdout, 1, TRUE, FALSE);
+               assert(fd == 1);
+               fd = insert_file(&p->open_files, dev_stderr, 2, TRUE, FALSE);
+               assert(fd == 2);
        }
        /* Init the ucq hash lock */
        p->ucq_hashlock = (struct hashlock*)&p->ucq_hl_noref;
index 7eaf509..2be0963 100644 (file)
@@ -1303,7 +1303,7 @@ static intreg_t sys_open(struct proc *p, const char *path, size_t path_l,
        /* VFS */
        if (file) {
                /* stores the ref to file */
-               fd = insert_file(&p->open_files, file, 0, FALSE);
+               fd = insert_file(&p->open_files, file, 0, FALSE, oflag & O_CLOEXEC);
                kref_put(&file->f_kref);        /* drop our ref */
                if (fd < 0)
                        warn("File insertion failed");
@@ -1469,9 +1469,10 @@ intreg_t sys_fcntl(struct proc *p, int fd, int cmd, int arg)
                return -1;
        }
 
+       /* TODO: these are racy */
        switch (cmd) {
                case (F_DUPFD):
-                       retval = insert_file(&p->open_files, file, arg, FALSE);
+                       retval = insert_file(&p->open_files, file, arg, FALSE, FALSE);
                        if (retval < 0) {
                                set_errno(-retval);
                                retval = -1;
@@ -1481,8 +1482,13 @@ intreg_t sys_fcntl(struct proc *p, int fd, int cmd, int arg)
                        retval = p->open_files.fd[fd].fd_flags;
                        break;
                case (F_SETFD):
-                       if (arg == FD_CLOEXEC)
-                               file->f_flags |= O_CLOEXEC;
+                       /* I'm considering not supporting this at all.  They must do it at
+                        * open time or fix their buggy/racy code. */
+                       spin_lock(&p->open_files.lock);
+                       if (arg & FD_CLOEXEC)
+                               p->open_files.fd[fd].fd_flags |= FD_CLOEXEC;
+                       retval = p->open_files.fd[fd].fd_flags;
+                       spin_unlock(&p->open_files.lock);
                        break;
                case (F_GETFL):
                        retval = file->f_flags;
@@ -2174,7 +2180,8 @@ static intreg_t sys_dup_fds_to(struct proc *p, unsigned int pid,
                map[i].ok = -1;
                file = get_file_from_fd(&p->open_files, map[i].parentfd);
                if (file) {
-                       slot = insert_file(&child->open_files, file, map[i].childfd, TRUE);
+                       slot = insert_file(&child->open_files, file, map[i].childfd, TRUE,
+                                          FALSE);
                        if (slot == map[i].childfd) {
                                map[i].ok = 0;
                                ret++;
index 1ba6f14..cd330bc 100644 (file)
@@ -2458,12 +2458,15 @@ static int __get_fd(struct files_struct *open_files, int low_fd)
        return slot;
 }
 
-/* Gets and claims a free FD, used by 9ns.  < 0 == error. */
-int get_fd(struct files_struct *open_files, int low_fd)
+/* Gets and claims a free FD, used by 9ns.  < 0 == error.  cloexec is tracked on
+ * the VFS FD.  It's value will be O_CLOEXEC (not 1) or 0. */
+int get_fd(struct files_struct *open_files, int low_fd, int cloexec)
 {
        int slot;
        spin_lock(&open_files->lock);
        slot = __get_fd(open_files, low_fd);
+       if (cloexec && (slot >= 0))
+               open_files->fd[slot].fd_flags |= FD_CLOEXEC;
        spin_unlock(&open_files->lock);
        return slot;
 }
@@ -2495,7 +2498,8 @@ static int __claim_fd(struct files_struct *open_files, int file_desc)
        return 0;
 }
 
-/* Claims a specific FD when duping FDs. used by 9ns.  < 0 == error. */
+/* Claims a specific FD when duping FDs. used by 9ns.  < 0 == error.  No need
+ * for cloexec here, since it's not used during dup. */
 int claim_fd(struct files_struct *open_files, int file_desc)
 {
        int ret;
@@ -2506,9 +2510,12 @@ int claim_fd(struct files_struct *open_files, int file_desc)
 }
 
 /* Inserts the file in the files_struct, returning the corresponding new file
- * descriptor, or an error code.  We start looking for open fds from low_fd. */
+ * descriptor, or an error code.  We start looking for open fds from low_fd.
+ *
+ * Passing cloexec is a bit cheap, since we might want to expand it to support
+ * more FD options in the future. */
 int insert_file(struct files_struct *open_files, struct file *file, int low_fd,
-                bool must)
+                bool must, bool cloexec)
 {
        int slot, ret;
        spin_lock(&open_files->lock);
@@ -2533,12 +2540,14 @@ int insert_file(struct files_struct *open_files, struct file *file, int low_fd,
        kref_get(&file->f_kref, 1);
        open_files->fd[slot].fd_file = file;
        open_files->fd[slot].fd_flags = 0;
+       if (cloexec)
+               open_files->fd[slot].fd_flags |= FD_CLOEXEC;
        spin_unlock(&open_files->lock);
        return slot;
 }
 
 /* Closes all open files.  Mostly just a "put" for all files.  If cloexec, it
- * will only close files that are opened with O_CLOEXEC. */
+ * will only close the FDs with FD_CLOEXEC (opened with O_CLOEXEC or fcntld). */
 void close_all_files(struct files_struct *open_files, bool cloexec)
 {
        struct file *file;
@@ -2556,7 +2565,7 @@ void close_all_files(struct files_struct *open_files, bool cloexec)
                        /* no file == 9ns uses the FD.  they will deal with it */
                        if (!file)
                                continue;
-                       if (cloexec && !(open_files->fd[i].fd_flags & O_CLOEXEC))
+                       if (cloexec && !(open_files->fd[i].fd_flags & FD_CLOEXEC))
                                continue;
                        /* Actually close the file */
                        open_files->fd[i].fd_file = 0;