Skip to content

Disk (Linux): Don't mark / as read-only in Fedora Atomic #1762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Disk (Linux): Don't mark / as read-only in Fedora Atomic #1762

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/detection/disk/disk_linux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,35 @@ static bool isRemovable(FFDisk* currentDisk)
return ffReadFileData(sysBlockVolume, 1, &removableChar) > 0 && removableChar == '1';
}

static bool isReadOnly(FFDisk* currentDisk, struct mntent* device)
{
if (!hasmntopt(device, MNTOPT_RO))
return false;

// For BTRFS on Fedora Atomic, check if there are other R/W mount points for the same device
if (ffStrbufEqualS(&currentDisk->filesystem, "btrfs"))
Copy link
Preview

Copilot AI May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Avoid using the literal string "btrfs"; consider defining a constant or using an enum for filesystem types to improve readability and reduce risk of typos.

Suggested change
if (ffStrbufEqualS(&currentDisk->filesystem, "btrfs"))
if (ffStrbufEqualS(&currentDisk->filesystem, FILESYSTEM_BTRFS))

Copilot uses AI. Check for mistakes.

Copy link
Author

@ledif ledif May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no FILESYSTEM_BTRFS enum defined anywhere and I do not believe it makes sense to create such an enum for an unbounded list of filesystem types.

{
const char* deviceName = device->mnt_fsname;
FILE* mountsFile = setmntent("/proc/mounts", "r");
if (mountsFile != NULL)
{
struct mntent* otherDev;
while ((otherDev = getmntent(mountsFile)))
{
// If same device but not read-only, then this device has R/W access
if (ffStrEquals(otherDev->mnt_fsname, deviceName) &&
!hasmntopt(otherDev, MNTOPT_RO))
{
endmntent(mountsFile);
return false;
}
}
endmntent(mountsFile);
}
Comment on lines +230 to +245
Copy link
Preview

Copilot AI May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Opening and scanning /proc/mounts on every call could add I/O overhead; consider caching the mounts list once per scan or reusing the open file between disk checks.

Suggested change
FILE* mountsFile = setmntent("/proc/mounts", "r");
if (mountsFile != NULL)
{
struct mntent* otherDev;
while ((otherDev = getmntent(mountsFile)))
{
// If same device but not read-only, then this device has R/W access
if (ffStrEquals(otherDev->mnt_fsname, deviceName) &&
!hasmntopt(otherDev, MNTOPT_RO))
{
endmntent(mountsFile);
return false;
}
}
endmntent(mountsFile);
}
static struct mntent** cachedMounts = NULL;
static size_t cachedMountsCount = 0;
if (cachedMounts == NULL)
{
FILE* mountsFile = setmntent("/proc/mounts", "r");
if (mountsFile != NULL)
{
struct mntent* entry;
size_t count = 0;
while ((entry = getmntent(mountsFile)))
count++;
rewind(mountsFile);
cachedMounts = malloc(count * sizeof(struct mntent*));
cachedMountsCount = count;
count = 0;
while ((entry = getmntent(mountsFile)))
{
cachedMounts[count] = malloc(sizeof(struct mntent));
*cachedMounts[count] = *entry;
count++;
}
endmntent(mountsFile);
}
}
for (size_t i = 0; i < cachedMountsCount; i++)
{
struct mntent* otherDev = cachedMounts[i];
// If same device but not read-only, then this device has R/W access
if (ffStrEquals(otherDev->mnt_fsname, deviceName) &&
!hasmntopt(otherDev, MNTOPT_RO))
{
return false;
}
}

Copilot uses AI. Check for mistakes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree that this patch potentially introduces an additional overhead where under certain conditions, the /proc/mounts file will be processed multiple times, I would argue that this situation only occurs in systems where a device is mounted read-only which itself is a rare condition and occurs most consistently for immutable-type distributions such as Fedora Atomic.

If we denote the number of /proc/mount entries as n and the number of such entries that are read-only as k, this patch increases the asymptotic complexity of this function from O(n) to O(n*(k+1)). However, k is usually 0 for most non-immutable systems and this patch would introduce no overhead. Creating a static cache of mount entries in my opinion is overkill for such a rare scenario.

}
return true;
}

static void detectType(const FFlist* disks, FFDisk* currentDisk, struct mntent* device)
{
if(ffStrbufStartsWithS(&currentDisk->mountpoint, "/boot") || ffStrbufStartsWithS(&currentDisk->mountpoint, "/efi"))
Expand All @@ -228,7 +257,7 @@ static void detectType(const FFlist* disks, FFDisk* currentDisk, struct mntent*
currentDisk->type = FF_DISK_VOLUME_TYPE_EXTERNAL_BIT;
else
currentDisk->type = FF_DISK_VOLUME_TYPE_REGULAR_BIT;
if (hasmntopt(device, MNTOPT_RO))
if (isReadOnly(currentDisk, device))
currentDisk->type |= FF_DISK_VOLUME_TYPE_READONLY_BIT;
}

Expand Down