hal: don't free a funct entry the running thread may still use (delf/unloadrt crash)

[yurc]

Symptom

delf of a function from a running thread, followed by unloadrt of its component, kills rtapi_app (halcmd: recv_result 1 failed: Connection reset by peer on unloadrt) or leaves the thread spinning.

Cause

The realtime thread walks thread->funct_list without the HAL mutex (thread_task()).

• hal_del_funct_from_thread() and free_funct_struct() unlink the entry with list_remove_entry(), which points the removed entry's next/prev at itself. A thread standing on that entry keeps calling it.
• The entry is returned to the free list at once, and unloadrt then dlclose()s the module while the thread may still execute its code.

Fix

• funct_entry_unlink(): take the entry out of the list but keep the entry's own links, so a thread standing on it continues to the rest of the list.
• funct_entry_release() frees the unlinked entry. With threads stopped nothing changes. With threads running:
  • delf (ULAPI): waits until <thread>.threadbeat advanced by two (the pin is looked up by name), at most 1000 thread periods. On timeout delf fails with -ETIMEDOUT, the entry is not recycled and an error says the function must not be unloaded.
  • unloadrt / hal_exit() (RTAPI): does not wait (no rtapi_delay() in RTAPI). The entry is unlinked but not recycled, and RTAPI_MSG_ERR says to stop the threads or delf the function before unloading.

So replacing a component while the threads keep running is: delf (waits), then unloadrt (the function is no longer in any thread).

Test

tests/hal-delf-live: the function of a small test component (delfvictim) keeps the thread inside for 500 periods when its pin hold is set. delftest.py sets hold, waits for inside, runs halcmd delf and checks that inside is false once delf has returned; then unloadrt and t.threadbeat must keep advancing. Pins are accessed with hal.set_p()/hal.get_p(); no nets, no wall-clock timeouts (only timeout in test.sh). skip when SYSTEM_BUILD is set, no sudo.

Results

uspace RIP, Ubuntu 24.04, non-root user:

tree	result
this branch, master hal_lib.c	5/5 failed (delf returns while the thread is inside the function)
this branch	10/10 passed
2.9 fe72abb20 + same test (2.9 variant)	5/5 failed
synctwin/hal-delf-quiescence-2.9	13/13 passed

Why

Replacing a HAL component while the servo thread keeps running, without stopping the machine.

2.9

