Skip to content

Commit

Permalink
Fix leak of file descriptors for PhysicalInode:s
Browse files Browse the repository at this point in the history
This fixes flatpak#689

The mechanism we're using to flush the dcache and thus avoiding
long-term caching of physical inodes which keep a file descriptor open
doesn't seem to work quite right.

A trivial test of cat /run/user/1000/doc/$docid/$filename properly
leads to the PhysicalFile being freed. However, an application that
keeps the file open for a longer time (longer than the delay we have
before the current dcache flush workaround is run), ends up with the
dcache entry for the inode to be kept in cache. This means that we
when the last other kernel reference (like an open fd) to the inode is
gone we don't get a FORGET event from fuse, and thus we never free the
PhysicalInode.

This is clearly visible, because if you end up with the file
descriptor leak like in the issue, then flushing the dcache (`sudo sh
-c 'echo 2 > /proc/sys/vm/drop_caches'`) fixes the leak.

It seems the current workaround of just invalidating the toplevel
directory entry doesn't work, so we replace it with tracking each
individual lookup that ends up with a physical inode and flush all of
them. This seems to fix the issue for me.
  • Loading branch information
alexlarsson authored and hfiguiere committed Nov 22, 2023
1 parent 4b12da4 commit 63e3450
Showing 1 changed file with 52 additions and 22 deletions.
74 changes: 52 additions & 22 deletions document-portal/document-portal-fuse.c
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,6 @@ struct _XdpDomain {
guint64 doc_dir_inode;
guint32 doc_flags;

int doc_queued_invalidate; /* Access atomically, 1 if queued invalidate */

/* Below is mutable, protected by mutex */
GMutex tempfile_mutex;
GHashTable *tempfiles; /* Name -> physical */
Expand Down Expand Up @@ -223,6 +221,14 @@ typedef struct {
XdpInode *root_inode;
XdpInode *by_app_inode;

typedef struct {
ino_t parent_ino;
char name[0];
} XdpInvalidateData;

static GList *invalidate_list;
G_LOCK_DEFINE (invalidate_list);

static XdpInode *xdp_inode_ref (XdpInode *inode);
static void xdp_inode_unref (XdpInode *inode);

Expand All @@ -238,6 +244,8 @@ static int ensure_docdir_inode (XdpInode *parent,
struct fuse_entry_param *e,
XdpInode **inode_out);

static void queue_invalidate_dentry (XdpInode *parent, const char *name);

static gboolean
app_can_write_doc (PermissionDbEntry *entry, const char *app_id)
{
Expand Down Expand Up @@ -1061,6 +1069,8 @@ get_tempfile_for (XdpInode *parent,
/* This is atomic, because we're called with the lock held */
g_hash_table_replace (domain->tempfiles, tempfile->name, xdp_tempfile_ref (tempfile));

queue_invalidate_dentry (parent, tempfile->name);

if (tempfile_out)
*tempfile_out = g_steal_pointer (&tempfile);
return 0;
Expand Down Expand Up @@ -1108,6 +1118,8 @@ create_tempfile (XdpInode *parent,
/* This is atomic, because we're called with the lock held */
g_hash_table_replace (domain->tempfiles, tempfile->name, xdp_tempfile_ref (tempfile));

queue_invalidate_dentry (parent, tempfile->name);

if (tempfile_out)
*tempfile_out = g_steal_pointer (&tempfile);
return 0;
Expand Down Expand Up @@ -1707,37 +1719,53 @@ ensure_doc_inode (XdpInode *parent,
}

static gboolean
invalidate_doc_domain (gpointer user_data)
invalidate_dentry_cb (gpointer user_data)
{
g_autoptr(XdpDomain) doc_domain = user_data;
ino_t parent_ino;

g_atomic_int_set (&doc_domain->doc_queued_invalidate, 0);

parent_ino = xdp_inode_to_ino (doc_domain->parent_inode);
GList *to_invalidate = NULL;
{
XDP_AUTOLOCK (invalidate_list);
to_invalidate = g_steal_pointer (&invalidate_list);
}
to_invalidate = g_list_reverse (to_invalidate);

XDP_AUTOLOCK (session);
if (session && g_atomic_int_get (&doc_domain->parent_inode->kernel_ref_count) > 0)
fuse_lowlevel_notify_inval_entry (session, parent_ino, doc_domain->doc_id,
strlen (doc_domain->doc_id));
for (GList *l = to_invalidate; l != NULL; l = l->next)
{
XdpInvalidateData *data = l->data;
if (session)
fuse_lowlevel_notify_inval_entry (session, data->parent_ino, data->name,
strlen (data->name));
g_free (data);
}

g_list_free (to_invalidate);

return FALSE;
}

/* Queue an inval_entry call on this domain, thereby freeing all unused inodes
* in the dcache which will free up a bunch of O_PATH fds in the fuse implementation
/* Queue an inval_dentry, thereby freeing unused inodes in the dcache
* which will free up a bunch of O_PATH fds in the fuse implementation.
*/
static void
doc_domain_queue_entry_invalidate (XdpDomain *doc_domain)
queue_invalidate_dentry (XdpInode *parent, const char *name)
{
int old = g_atomic_int_get (&doc_domain->doc_queued_invalidate);
if (old != 0)
return;
XDP_AUTOLOCK (invalidate_list);

if (!g_atomic_int_compare_and_exchange (&doc_domain->doc_queued_invalidate, old, 1))
return; // Someone else set it to 1, return
for (GList *l = invalidate_list; l != NULL; l = l->next)
{
XdpInvalidateData *data = l->data;
if (data->parent_ino == parent->ino && strcmp (name, data->name) == 0)
return;
}

XdpInvalidateData *data = g_malloc0(sizeof(XdpInvalidateData) + strlen(name) + 1);
data->parent_ino = parent->ino;
strcpy(data->name, name);

g_timeout_add (1000, invalidate_doc_domain, xdp_domain_ref (doc_domain));
if (invalidate_list == NULL)
g_timeout_add (10, invalidate_dentry_cb, NULL);

invalidate_list = g_list_append(invalidate_list, data);
}

static void
Expand Down Expand Up @@ -1800,7 +1828,7 @@ xdp_fuse_lookup (fuse_req_t req,
if (res != 0)
return xdp_reply_err (op, req, -res);

doc_domain_queue_entry_invalidate (parent_domain);
queue_invalidate_dentry (parent, name);
}

g_debug ("LOOKUP %lx:%s => %lx", parent_ino, name, e.ino);
Expand Down Expand Up @@ -1910,6 +1938,8 @@ xdp_fuse_create (fuse_req_t req,
xdp_file_free (file);
abort_reply_entry (&e);
}

queue_invalidate_dentry (parent, filename);
}

static void
Expand Down

0 comments on commit 63e3450

Please sign in to comment.