From 8bca37d604536e5908c5a4ce224ee5afb4ebafc7 Mon Sep 17 00:00:00 2001 From: Adrien Destugues Date: Sun, 14 Jan 2018 11:26:12 +0100 Subject: [PATCH] vfs: Bind flock locks to file descriptors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * File locks created by flock should only apply for the file descriptor that was used to lock the file. Another fd on the same file should then be denied access (calling flock should fail). * fcntl based locks, however, are in a separate namespace and are global to a team. * This issue was found when running webkitpy test suite, and should close ticket #13795. * Don't use session or team as comparison in release_advisory_lock(), as that information might not be available anymore (e.g. when called from Team::~Team()). This fixes #14121. Change-Id: I9efb96cfcefe7e72b0060220c635a665e7e643cc Co-authored-by: Axel Dörfler --- headers/private/kernel/fs/fd.h | 26 ++++++++---- headers/private/kernel/vfs.h | 4 +- src/system/kernel/fs/fd.cpp | 14 ++++--- src/system/kernel/fs/vfs.cpp | 75 +++++++++++++++++++++------------- 4 files changed, 75 insertions(+), 44 deletions(-) diff --git a/headers/private/kernel/fs/fd.h b/headers/private/kernel/fs/fd.h index 0312b5fd5d..f9ee92cc30 100644 --- a/headers/private/kernel/fs/fd.h +++ b/headers/private/kernel/fs/fd.h @@ -1,5 +1,5 @@ /* - * Copyright 2002-2006, Axel Dörfler, axeld@pinc-software.de. + * Copyright 2002-2018, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. */ #ifndef _FD_H @@ -22,10 +22,13 @@ struct selectsync; struct select_info; struct fd_ops { - status_t (*fd_read)(struct file_descriptor *, off_t pos, void *buffer, size_t *length); - status_t (*fd_write)(struct file_descriptor *, off_t pos, const void *buffer, size_t *length); + status_t (*fd_read)(struct file_descriptor *, off_t pos, void *buffer, + size_t *length); + status_t (*fd_write)(struct file_descriptor *, off_t pos, + const void *buffer, size_t *length); off_t (*fd_seek)(struct file_descriptor *, off_t pos, int seekType); - status_t (*fd_ioctl)(struct file_descriptor *, ulong op, void *buffer, size_t length); + status_t (*fd_ioctl)(struct file_descriptor *, ulong op, void *buffer, + size_t length); status_t (*fd_set_flags)(struct file_descriptor *, int flags); status_t (*fd_select)(struct file_descriptor *, uint8 event, struct selectsync *sync); @@ -36,7 +39,8 @@ struct fd_ops { size_t bufferSize, uint32 *_count); status_t (*fd_rewind_dir)(struct file_descriptor *); status_t (*fd_read_stat)(struct file_descriptor *, struct stat *); - status_t (*fd_write_stat)(struct file_descriptor *, const struct stat *, int statMask); + status_t (*fd_write_stat)(struct file_descriptor *, const struct stat *, + int statMask); status_t (*fd_close)(struct file_descriptor *); void (*fd_free)(struct file_descriptor *); }; @@ -76,11 +80,13 @@ enum fd_types { /* Prototypes */ extern struct file_descriptor *alloc_fd(void); -extern int new_fd_etc(struct io_context *, struct file_descriptor *, int firstIndex); +extern int new_fd_etc(struct io_context *, struct file_descriptor *, + int firstIndex); extern int new_fd(struct io_context *, struct file_descriptor *); extern struct file_descriptor *get_fd(struct io_context *, int); extern struct file_descriptor *get_open_fd(struct io_context *, int); -extern void close_fd(struct file_descriptor *descriptor); +extern void close_fd(struct io_context *context, + struct file_descriptor *descriptor); extern status_t close_fd_index(struct io_context *context, int fd); extern void put_fd(struct file_descriptor *descriptor); extern void disconnect_fd(struct file_descriptor *descriptor); @@ -92,11 +98,13 @@ extern bool fd_is_valid(int fd, bool kernel); extern struct vnode *fd_vnode(struct file_descriptor *descriptor); extern bool fd_close_on_exec(struct io_context *context, int fd); -extern void fd_set_close_on_exec(struct io_context *context, int fd, bool closeFD); +extern void fd_set_close_on_exec(struct io_context *context, int fd, + bool closeFD); static struct io_context *get_current_io_context(bool kernel); -extern status_t user_fd_kernel_ioctl(int fd, ulong op, void *buffer, size_t length); +extern status_t user_fd_kernel_ioctl(int fd, ulong op, void *buffer, + size_t length); /* The prototypes of the (sys|user)_ functions are currently defined in vfs.h */ diff --git a/headers/private/kernel/vfs.h b/headers/private/kernel/vfs.h index fc818ba68d..f754c75973 100644 --- a/headers/private/kernel/vfs.h +++ b/headers/private/kernel/vfs.h @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016, Axel Dörfler, axeld@pinc-software.de. + * Copyright 2002-2018, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. * * Copyright 2001-2002, Travis Geiselbrecht. All rights reserved. @@ -124,6 +124,8 @@ status_t vfs_entry_ref_to_path(dev_t device, ino_t inode, const char *leaf, bool kernel, char *path, size_t pathLength); status_t vfs_get_cwd(dev_t *_mountID, ino_t *_vnodeID); void vfs_unlock_vnode_if_locked(struct file_descriptor *descriptor); +status_t vfs_release_posix_lock(io_context* context, + struct file_descriptor* descriptor); status_t vfs_unmount(dev_t mountID, uint32 flags); status_t vfs_disconnect_vnode(dev_t mountID, ino_t vnodeID); status_t vfs_resolve_parent(struct vnode* parent, dev_t* device, diff --git a/src/system/kernel/fs/fd.cpp b/src/system/kernel/fs/fd.cpp index 25b85d9ea3..225a47a6c8 100644 --- a/src/system/kernel/fs/fd.cpp +++ b/src/system/kernel/fs/fd.cpp @@ -1,6 +1,6 @@ /* * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de. - * Copyright 2002-2015, Axel Dörfler, axeld@pinc-software.de. + * Copyright 2002-2018, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. */ @@ -237,8 +237,12 @@ put_fd(struct file_descriptor* descriptor) its close hook when appropriate. */ void -close_fd(struct file_descriptor* descriptor) +close_fd(struct io_context* context, struct file_descriptor* descriptor) { + // POSIX advisory locks need to be released when any file descriptor closes + if (descriptor->type == FDTYPE_FILE) + vfs_release_posix_lock(context, descriptor); + if (atomic_add(&descriptor->open_count, -1) == 1) { vfs_unlock_vnode_if_locked(descriptor); @@ -256,7 +260,7 @@ close_fd_index(struct io_context* context, int fd) if (descriptor == NULL) return B_FILE_ERROR; - close_fd(descriptor); + close_fd(context, descriptor); put_fd(descriptor); // the reference associated with the slot @@ -452,7 +456,7 @@ dup2_fd(int oldfd, int newfd, bool kernel) // Say bye bye to the evicted fd if (evicted) { deselect_select_infos(evicted, selectInfos, true); - close_fd(evicted); + close_fd(context, evicted); put_fd(evicted); } @@ -605,7 +609,7 @@ select_fd(int32 fd, struct select_info* info, bool kernel) deselect_select_infos(descriptor, info, false); // Release our open reference of the descriptor. - close_fd(descriptor); + close_fd(context, descriptor); return B_FILE_ERROR; } diff --git a/src/system/kernel/fs/vfs.cpp b/src/system/kernel/fs/vfs.cpp index 98c51c5ea6..4f1657a55c 100644 --- a/src/system/kernel/fs/vfs.cpp +++ b/src/system/kernel/fs/vfs.cpp @@ -172,6 +172,7 @@ namespace { struct advisory_lock : public DoublyLinkedListLinkImpl { list_link link; + void* bound_to; team_id team; pid_t session; off_t start; @@ -1628,7 +1629,8 @@ test_advisory_lock(struct vnode* vnode, struct flock* flock) if \a flock is NULL. */ static status_t -release_advisory_lock(struct vnode* vnode, struct flock* flock) +release_advisory_lock(struct vnode* vnode, struct io_context* context, + struct file_descriptor* descriptor, struct flock* flock) { FUNCTION(("release_advisory_lock(vnode = %p, flock = %p)\n", vnode, flock)); @@ -1636,10 +1638,6 @@ release_advisory_lock(struct vnode* vnode, struct flock* flock) if (locking == NULL) return B_OK; - // TODO: use the thread ID instead?? - team_id team = team_get_current_team_id(); - pid_t session = thread_get_current_thread()->team->session_id; - // find matching lock entries LockList::Iterator iterator = locking->locks.GetIterator(); @@ -1647,9 +1645,12 @@ release_advisory_lock(struct vnode* vnode, struct flock* flock) struct advisory_lock* lock = iterator.Next(); bool removeLock = false; - if (lock->session == session) + if (descriptor != NULL && lock->bound_to == descriptor) { + // Remove flock() locks removeLock = true; - else if (lock->team == team && advisory_lock_intersects(lock, flock)) { + } else if (lock->bound_to == context + && advisory_lock_intersects(lock, flock)) { + // Remove POSIX locks bool endsBeyond = false; bool startsBefore = false; if (flock != NULL) { @@ -1678,6 +1679,7 @@ release_advisory_lock(struct vnode* vnode, struct flock* flock) lock->end = flock->l_start - 1; + secondLock->bound_to = context; secondLock->team = lock->team; secondLock->session = lock->session; // values must already be normalized when getting here @@ -1735,19 +1737,22 @@ release_advisory_lock(struct vnode* vnode, struct flock* flock) will wait for the lock to become available, if there are any collisions (it will return B_PERMISSION_DENIED in this case if \a wait is \c false). - If \a session is -1, POSIX semantics are used for this lock. Otherwise, + If \a descriptor is NULL, POSIX semantics are used for this lock. Otherwise, BSD flock() semantics are used, that is, all children can unlock the file in question (we even allow parents to remove the lock, though, but that seems to be in line to what the BSD's are doing). */ static status_t -acquire_advisory_lock(struct vnode* vnode, pid_t session, struct flock* flock, - bool wait) +acquire_advisory_lock(struct vnode* vnode, io_context* context, + struct file_descriptor* descriptor, struct flock* flock, bool wait) { FUNCTION(("acquire_advisory_lock(vnode = %p, flock = %p, wait = %s)\n", vnode, flock, wait ? "yes" : "no")); + dprintf("acquire_advisory_lock(vnode = %p, flock = %p, wait = %s)\n", + vnode, flock, wait ? "yes" : "no"); bool shared = flock->l_type == F_RDLCK; + void* boundTo = descriptor != NULL ? (void*)descriptor : (void*)context; status_t status = B_OK; // TODO: do deadlock detection! @@ -1771,7 +1776,8 @@ acquire_advisory_lock(struct vnode* vnode, pid_t session, struct flock* flock, struct advisory_lock* lock = iterator.Next(); // TODO: locks from the same team might be joinable! - if (lock->team != team && advisory_lock_intersects(lock, flock)) { + if ((lock->team != team || lock->bound_to != boundTo) + && advisory_lock_intersects(lock, flock)) { // locks do overlap if (!shared || !lock->shared) { // we need to wait @@ -1788,7 +1794,7 @@ acquire_advisory_lock(struct vnode* vnode, pid_t session, struct flock* flock, if (!wait) { put_advisory_locking(locking); - return session != -1 ? B_WOULD_BLOCK : B_PERMISSION_DENIED; + return descriptor != NULL ? B_WOULD_BLOCK : B_PERMISSION_DENIED; } status = switch_sem_etc(locking->lock, waitForLock, 1, @@ -1809,8 +1815,9 @@ acquire_advisory_lock(struct vnode* vnode, pid_t session, struct flock* flock, return B_NO_MEMORY; } + lock->bound_to = boundTo; lock->team = team_get_current_team_id(); - lock->session = session; + lock->session = thread_get_current_thread()->team->session_id; // values must already be normalized when getting here lock->start = flock->l_start; lock->end = flock->l_start - 1 + flock->l_len; @@ -3645,7 +3652,7 @@ free_io_context(io_context* context) for (i = 0; i < context->table_size; i++) { if (struct file_descriptor* descriptor = context->fds[i]) { - close_fd(descriptor); + close_fd(context, descriptor); put_fd(descriptor); } } @@ -4877,6 +4884,19 @@ vfs_unlock_vnode_if_locked(struct file_descriptor* descriptor) } +/*! Releases any POSIX locks on the file descriptor. */ +status_t +vfs_release_posix_lock(io_context* context, struct file_descriptor* descriptor) +{ + struct vnode* vnode = descriptor->u.vnode; + + if (HAS_FS_CALL(vnode, release_lock)) + return FS_CALL(vnode, release_lock, descriptor->cookie, NULL); + + return release_advisory_lock(vnode, context, NULL, NULL); +} + + /*! Closes all file descriptors of the specified I/O context that have the O_CLOEXEC flag set. */ @@ -4901,7 +4921,7 @@ vfs_exec_io_context(io_context* context) mutex_unlock(&context->io_mutex); if (remove) { - close_fd(descriptor); + close_fd(context, descriptor); put_fd(descriptor); } } @@ -5634,7 +5654,7 @@ file_close(struct file_descriptor* descriptor) if (HAS_FS_CALL(vnode, release_lock)) status = FS_CALL(vnode, release_lock, descriptor->cookie, NULL); else - status = release_advisory_lock(vnode, NULL); + status = release_advisory_lock(vnode, NULL, descriptor, NULL); } return status; } @@ -6083,8 +6103,9 @@ common_fcntl(int fd, int op, size_t argument, bool kernel) FUNCTION(("common_fcntl(fd = %d, op = %d, argument = %lx, %s)\n", fd, op, argument, kernel ? "kernel" : "user")); - struct file_descriptor* descriptor = get_fd(get_current_io_context(kernel), - fd); + struct io_context* context = get_current_io_context(kernel); + + struct file_descriptor* descriptor = get_fd(context, fd); if (descriptor == NULL) return B_FILE_ERROR; @@ -6108,7 +6129,6 @@ common_fcntl(int fd, int op, size_t argument, bool kernel) switch (op) { case F_SETFD: { - struct io_context* context = get_current_io_context(kernel); // Set file descriptor flags // O_CLOEXEC is the only flag available at this time @@ -6122,8 +6142,6 @@ common_fcntl(int fd, int op, size_t argument, bool kernel) case F_GETFD: { - struct io_context* context = get_current_io_context(kernel); - // Get file descriptor flags mutex_lock(&context->io_mutex); status = fd_close_on_exec(context, fd) ? FD_CLOEXEC : 0; @@ -6160,8 +6178,6 @@ common_fcntl(int fd, int op, size_t argument, bool kernel) case F_DUPFD: case F_DUPFD_CLOEXEC: { - struct io_context* context = get_current_io_context(kernel); - status = new_fd_etc(context, descriptor, (int)argument); if (status >= 0) { mutex_lock(&context->io_mutex); @@ -6220,8 +6236,10 @@ common_fcntl(int fd, int op, size_t argument, bool kernel) if (HAS_FS_CALL(vnode, release_lock)) { status = FS_CALL(vnode, release_lock, descriptor->cookie, &flock); - } else - status = release_advisory_lock(vnode, &flock); + } else { + status = release_advisory_lock(vnode, context, NULL, + &flock); + } } else { // the open mode must match the lock type if (((descriptor->open_mode & O_RWMASK) == O_RDONLY @@ -6234,7 +6252,7 @@ common_fcntl(int fd, int op, size_t argument, bool kernel) status = FS_CALL(vnode, acquire_lock, descriptor->cookie, &flock, op == F_SETLKW); } else { - status = acquire_advisory_lock(vnode, -1, + status = acquire_advisory_lock(vnode, context, NULL, &flock, op == F_SETLKW); } } @@ -9131,14 +9149,13 @@ _user_flock(int fd, int operation) if (HAS_FS_CALL(vnode, release_lock)) status = FS_CALL(vnode, release_lock, descriptor->cookie, &flock); else - status = release_advisory_lock(vnode, &flock); + status = release_advisory_lock(vnode, NULL, descriptor, &flock); } else { if (HAS_FS_CALL(vnode, acquire_lock)) { status = FS_CALL(vnode, acquire_lock, descriptor->cookie, &flock, (operation & LOCK_NB) == 0); } else { - status = acquire_advisory_lock(vnode, - thread_get_current_thread()->team->session_id, &flock, + status = acquire_advisory_lock(vnode, NULL, descriptor, &flock, (operation & LOCK_NB) == 0); } }