2.9 has the same bug. It has no threadbeat pin, so a backport needs a pass counter in hal_thread_t (and a HAL_VER bump). A branch with the same behaviour exists: SyncTwin/linuxcnc synctwin/hal-delf-quiescence-2.9 (its test uses halcmd getp/setp and the component's own pass counter, since the 2.9 python hal module has no get_p). I can open it if you want it in 2.9.

Limits

• uspace only; not built or run on RTAI.
• Runs were without SCHED_FIFO (non-RT uspace).
• init_funct_list (initf) is not touched.

[grandixximo]

Interesting use case, just out of curiosity, what are you doing where you need swapping components with the thread running?
Ps:
test.sh fails shellcheck warning

[yurc]

2.9 backport, for reference: branch synctwin/hal-delf-quiescence-2.9 (2 commits on 2.9 fe72abb20: test 7cef18d9b, fix 8dee91099).

Differences from this PR:

• 2.9 has no pass counter, so hal_thread_t gets a private unsigned int beatcnt (no pin), incremented with a release store at the end of each pass in thread_task(). Unlink/wait/leak logic in hal_del_funct_from_thread() and free_funct_struct() is the same as here.
• Since a shared-memory struct changes, HAL_VER is bumped 0x10 -> 0x11 as the comment in hal_priv.h asks. Note 0x11 was also used on master for a while (a1c1347), so you may prefer a different value. hal_priv.h is not an installed header, so out-of-tree components are not affected.
• Test: no threadbeat pin in 2.9, so the test runs siggen.0.update in the same thread and checks its output still changes; uint param -> u32 (2.9 halcompile).

Results, same setup as above (ubuntu:24.04 container, uspace without SCHED_FIFO, --enable-werror, only this test, DELF_ITER=200, 10 runs each):

• 2.9 without the fix: 10/10 fail (unloadrt failed)
• 2.9 with the fix: 0/10 fail

If you'd rather have this in 2.9 as well, I can open a separate PR against 2.9.

[grandixximo]

@yurc did you see my text?

[yurc]

Hi Luca, sorry for the late reply, and thanks for the review! We use LinuxCNC not only for machine tools but also as the controller for automation cells: a rotary table, a conveyor, a palletizing robot, all on the same engine. Each cell's logic is an IEC 61131-3 program (SFC/ST) compiled by MATIEC into a HAL component. Motion goes through PLCopen-style blocks (MC_MoveAbsolute, MC_MoveVelocity, MC_Halt, MC_SetPosition). Cells talk to each other through PackML tags over NATS. The operator can swap a cell's program on a running station. The servo thread with the EtherCAT CiA402 drives keeps running, the drives stay in OP and hold position. So we load the new component and delf/unloadrt the old one while the thread is running. Restarting HAL just to change a program is not an option for us. Today's example: a servo rotary table following a hand gesture from a camera, with the angle coming in as a tag. That is how we hit the rtapi_app crash. The shellcheck warning (SC2034, unused loop variable) is fixed in 6af8e3b. вс, 4 окт. 2026 г. в 11:43, Luca Toniolo ***@***.***>:

…

*grandixximo* left a comment (LinuxCNC/linuxcnc#4630) <#4630 (comment)> Interesting use case, just out of curiosity, what are you doing where you need swapping components with the thread running? Ps: test.sh fails shellcheck warning — Reply to this email directly, view it on GitHub <#4630?email_source=notifications&email_token=AAU7VNU254YNFSWA25T3ROL5SIEULA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXHAZDCMJYGQ4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5978211848>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAU7VNRLMH5L367NARGRGMD5SIEULAVCNFSNUABDKJSXA33TNF2G64TZHMZTMNRSHEYDKO2JONZXKZJ3GU3DSNZVGUZTEMRXUF3AE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[grandixximo]

from no reply, to two in a row, nice ;-)

[grandixximo]

what's your handover sequence, do you halt motion blocks before delf, or does new comp take over nets first?

[yurc]

Thanks for the approval!

We do break-before-make, and only from a stopped node. The new component never takes over nets while the old one is still on them.

1. The swap happens only when the node is in PackML Stopped. A program swap during Execute is refused, and stopping the node first drops the enable request, so the drives go out of OP.
2. Unload: halt any in-flight motion-block player (MC_Halt on its axes) → node enable pin to 0 → delf the node's function from the servo thread → unloadrt the node comp → unload its userspace tag bridge → drop the node's nets (nets outlive unloadrt and would otherwise keep the mc-axis pins claimed).
3. Load: loadrt the new comp → net → addf → enable.

The base layer stays up the whole time: EtherCAT/cia402, the mc_axis motion blocks, the servo thread. So the swap is fast and the bus never leaves OP. Only the node's own function goes through delf, and that is exactly where the self-looped funct entry bit us.

[yurc]

Thanks for the review. Updated in 61c5143 and 4fd8613:

1. beatcnt and thread_task() are back to what master has. The wait reads the threadbeat pin with hal_get_sint(). The pin reference in hal_thread_t is only valid in the context that created the thread (hal_priv.h:423), and delf runs in halcmd, so it is mapped through the owner's shmem_base the same way unlink_pin() does.
2. The wait is now your sequence: read threadbeat; while threads are running, delay 2 periods first and 1 period after that, then stop once threadbeat has advanced by 2. rtapi_delay() is capped at rtapi_delay_max() (10 us in uspace rtapi_app), so the delay is split into chunks of that length. The 1000-period timeout and the leak path are removed.
3. users > 1 only happens for reentrant functs (hal_add_funct_to_thread() refuses otherwise, hal_lib.c:2387). Each entry is on exactly one thread's list, so only that thread can still be on it. delf frees that one entry after that thread has advanced by 2, and the other entries stay linked and valid. unloadrt (free_funct_struct()) waits on each thread for each entry it unlinks.
4. Tests: no sudo, and control is removed. beat-check.sh no longer compares two reads 0.1 s apart. It waits until threadbeat has advanced by 2, and a stuck thread is caught by the timeout in test.sh. shellcheck is clean.

Results: ubuntu:24.04 container, uspace without SCHED_FIFO, --enable-werror, runtests as a normal user without sudo:

• HAL/RTAPI tests (hal-, halcompile, halmodule, halrun-, module-loading, realtime-math, rtapi-shmem, symbols., threads., hal-delf-live): 71/71 pass
• hal-delf-live, DELF_ITER=200, 10 runs: 10/10 pass
• same test with master's hal_lib.c, 5 runs: 5/5 fail

[grandixximo]

@yurc the package-arch failures come from dropping ${SUDO} in 4fd8613. Those jobs build the debs, install them, and run the tests as an unprivileged user against the system install, so halcompile --install tries to write /usr/lib/linuxcnc/modules/delfvictim.so and gets Permission denied. The RIP jobs pass because there the install goes into the run-in-place rtlib, which is user-writable.

The established pattern for tests that build components is exactly what you had before:

${SUDO} halcompile --install delfvictim.comp

plus Restrictions: sudo in the control file (see tests/realtime-math, tests/symbols.0, tests/symbols.1, tests/rtapi-shmem). runtests exports SUDO=sudo only for system builds (scripts/runtests.in); on RIP builds it is empty, so the same line runs without sudo in rip-and-test and with sudo in package-arch. And runtests -u skips control-marked tests where sudo is not available.

@BsAtHome so "no sudo" is already true on RIP; ${SUDO} is the mechanism that makes the same test portable to both. Restoring ${SUDO} and the control file should turn package-arch green again while keeping everything else from the rework.

[yurc]

Restored ${SUDO} + control (Restrictions: sudo), bit → bool in delfvictim.comp: 85a3ac5.

[BsAtHome]

No, there should not be sudo, at all.

These tests are static tests and do not require the package install test. This is a build test. @grandixximo has put a build guard in place in multiple tests.

[grandixximo]

@BsAtHome agreed, and I had lost sight of that being the convention for new tests: skip when testing installed packages, no sudo anywhere. The ${SUDO} + control pattern in realtime-math and symbols.* is the older legacy shape.

@yurc concretely: keep test.sh as plain halcompile --install delfvictim.comp (no ${SUDO}), no control file, and add an executable skip file like tests/kins-frames/skip:

#!/bin/sh
# Builds a realtime component with halcompile, which needs the build
# tree.  Skip when testing installed packages.
[ -z "$SYSTEM_BUILD" ]

RIP jobs run the test, package-arch skips it, nothing ever touches /usr/lib/linuxcnc/modules.

[yurc]

Done in 005e951: plain halcompile --install delfvictim.comp, control file dropped, executable skip ([ -z "$SYSTEM_BUILD" ]) as in tests/kins-frames. RIP runs it, package-arch skips it.

Checked on this branch (uspace RIP, Ubuntu 24.04, non-root user): runtests tests/halcompile tests/kins-frames tests/hal-delf-live 12/12 pass; hal-delf-live 5 x 200 iterations, 0 failed; with SYSTEM_BUILD=1 it reports "Skipping disabled test: tests/hal-delf-live".

[BsAtHome]

Casting and fiddling with the pin reference is a bad way to do it. You know the name of the thread when you get here. Therefore, you can determine and construct the full pinname. All this code is only called from non-RT (ULAPI), so that is no problem. You can then use halpr_find_pin_by_name() and you need to use SHMPTR on the reference (and need to do signal deref). Easier is to call hal_getref_p() with the name and HAL_QTYPE_PIN limitation to retrieve the actual pin reference.

You should also add a comment why you need to do it that way (because of the context sensitivity of pin addressing).

[yurc]

Done in d36ff69: the wait now builds <thread>.threadbeat and resolves it with halpr_find_pin_by_name(), then follows the signal or dummysig the way hal_getref_p() does. I didn't call hal_getref_p() directly because it is ULAPI only, and unloadrt reaches this wait from RTAPI through free_funct_struct(). A comment explains this.

[BsAtHome]

But you are not really allowed to call rtapi_delay from RTAPI. That is problematic.

Unloading while the threads are running is a problem. Normally, a stop command is issued before unloading is done (at program termination). I guess no one has ever considered your use case. This may need some more consideration to see what alternatives there are.

[yurc]

Agreed. In 2fcb0d4 nothing waits in RTAPI any more:

• delf (hal_del_funct_from_thread() from halcmd, ULAPI) waits for the thread to leave the entry. rtapi_delay() is already used to wait in ULAPI in hal_port_wait_readable().
• In RTAPI (free_funct_struct() from hal_exit(), or a component's exit code calling hal_del_funct_from_thread(), like scope_rt), there is no wait. If the threads are running, the entry is unlinked but not recycled, and RTAPI_MSG_ERR says to stop the threads or delf the function before unloading. With stopped threads nothing changes.

The use case is replacing a component while the threads keep running: delf first (it waits), then unloadrt (no entries left). The usual stop-then-unload path is unchanged. If you'd rather have halcmd refuse unloadrt (in ULAPI) while the component still has functions in a running thread, I can add that as a separate change.

[BsAtHome]

The use case is replacing a component while the threads keep running: delf first (it waits), then unloadrt (no entries left). The usual stop-then-unload path is unchanged. If you'd rather have halcmd refuse unloadrt (in ULAPI) while the component still has functions in a running thread, I can add that as a separate change.

Indeed, it is a good idea to have the unload code test whether there are any functions in use from the component being unloaded. If there are, then it should fail.

This must be done in the hal_lib code when trying to remove the component and holding the mutex to prevent a race between test and unload. First test whether functions of that component are in use an then unload the component. That should work and be sufficient.

[yurc]

Done in 32a602d/0b36e4861. hal_comp_check_unload() in hal_lib takes the mutex and returns -EBUSY (naming each function) when a function of the component is in a thread while the threads run; halcmd unloadrt calls it before unloading and fails. It has to run before the unload request: rtapi_app_exit() is void and rtapi_app dlclose()s the module whatever hal_exit() returns, so hal_exit() itself cannot refuse.

delf still waits: unlinking does not move a thread that is already on the entry, and freeing the entry at once would drop funct->users and pass the unload check while the thread is still inside. If delf times out, the entry is not freed, users stays non-zero, and unloadrt is refused.

The test now checks: unloadrt refused before delf, works after it, and stop/unloadrt/start unchanged; every wait aborts after 10 s. Indentation of the edited lines restored; the threadbeat message prints the error. Merged onto current master: hal/rtapi tests 71/71, hal-delf-live 10/10 (uspace RIP, non-root).

halrmt.cc has its own unloadrt_comp(); I left it as is — happy to add the same check there if you want.

[BsAtHome]

But you are not really allowed to call rtapi_delay from RTAPI. That is problematic.

Unloading while the threads are running is a problem. Normally, a stop command is issued before unloading is done (at program termination). I guess no one has ever considered your use case. This may need some more consideration to see what alternatives there are.

[BsAtHome]

What happened to the indenting?

[BsAtHome]

Here too, what happened to the indenting?

[BsAtHome]

Dotting i, crossing t and splitting hairs... Since you now use hal_getref_p, you can capture its return value and print it with the message.

    int rv = hal_getref_p(&q);
    if(0 != rv) {
        /* This is a *very* serious error.
           We have a thread that has missing interface pins */
        rtapi_print_msg(RTAPI_MSG_ERR,
            "HAL: ERROR: thread_wait_quiescent: pin '%s' cannot be found, error=%d\n",
            name, rv);
...

[BsAtHome]

This probably needs a guard or infinite waits are possible. We expect things to advance, but should plan for the worst and die with an error in that case.

If this runs for more that 10 seconds of wall time, then we are on a machine that is dying or dead. It is better to abort than to block the runtests process.