Mounting two filesystems with the same label at the same time fails with "File exists"

[ajax-semenko-d]

Issue
If two block devices have the same filesystem label and Filesystem.Mount is called on both at roughly the same time, one of the calls fails:
GDBus.Error:org.freedesktop.UDisks2.Error.Failed: Error creating mount point '/run/media/root/ARCHIVE': File exists
The first device checks whether /run/media/root/ARCHIVE is available, gets TRUE, and moves on to udisks_linux_calculate_mount_options, where it spends some time. Meanwhile the second device checks the same path, also gets TRUE (nothing has been created yet), proceeds through udisks_linux_calculate_mount_options, and races ahead to g_mkdir - while the first device still thinks it owns /run/media/root/ARCHIVE and is about to call g_mkdir itself. Whichever thread calls g_mkdir first creates the directory and proceeds. The second one gets EEXIST and fails with error - even though ARCHIVE1 was free and the uniquify loop would have picked it had the check and create been atomic.
This is easy to hit in practice - two USB sticks from the same backup set, cloned drives, anything where labels match.
The window got wider with this change, and that's when we started seeing it after upgrading from 2.9.x to 2.10.x

Environment
udisks: 2.10.2 (but source code from master is affected as well)
Kernel: 5.10.61

Repro
Two devices with the same label, both unmounted. The script below fires two parallel Mount calls per iteration and stops at the first failing one.

#!/bin/sh
# repro_udisks_mount_race.sh
# Usage: ./repro_udisks_mount_race.sh <dev1> <dev2> [max_iters]

DEV1="${1:-}"; DEV2="${2:-}"; MAX_ITERS="${3:-1000}"
LABEL_PREFIX="${LABEL_PREFIX:-ARCHIVE}"
[ -n "$DEV1" ] && [ -n "$DEV2" ] || { echo "usage: $0 <dev1> <dev2> [n]" >&2; exit 2; }

OBJ1="/org/freedesktop/UDisks2/block_devices/$(basename "$DEV1")"
OBJ2="/org/freedesktop/UDisks2/block_devices/$(basename "$DEV2")"
WORK=$(mktemp -d)

dbus_call() {
    gdbus call --system --dest org.freedesktop.UDisks2 \
        --object-path "$1" \
        --method "org.freedesktop.UDisks2.Filesystem.$2" \
        '{}' >"$3" 2>&1
}
unmount() { dbus_call "$1" Unmount /dev/null || true; }
cleanup_stale() {
    for d in /run/media/*/${LABEL_PREFIX}*; do
        [ -d "$d" ] || continue
        mountpoint -q "$d" 2>/dev/null || rmdir "$d" 2>/dev/null || true
    done
}
cleanup_all() { unmount "$OBJ1"; unmount "$OBJ2"; cleanup_stale; rm -rf "$WORK"; }
trap cleanup_all EXIT INT TERM

unmount "$OBJ1"; unmount "$OBJ2"; cleanup_stale
i=0
while [ "$i" -lt "$MAX_ITERS" ]; do
    i=$((i + 1))
    O1="$WORK/m1"; O2="$WORK/m2"
    dbus_call "$OBJ1" Mount "$O1" & P1=$!
    dbus_call "$OBJ2" Mount "$O2" & P2=$!
    wait "$P1"; RC1=$?
    wait "$P2"; RC2=$?
    if [ "$RC1" -ne 0 ] || [ "$RC2" -ne 0 ]; then
        echo "===== iter $i: race triggered ====="
        echo "--- $DEV1 rc=$RC1 ---"; cat "$O1"
        echo "--- $DEV2 rc=$RC2 ---"; cat "$O2"
        exit 1
    fi
    unmount "$OBJ1"; unmount "$OBJ2"; cleanup_stale
    [ $((i % 25)) -eq 0 ] && echo "iter $i ok"
done
echo "Completed $MAX_ITERS iterations without triggering."

On stock udisks this is not stable and can take a lot of iterations, depends heavily on the timing, I/O load etc. If you want to see it on iteration 1, add a sleep between calculate_mount_point and g_mkdir to widen the window - it's just a debug aid, obviously not for merging:

diff--- a/src/udiskslinuxfilesystem.c
+++ b/src/udiskslinuxfilesystem.c
@@ -1176,6 +1176,9 @@ handle_mount_dynamic (UDisksDaemon          *daemon,
       return FALSE;
     }

+  /* DEBUG */
+  g_usleep (5 * G_USEC_PER_SEC);
+
   /* Calculate mount options (guaranteed to be valid UTF-8) */

Fix
The cleanest fix I can think of is to do the claim atomically: move g_mkdir(..., 0700) into the uniquify loop in calculate_mount_point, treat EEXIST as "next suffix please," report any other errno properly, and drop the now-redundant g_mkdir from handle_mount_dynamic.

[tbzatek]

Good point, since method calls are handled in threads, mountpoint creation is somewhat racy. I guess we'll need some locking around mountpoint calculation globally as this is not atomic. However even with internal locking it will always be racy with external factors.

[ajax-semenko-d]

Could we utilize mkdir(2) - which is atomic at the VFS level - to achieve serialization for both internal threads and external actors, rather than a userspace mutex?

I may be missing an edge-case - would appreciate a second pair of eyes. I tested this locally with an artificial delay injected into handle_mount_dynamic and with multiple concurrent threads; the race was consistently reproduced before the fix and does not appear after it.

I've opened a PR with this approach: #1515