Thank you.
Here is a patch attached that tries to do what you suggested.
To test:
I have a way of semi deterministically triggering the behavior we are
trying to fix:
I have an abandoned journaling patch (attached) that can be applied to the
head of Hurd repo and then run in QEMU with little memory (perhaps only
512mb of ram)
and on the secondary journal enabled ext2fs (established with settrans) and
run this loop:
cd /mnt/stress/test && for i in {1..5}; do echo "remove start at: $(date
+'%T')" && rm -rf linux-7.2.3 && echo "untar start at: $(date +'%T')" &&
time tar -xf linux-7.2.3.tar.xz; done && sudo settrans -fg /mnt/stress &&
cd ~ && sudo /sbin/fsck.ext2 -fy /dev/sd0
(this will likely corrupt the file system on that drive, be warned), but it
will trigger the assert (because if changes the timing of port
deallocations)), for me it does after about 2-3 iterations in that loop,
semi-reliably.
Now with the fix attached, that doesn't happen any more...things just keep
going (and corrupt the partition due to a faulty journaling patch, lol), so
it seems like it is fixed (for me even after 1 hour of this loop the assert
doesn't trigger any failed assertions any more)
I also just run it on my system and upgrade, compile and do everything
normally, so i don't see any regressions.
Let me know please.
Thanks
Milos
On Sun, Sep 13, 2026 at 3:44 PM Samuel Thibault <[email protected]>
wrote:
> Hello,
>
> Milos Nikic, le ven. 11 sept. 2026 13:33:02 -0700, a ecrit:
> > Unfortunately, i only have the victim thread, not the destruction
> thread. Here
> > is the backtrace of the crash:
> >
> > Thread 26 (Thread 759.26):
> > #7 0x08048650 in __assert_fail_base_backtrace (fmt=...,
> assertion=0x81e3fa0 "!
> > (r.hard == 1 && r.weak == 0) || !\"refcount detected
> use-after-free!\""...) at
> > ../../libshouldbeinlibc/assert-backtrace.c:59
>
> Ah!
>
> You should have given this backtrace before...
>
> We have already discussed this a decade ago,
> http://lists.gnu.org/archive/html/bug-hurd/2016-03/msg00034.html
>
> with the same kind of workaround (but safer)
>
> > I want to write a patch to make the destruction path more cautious but
> because
> > dropping the refcount is atomic, and removing it from the ihash requires
> the
> > write-lock, there is an inherent race window unless we acquire the
> write-lock
> > before dropping the final reference (which would cause massive lock
> > contention).
> >
> > Do you have a preferred design in mind for how libports should safely
> hide
> > these dying ports from the read-locked iterators without killing
> performance?
>
> Actually here the assertion is erroneously raised: the entry was not
> actually freed (it's still in the hashtables etc.)
>
> The error is rather an improper use of reference counts: there is still
> a reference to the port, in the hash tables.
>
> I.e. we should be keeping a weak reference while the port is still in
> the hash tables, i.e. make _ports_create_port_internal/ports_import_port
> initialize the refcounts to 1, 1 instead of 1, 0. And then
> ports_port_deref_weak/ports_port_deref should look for hard being 0 and
> weak being 1, i.e. the only reference left is in the hashtable, in which
> case we indeed try to complete the deallocation, and once we have the
> wrlock we can check if it's indeed still 0 and 1, otherwise there was a
> reacquisition through the hashtable, we then abort the deallocation.
>
> Samuel
>
From fb9f567c097c5c1b5d14b2eccab8ebbadb3e35a8 Mon Sep 17 00:00:00 2001
From: Milos Nikic <[email protected]>
Date: Wed, 16 Sep 2026 09:07:15 -0700
Subject: [PATCH] libports: fix an erroneously raised assert in
_ports_bucket_class_iterate
There is an erroneously raised assert due to an improper use of reference
counts in _ports_bucket_class_iterate where an entry is not actually freed
because there still is a reference to the port, in the hash tables.
To fix it we keep a weak reference while the port is still in
the hash tables, and make _ports_create_port_internal/ports_import_port
initialize the refcounts to 1, 1 instead of 1, 0 and changing how
ports_port_deref_weak/ports_port_deref look for errors.
---
libports/complete-deallocate.c | 8 ++++----
libports/create-internal.c | 3 ++-
libports/import-port.c | 3 ++-
libports/port-deref-weak.c | 4 +++-
libports/port-deref.c | 4 +++-
libports/ports.h | 4 ++--
6 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/libports/complete-deallocate.c b/libports/complete-deallocate.c
index 25aba04f3..b7d5f765e 100644
--- a/libports/complete-deallocate.c
+++ b/libports/complete-deallocate.c
@@ -33,18 +33,18 @@ _ports_complete_deallocate (struct port_info *pi)
pthread_rwlock_wrlock (&_ports_htable_lock);
refcounts_references (&pi->refcounts, &result);
- if (result.hard > 0 || result.weak > 0)
+ if (result.hard > 0 || result.weak > 1)
{
/* A reference was reacquired through a hash table lookup.
- It's fine, we didn't touch anything yet. */
- /* XXX: This really shouldn't happen. */
- assert_backtrace (! "reacquired reference w/o send rights");
+ It's fine, we didn't touch anything yet. */
pthread_rwlock_unlock (&_ports_htable_lock);
return;
}
hurd_ihash_locp_remove (&_ports_htable, pi->ports_htable_entry);
hurd_ihash_locp_remove (&pi->bucket->htable, pi->hentry);
+ /* Drop the reference held by the hash tables. */
+ refcounts_deref_weak (&pi->refcounts, &result);
pthread_rwlock_unlock (&_ports_htable_lock);
mach_port_mod_refs (mach_task_self (), pi->port_right,
diff --git a/libports/create-internal.c b/libports/create-internal.c
index 853d541ea..8a52eaf49 100644
--- a/libports/create-internal.c
+++ b/libports/create-internal.c
@@ -54,7 +54,8 @@ _ports_create_port_internal (struct port_class *class,
}
pi->class = class;
- refcounts_init (&pi->refcounts, 1, 0);
+ /* The hash tables hold a weak reference to the port. */
+ refcounts_init (&pi->refcounts, 1, 1);
pi->cancel_threshold = 0;
pi->mscount = 0;
pi->flags = 0;
diff --git a/libports/import-port.c b/libports/import-port.c
index 3a13876be..1cdd3f5e1 100644
--- a/libports/import-port.c
+++ b/libports/import-port.c
@@ -48,7 +48,8 @@ ports_import_port (struct port_class *class, struct port_bucket *bucket,
return ENOMEM;
pi->class = class;
- refcounts_init (&pi->refcounts, 1 + !!stat.mps_srights, 0);
+ /* The hash tables hold a weak reference to the port. */
+ refcounts_init (&pi->refcounts, 1 + !!stat.mps_srights, 1);
pi->cancel_threshold = 0;
pi->mscount = stat.mps_mscount;
pi->flags = stat.mps_srights ? PORT_HAS_SENDRIGHTS : 0;
diff --git a/libports/port-deref-weak.c b/libports/port-deref-weak.c
index cb3f435ad..93bea3cff 100644
--- a/libports/port-deref-weak.c
+++ b/libports/port-deref-weak.c
@@ -27,6 +27,8 @@ ports_port_deref_weak (void *portstruct)
struct port_info *pi = portstruct;
struct references result;
refcounts_deref_weak (&pi->refcounts, &result);
- if (result.hard == 0 && result.weak == 0)
+ if (result.hard == 0 && result.weak == 1)
+ /* Only the reference held by the hash tables is left, try to
+ complete the deallocation. */
_ports_complete_deallocate (pi);
}
diff --git a/libports/port-deref.c b/libports/port-deref.c
index 34fa6f436..86c279792 100644
--- a/libports/port-deref.c
+++ b/libports/port-deref.c
@@ -43,6 +43,8 @@ ports_port_deref (void *portstruct)
else
refcounts_deref (&pi->refcounts, &result);
- if (result.hard == 0 && result.weak == 0)
+ if (result.hard == 0 && result.weak == 1)
+ /* Only the reference held by the hash tables is left, try to
+ complete the deallocation. */
_ports_complete_deallocate (pi);
}
diff --git a/libports/ports.h b/libports/ports.h
index defae8dd0..6ab4078bb 100644
--- a/libports/ports.h
+++ b/libports/ports.h
@@ -492,8 +492,8 @@ extern pthread_cond_t _ports_block;
/* A global hash table mapping port names to port_info objects. This
table is used for port lookups and to iterate over classes.
- A port in this hash table carries an implicit light reference.
- When the reference counts reach zero, we call
+ A port in this hash table carries an implicit weak reference.
+ When all the other references are dropped, we call
_ports_complete_deallocate. There we reacquire our lock
momentarily to check whether someone else reacquired a reference
through the hash table. */
--
2.55.0
From 41278002ee7b6155e624acf0c7aa8d9f2b540631 Mon Sep 17 00:00:00 2001
From: Milos Nikic <[email protected]>
Date: Sat, 5 Sep 2026 18:09:33 -0700
Subject: [PATCH] ext2fs: don't sync everything on journal checkpoint
---
ext2fs/ext2fs.h | 7 -------
ext2fs/journal.c | 21 ++++++++++++++++++++-
ext2fs/pager.c | 24 ------------------------
3 files changed, 20 insertions(+), 32 deletions(-)
diff --git a/ext2fs/ext2fs.h b/ext2fs/ext2fs.h
index 6f9d184d2..0975457d1 100644
--- a/ext2fs/ext2fs.h
+++ b/ext2fs/ext2fs.h
@@ -345,13 +345,6 @@ extern struct journal *ext2_journal;
error_t
journal_dirty_block (diskfs_transaction_t * txn, block_t fs_blocknr);
-/**
- * This function exists to sync all AND avoid a deadlock with commit.
- * It doesn't call journal_commit back yet it syncs everything.
- **/
-void
-journal_sync_everything (void);
-
void journal_notify_block_changed (block_t block);
/* ---------------------------------------------------------------- */
diff --git a/ext2fs/journal.c b/ext2fs/journal.c
index b25bf9de7..0df9b5029 100644
--- a/ext2fs/journal.c
+++ b/ext2fs/journal.c
@@ -697,6 +697,14 @@ kjournald_thread (void *arg)
if (diskfs_readonly)
continue;
+ JOURNAL_UNLOCK (journal);
+
+ /* Push dirty VFS inodes down into the global_pokel.
+ Because kjournald doesn't serve page faults, it is safe to
+ block on node locks here without starving the system. */
+ write_all_disknodes ();
+
+ JOURNAL_LOCK (journal);
if (journal->j_running_transaction)
{
JRNL_LOG_DEBUG ("Woke the journal up:\n"
@@ -713,6 +721,17 @@ kjournald_thread (void *arg)
JRNL_LOG_WARN ("Background commit failed: %s", strerror (err));
JOURNAL_LOCK (journal);
}
+ /* Asynchronously flush the global_pokel to the metal!
+ This trickle-flushes older transactions to the disk without
+ blocking, allowing the journal tail to continuously advance.
+ This prevents the journal from ever filling up and triggering
+ the fatal sync_global(1) emergency brake. */
+ if (journal->j_checkpoint_list)
+ {
+ JOURNAL_UNLOCK (journal);
+ sync_global (0);
+ JOURNAL_LOCK (journal);
+ }
}
JOURNAL_UNLOCK (journal);
return NULL;
@@ -1484,7 +1503,7 @@ journal_force_checkpoint_locked (journal_t *journal)
thread_is_checkpointing = 1;
deferred_count = 0;
- journal_sync_everything ();
+ sync_global (1);
/* Disarm the circuit breaker */
thread_is_checkpointing = 0;
diff --git a/ext2fs/pager.c b/ext2fs/pager.c
index 70c555bb8..0f35dab11 100644
--- a/ext2fs/pager.c
+++ b/ext2fs/pager.c
@@ -1581,30 +1581,6 @@ diskfs_shutdown_pager (void)
pager, just make sure it's synced. */
}
-static error_t
-journal_sync_one (void *v_p)
-{
- struct pager *p = v_p;
- pager_sync (p, 1);
- return 0;
-}
-
-/**
- * Sync all the pagers synchronously, but don't call
- * journal_commit here. It would deadlock.
- **/
-void
-journal_sync_everything (void)
-{
- write_all_disknodes ();
- ports_bucket_iterate (file_pager_bucket, journal_sync_one);
- sync_global (1);
- error_t err = store_sync (store);
- /* Ignore EOPNOTSUPP (drivers), but warn on real I/O errors */
- if (err && err != EOPNOTSUPP && err != D_INVALID_OPERATION)
- ext2_warning ("device flush failed: %s", strerror (err));
-}
-
/* Sync all the pagers. */
void
diskfs_sync_everything (int wait)
--
2.55.0
