hrev52646 adds 4 changesets to branch 'master'
old head: 675e5dfcac36505f55f6d5b3f4c7692038f01b63
new head: e7e7a55250c1b00a82cef3676c6a035b06044cfb
overview:
https://git.haiku-os.org/haiku/log/?qt=range&q=e7e7a55250c1+%5E675e5dfcac36
----------------------------------------------------------------------------
15ec0b5cc8c0: Revert "VFS: Slight rework of the FD disconnect feature."
This reverts commit eb62d3337b82f4dce6b1a0b3f116e38491c66f14.
64d1636feace: kernel: Rework the FD disconnect feature (again).
axeld's solution from 2015 worked in that it solved the panics and
problems with leaking FDs ... but only if nobody actually tried to
use the FDs again. As you can see in the diff of the previous commit,
in allowing closed FDs (which have NULL "ops") to be returned by get_fd,
all consumers of the get_fd API (so, pretty much most functions in
vfs.cpp and fd.cpp) have to check *both* that (1) the fd is not NULL,
and (2) the fd does not have O_DISCONNECT set.
Besides missing a large majority of consumers of get_fd (which caused
ticket #14532 and also the first half of ticket #14756, probably among
others, as I haven't reviewed all NULL-dereference-in-VFS tickets yet)
this solution missed the fact that calling get_fd increments the reference
count, but then exiting the exact same way as if the FD was NULL
(without putting it) when it is disconnected *also* leaks the FD.
As it turns out, a not insignificant number of applications try
to do this, which (depending on whether you went through one of the
'lucky' functions axeld's commit touched) either (1) leaked the FD,
or (2) caused a kernel panic.
Now, we could go through and add O_DISCONNECT checks to every single
consumer of get_fd followed by put_fd to get the proper behavior ...
but that would be the same thing as just returning NULL here and not
incrementing the reference count.
So it seems the first part of axeld's solution (don't set open_count
or ref_count to -1 but leave them as-is) is the only change necessary.
A few places where there were legitimately missing O_DISCONNECT checks
(some originally added by axeld) are (re-)added in the next commit.
Otherwise this seems to be the more robust solution. (But I wonder why
nobody caught this in the code review axeld asked for in the commit
and the ticket back in 2015? Did nobody notice the unbalanced get/put?)
Fixes #14532, part of #14756, and probably any other NULL dereferences
in VFS I/O functions (XHCI is especially good at exposing these)
that are lingering around on the bugtracker.
bee962a25eca: kernel/fs: Consumers of context->fds[] must check O_DISCONNECTED.
Since these do not go through get_fd, which would check for them,
we need to do these checks manually in the relevant locations.
Some of these changes were broken out from axeld's original commit,
and some were found by my own auditing.
e7e7a55250c1: kernel/fs: Account for vnode being NULL in vfs_release_posix_lock.
When the FD is put() but not freed while O_DISCONNECTED, its "ops"
and "vnode" are cleared. Thus it is entirely valid for a non-NULL
file FD to have a NULL vnode, so we should just treat such FDs
as if the locks had already been cleared (which they should have.)
Fixes #14294.
[ Augustin Cavalier <waddlesplash@xxxxxxxxx> ]
----------------------------------------------------------------------------
2 files changed, 36 insertions(+), 28 deletions(-)
src/system/kernel/fs/fd.cpp | 34 +++++++++++++++++++---------------
src/system/kernel/fs/vfs.cpp | 30 +++++++++++++++++-------------
############################################################################
Commit: 15ec0b5cc8c0fad62c63f0ddef75668e417db4a1
URL: https://git.haiku-os.org/haiku/commit/?id=15ec0b5cc8c0
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Wed Dec 12 23:40:04 2018 UTC
Revert "VFS: Slight rework of the FD disconnect feature."
This reverts commit eb62d3337b82f4dce6b1a0b3f116e38491c66f14.
----------------------------------------------------------------------------
diff --git a/src/system/kernel/fs/fd.cpp b/src/system/kernel/fs/fd.cpp
index 225a47a6c8..32fc18e7ad 100644
--- a/src/system/kernel/fs/fd.cpp
+++ b/src/system/kernel/fs/fd.cpp
@@ -224,11 +224,13 @@ put_fd(struct file_descriptor* descriptor)
descriptor->ops->fd_free(descriptor);
// prevent this descriptor from being closed/freed again
+ descriptor->open_count = -1;
+ descriptor->ref_count = -1;
descriptor->ops = NULL;
descriptor->u.vnode = NULL;
// the file descriptor is kept intact, so that it's not
- // reused until someone explicitly closes it
+ // reused until someone explicetly closes it
}
}
@@ -297,8 +299,13 @@ get_fd_locked(struct io_context* context, int fd)
struct file_descriptor* descriptor = context->fds[fd];
if (descriptor != NULL) {
- TFD(GetFD(context, fd, descriptor));
- inc_fd_ref_count(descriptor);
+ // Disconnected descriptors cannot be accessed anymore
+ if (descriptor->open_mode & O_DISCONNECTED)
+ descriptor = NULL;
+ else {
+ TFD(GetFD(context, fd, descriptor));
+ inc_fd_ref_count(descriptor);
+ }
}
return descriptor;
@@ -320,7 +327,7 @@ get_open_fd(struct io_context* context, int fd)
MutexLocker _(context->io_mutex);
file_descriptor* descriptor = get_fd_locked(context, fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return NULL;
atomic_add(&descriptor->open_count, 1);
@@ -424,8 +431,7 @@ dup2_fd(int oldfd, int newfd, bool kernel)
// the table size could be changed)
if ((uint32)oldfd >= context->table_size
|| (uint32)newfd >= context->table_size
- || context->fds[oldfd] == NULL
- || (context->fds[oldfd]->open_mode & O_DISCONNECTED) != 0) {
+ || context->fds[oldfd] == NULL) {
mutex_unlock(&context->io_mutex);
return B_FILE_ERROR;
}
@@ -506,10 +512,10 @@ fd_ioctl(bool kernelFD, int fd, uint32 op, void* buffer,
size_t length)
int status;
descriptor = get_fd(get_current_io_context(kernelFD), fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
- if (descriptor->ops->fd_ioctl != NULL)
+ if (descriptor->ops->fd_ioctl)
status = descriptor->ops->fd_ioctl(descriptor, op, buffer,
length);
else
status = B_DEV_INVALID_IOCTL;
@@ -565,7 +571,7 @@ select_fd(int32 fd, struct select_info* info, bool kernel)
MutexLocker locker(context->io_mutex);
struct file_descriptor* descriptor = fdGetter.SetTo(context, fd, true);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
uint16 eventsToSelect = info->selected_events & ~B_EVENT_INVALID;
@@ -687,7 +693,7 @@ fd_is_valid(int fd, bool kernel)
{
struct file_descriptor* descriptor
= get_fd(get_current_io_context(kernel), fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return false;
put_fd(descriptor);
@@ -728,7 +734,7 @@ common_user_io(int fd, off_t pos, void* buffer, size_t
length, bool write)
FDGetter fdGetter;
struct file_descriptor* descriptor = fdGetter.SetTo(fd, false);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (!descriptor)
return B_FILE_ERROR;
if (write ? (descriptor->open_mode & O_RWMASK) == O_RDONLY
@@ -780,7 +786,7 @@ common_user_vector_io(int fd, off_t pos, const iovec*
userVecs, size_t count,
FDGetter fdGetter;
struct file_descriptor* descriptor = fdGetter.SetTo(fd, false);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (!descriptor)
return B_FILE_ERROR;
if (write ? (descriptor->open_mode & O_RWMASK) == O_RDONLY
@@ -893,12 +899,12 @@ _user_seek(int fd, off_t pos, int seekType)
struct file_descriptor* descriptor;
descriptor = get_fd(get_current_io_context(false), fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (!descriptor)
return B_FILE_ERROR;
TRACE(("user_seek(descriptor = %p)\n", descriptor));
- if (descriptor->ops->fd_seek != NULL)
+ if (descriptor->ops->fd_seek)
pos = descriptor->ops->fd_seek(descriptor, pos, seekType);
else
pos = ESPIPE;
@@ -939,7 +945,7 @@ _user_read_dir(int fd, struct dirent* userBuffer, size_t
bufferSize,
io_context* ioContext = get_current_io_context(false);
FDGetter fdGetter;
struct file_descriptor* descriptor = fdGetter.SetTo(ioContext, fd,
false);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
if (descriptor->ops->fd_read_dir == NULL)
@@ -985,10 +991,10 @@ _user_rewind_dir(int fd)
TRACE(("user_rewind_dir(fd = %d)\n", fd));
descriptor = get_fd(get_current_io_context(false), fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
- if (descriptor->ops->fd_rewind_dir != NULL)
+ if (descriptor->ops->fd_rewind_dir)
status = descriptor->ops->fd_rewind_dir(descriptor);
else
status = B_UNSUPPORTED;
@@ -1126,7 +1132,7 @@ _kern_write(int fd, off_t pos, const void* buffer, size_t
length)
FDGetter fdGetter;
struct file_descriptor* descriptor = fdGetter.SetTo(fd, true);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
if ((descriptor->open_mode & O_RWMASK) == O_RDONLY)
return B_FILE_ERROR;
@@ -1254,7 +1260,7 @@ _kern_read_dir(int fd, struct dirent* buffer, size_t
bufferSize,
struct io_context* ioContext = get_current_io_context(true);
descriptor = get_fd(ioContext, fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
if (descriptor->ops->fd_read_dir) {
@@ -1280,7 +1286,7 @@ _kern_rewind_dir(int fd)
TRACE(("sys_rewind_dir(fd = %d)\n",fd));
descriptor = get_fd(get_current_io_context(true), fd);
- if (descriptor == NULL || (descriptor->open_mode & O_DISCONNECTED) != 0)
+ if (descriptor == NULL)
return B_FILE_ERROR;
if (descriptor->ops->fd_rewind_dir)
diff --git a/src/system/kernel/fs/vfs.cpp b/src/system/kernel/fs/vfs.cpp
index e5de2b366f..3993058c57 100644
--- a/src/system/kernel/fs/vfs.cpp
+++ b/src/system/kernel/fs/vfs.cpp
@@ -4990,8 +4990,7 @@ vfs_new_io_context(io_context* parentContext, bool
purgeCloseOnExec)
for (i = 0; i < tableSize; i++) {
struct file_descriptor* descriptor =
parentContext->fds[i];
- if (descriptor != NULL
- && (descriptor->open_mode &
O_DISCONNECTED) == 0) {
+ if (descriptor != NULL) {
bool closeOnExec =
fd_close_on_exec(parentContext, i);
if (closeOnExec && purgeCloseOnExec)
continue;
############################################################################
Commit: 64d1636feacea3dbc59b92bd6050c6889bddd481
URL: https://git.haiku-os.org/haiku/commit/?id=64d1636feace
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Wed Dec 12 23:42:03 2018 UTC
Ticket: https://dev.haiku-os.org/ticket/14532
Ticket: https://dev.haiku-os.org/ticket/14756
kernel: Rework the FD disconnect feature (again).
axeld's solution from 2015 worked in that it solved the panics and
problems with leaking FDs ... but only if nobody actually tried to
use the FDs again. As you can see in the diff of the previous commit,
in allowing closed FDs (which have NULL "ops") to be returned by get_fd,
all consumers of the get_fd API (so, pretty much most functions in
vfs.cpp and fd.cpp) have to check *both* that (1) the fd is not NULL,
and (2) the fd does not have O_DISCONNECT set.
Besides missing a large majority of consumers of get_fd (which caused
ticket #14532 and also the first half of ticket #14756, probably among
others, as I haven't reviewed all NULL-dereference-in-VFS tickets yet)
this solution missed the fact that calling get_fd increments the reference
count, but then exiting the exact same way as if the FD was NULL
(without putting it) when it is disconnected *also* leaks the FD.
As it turns out, a not insignificant number of applications try
to do this, which (depending on whether you went through one of the
'lucky' functions axeld's commit touched) either (1) leaked the FD,
or (2) caused a kernel panic.
Now, we could go through and add O_DISCONNECT checks to every single
consumer of get_fd followed by put_fd to get the proper behavior ...
but that would be the same thing as just returning NULL here and not
incrementing the reference count.
So it seems the first part of axeld's solution (don't set open_count
or ref_count to -1 but leave them as-is) is the only change necessary.
A few places where there were legitimately missing O_DISCONNECT checks
(some originally added by axeld) are (re-)added in the next commit.
Otherwise this seems to be the more robust solution. (But I wonder why
nobody caught this in the code review axeld asked for in the commit
and the ticket back in 2015? Did nobody notice the unbalanced get/put?)
Fixes #14532, part of #14756, and probably any other NULL dereferences
in VFS I/O functions (XHCI is especially good at exposing these)
that are lingering around on the bugtracker.
----------------------------------------------------------------------------
diff --git a/src/system/kernel/fs/fd.cpp b/src/system/kernel/fs/fd.cpp
index 32fc18e7ad..838ae1fa78 100644
--- a/src/system/kernel/fs/fd.cpp
+++ b/src/system/kernel/fs/fd.cpp
@@ -224,13 +224,11 @@ put_fd(struct file_descriptor* descriptor)
descriptor->ops->fd_free(descriptor);
// prevent this descriptor from being closed/freed again
- descriptor->open_count = -1;
- descriptor->ref_count = -1;
descriptor->ops = NULL;
descriptor->u.vnode = NULL;
// the file descriptor is kept intact, so that it's not
- // reused until someone explicetly closes it
+ // reused until someone explicitly closes it
}
}
@@ -299,13 +297,12 @@ get_fd_locked(struct io_context* context, int fd)
struct file_descriptor* descriptor = context->fds[fd];
if (descriptor != NULL) {
- // Disconnected descriptors cannot be accessed anymore
+ // disconnected descriptors cannot be accessed anymore
if (descriptor->open_mode & O_DISCONNECTED)
- descriptor = NULL;
- else {
- TFD(GetFD(context, fd, descriptor));
- inc_fd_ref_count(descriptor);
- }
+ return NULL;
+
+ TFD(GetFD(context, fd, descriptor));
+ inc_fd_ref_count(descriptor);
}
return descriptor;
############################################################################
Commit: bee962a25eca79801ba4b4d2b157a5d16d61e187
URL: https://git.haiku-os.org/haiku/commit/?id=bee962a25eca
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Thu Dec 13 00:03:11 2018 UTC
kernel/fs: Consumers of context->fds[] must check O_DISCONNECTED.
Since these do not go through get_fd, which would check for them,
we need to do these checks manually in the relevant locations.
Some of these changes were broken out from axeld's original commit,
and some were found by my own auditing.
----------------------------------------------------------------------------
diff --git a/src/system/kernel/fs/fd.cpp b/src/system/kernel/fs/fd.cpp
index 838ae1fa78..df3af83434 100644
--- a/src/system/kernel/fs/fd.cpp
+++ b/src/system/kernel/fs/fd.cpp
@@ -428,7 +428,8 @@ dup2_fd(int oldfd, int newfd, bool kernel)
// the table size could be changed)
if ((uint32)oldfd >= context->table_size
|| (uint32)newfd >= context->table_size
- || context->fds[oldfd] == NULL) {
+ || context->fds[oldfd] == NULL
+ || (context->fds[oldfd]->open_mode & O_DISCONNECTED) != 0) {
mutex_unlock(&context->io_mutex);
return B_FILE_ERROR;
}
diff --git a/src/system/kernel/fs/vfs.cpp b/src/system/kernel/fs/vfs.cpp
index 3993058c57..64d29413ad 100644
--- a/src/system/kernel/fs/vfs.cpp
+++ b/src/system/kernel/fs/vfs.cpp
@@ -1955,21 +1955,23 @@ disconnect_mount_or_vnode_fds(struct fs_mount* mount,
sRoot, false);
for (uint32 i = 0; i < context->table_size; i++) {
- if (struct file_descriptor* descriptor =
context->fds[i]) {
- inc_fd_ref_count(descriptor);
-
- // if this descriptor points at this mount, we
- // need to disconnect it to be able to unmount
- struct vnode* vnode = fd_vnode(descriptor);
- if (vnodeToDisconnect != NULL) {
- if (vnode == vnodeToDisconnect)
- disconnect_fd(descriptor);
- } else if ((vnode != NULL && vnode->mount ==
mount)
- || (vnode == NULL &&
descriptor->u.mount == mount))
+ struct file_descriptor* descriptor = context->fds[i];
+ if (descriptor == NULL || (descriptor->open_mode &
O_DISCONNECTED) != 0)
+ continue;
+
+ inc_fd_ref_count(descriptor);
+
+ // if this descriptor points at this mount, we
+ // need to disconnect it to be able to unmount
+ struct vnode* vnode = fd_vnode(descriptor);
+ if (vnodeToDisconnect != NULL) {
+ if (vnode == vnodeToDisconnect)
disconnect_fd(descriptor);
+ } else if ((vnode != NULL && vnode->mount == mount)
+ || (vnode == NULL && descriptor->u.mount ==
mount))
+ disconnect_fd(descriptor);
- put_fd(descriptor);
- }
+ put_fd(descriptor);
}
}
}
@@ -4990,7 +4992,8 @@ vfs_new_io_context(io_context* parentContext, bool
purgeCloseOnExec)
for (i = 0; i < tableSize; i++) {
struct file_descriptor* descriptor =
parentContext->fds[i];
- if (descriptor != NULL) {
+ if (descriptor != NULL
+ && (descriptor->open_mode &
O_DISCONNECTED) == 0) {
bool closeOnExec =
fd_close_on_exec(parentContext, i);
if (closeOnExec && purgeCloseOnExec)
continue;
############################################################################
Revision: hrev52646
Commit: e7e7a55250c1b00a82cef3676c6a035b06044cfb
URL: https://git.haiku-os.org/haiku/commit/?id=e7e7a55250c1
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Thu Dec 13 00:24:59 2018 UTC
Ticket: https://dev.haiku-os.org/ticket/14294
kernel/fs: Account for vnode being NULL in vfs_release_posix_lock.
When the FD is put() but not freed while O_DISCONNECTED, its "ops"
and "vnode" are cleared. Thus it is entirely valid for a non-NULL
file FD to have a NULL vnode, so we should just treat such FDs
as if the locks had already been cleared (which they should have.)
Fixes #14294.
----------------------------------------------------------------------------
diff --git a/src/system/kernel/fs/vfs.cpp b/src/system/kernel/fs/vfs.cpp
index 64d29413ad..c28608847f 100644
--- a/src/system/kernel/fs/vfs.cpp
+++ b/src/system/kernel/fs/vfs.cpp
@@ -4891,6 +4891,8 @@ status_t
vfs_release_posix_lock(io_context* context, struct file_descriptor* descriptor)
{
struct vnode* vnode = descriptor->u.vnode;
+ if (vnode == NULL)
+ return B_OK;
if (HAS_FS_CALL(vnode, release_lock))
return FS_CALL(vnode, release_lock, descriptor->cookie, NULL);
