diff mbox series

Fstab xattrs: This update ensures that fstab xattrs are correctly updated without adding a lock to the database.

Message ID 20251019153922.27208-2-dani.barra25@gmail.com
State New
Headers show
Series Fstab xattrs: This update ensures that fstab xattrs are correctly updated without adding a lock to the database. | expand

Commit Message

Daniel Andrade Oct. 19, 2025, 3:39 p.m. UTC
From: Daniel Andrade <dani.barra25@gmail.com>

The process of repairing the database using -B flag of pseudo sometimes causes errors because the database lock file is present.
Therefore, to fix it, first we ensure that any connection to files.db is closed using pseudo flag -S followed by the actual command to
repair it after fstab was successfully updated.

Signed-off-by: Daniel Andrade <dani.barra25@gmail.com>
---
 scripts/lib/wic/partition.py             | 15 +--------------
 scripts/lib/wic/plugins/source/rootfs.py | 19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 19 deletions(-)

Comments

Alexander Kanavin Oct. 20, 2025, 12:09 p.m. UTC | #1
On Sun, 19 Oct 2025 at 23:45, dani.barra25 via lists.openembedded.org
<dani.barra25=gmail.com@lists.openembedded.org> wrote:

> +                    pseudo_prefix = cls.__get_pseudo(native_sysroot, new_rootfs, new_pseudo)
> +
> +                    # Shutdown any existing pseudo server
> +                    # This ensures no other processes are using the database
> +                    shutdown_cmd = "%s -S" % pseudo_prefix
> +                    exec_native_cmd(shutdown_cmd, native_sysroot)
> +
> +                    # Database is not locked anymore
> +                    # Repairs inodes related to fstab. This way xattrs and metadata is correctly applied to the new file
> +                    repair_cmd = "%s -B" % pseudo_prefix
> +                    exec_native_cmd(repair_cmd, native_sysroot)
> +

Is it possible to describe how to reproduce and observe the issue that
this patch aims to fix? This snippet makes me *very* nervous as pseudo
is used by mulitple bitbake tasks in parallel, and doing shutdowns and
repairs in the middle of it can blow up spectacularly.

Alex
Daniel Andrade Oct. 20, 2025, 1:18 p.m. UTC | #2
Hello Alex,
Yes I can. Maybe I am doing something wrong since the patch is an
alternative to past threads but somehow it gets separated without context.

Starting from the beginning.
My original patch aimed to fix a simple thing. No matter what fstype you
were using, no xattrs from the build process were assigned to /etc/fstab if
you had multiple partitions. And this was particularly problematic when you
have read-only rootfs like squashfs and you need to have all the metadata
available during build, like the SELinux file labels, since they cannot be
set during boot time.
This is because it creates a temp folder and then creates a new fstab that
is later replaced in the image.

The replacement is made via 2 options:
1. If it was ext* or msdos, it will fall under specific methods for those
types that will open the already built image and remove the fstab and
writing the new one, losing all the metadata of that file.
2. It falls under the "install" cmd of rootfs.py and this one was being
executed inside the pseudo environment. This means the original entry of
fstab on files db is completely replace with this new file and all the
xattrs are gone

To avoid this, my first patch removed the debugfs specifics of ext* and
msdos, since those did not leveragy any "write only content" mechanism
without replacing the actual file. Then, all the fstab modifications would
lie under rootfs.py
which seemed to be the common point for everything. On this I simply
replaced "install" with "cp" since I only wanted to modify the contents.
https://lists.openembedded.org/g/openembedded-core/topic/115043355
Later in the discussion, Mathieu correctly pointed out that it could not be
used because it fails a test due to the usage of copyhardlinktree() for
this temporary rootfs folder. This meant that I was not only modifying the
temporary fstab, but also the original one,
leading to a crash of wic.Wic.test_no_fstab_update.
https://lists.openembedded.org/g/openembedded-core/topic/115043356

The second approach I took was looking at how pseudo behaved. This time, I
decided to maintain the original install cmd, but instead of running it
under fakeroot, I ran it on the host system. This meant that, if a later
check of the db occurred, it would correctly detect
an inode change on fstab and therefore it could fix it using the -B option.
And that is what I did :
https://lists.openembedded.org/g/openembedded-core/topic/115805391

I only neglected the fact that, in reality, other tests could have failed
because of this. And, in fact, it happened. Because of this, tests
regarding including and excluding paths were failing because the db was
locked.
That's when I reached the current patch alternative.
I knew it could cause problems but honestly I never considered it would and
when I run wic tests, I didn't see any failure. In reality, I was trying to
replicate what was being done a few lines above with the options -B -m and
-M. The difference
is that pseudo doesn't have a flag to repair a single file, therefore this
was the only solution found.

A good alternative to this would be pseudo actually having a functionality
to repair single file inodes, avoiding to completely check the whole
database and therefore better management on this side.
In the end this bug rendered all my images that enforced SELinux with read
only rootfs (squashfs) useless and I'd assume I might not be the only one
with this issue.

Another thing I notice is that, for repeatability purposes, there are
multiple variables being used to timestamp files. This is just a
recommendation/question. Shouldn't all those variable timestamps be aligned
also?

In any case, if you have some other idea on how to approach this, let me
know!

Daniel

Alexander Kanavin <alex.kanavin@gmail.com> escreveu (segunda, 20/10/2025
à(s) 13:09):

> On Sun, 19 Oct 2025 at 23:45, dani.barra25 via lists.openembedded.org
> <dani.barra25=gmail.com@lists.openembedded.org> wrote:
>
> > +                    pseudo_prefix = cls.__get_pseudo(native_sysroot,
> new_rootfs, new_pseudo)
> > +
> > +                    # Shutdown any existing pseudo server
> > +                    # This ensures no other processes are using the
> database
> > +                    shutdown_cmd = "%s -S" % pseudo_prefix
> > +                    exec_native_cmd(shutdown_cmd, native_sysroot)
> > +
> > +                    # Database is not locked anymore
> > +                    # Repairs inodes related to fstab. This way xattrs
> and metadata is correctly applied to the new file
> > +                    repair_cmd = "%s -B" % pseudo_prefix
> > +                    exec_native_cmd(repair_cmd, native_sysroot)
> > +
>
> Is it possible to describe how to reproduce and observe the issue that
> this patch aims to fix? This snippet makes me *very* nervous as pseudo
> is used by mulitple bitbake tasks in parallel, and doing shutdowns and
> repairs in the middle of it can blow up spectacularly.
>
> Alex
>
Ross Burton Oct. 23, 2025, 1:04 p.m. UTC | #3
On 20 Oct 2025, at 13:09, Alexander Kanavin via lists.openembedded.org <alex.kanavin=gmail.com@lists.openembedded.org> wrote:
> 
> Is it possible to describe how to reproduce and observe the issue that
> this patch aims to fix? This snippet makes me *very* nervous as pseudo
> is used by mulitple bitbake tasks in parallel, and doing shutdowns and
> repairs in the middle of it can blow up spectacularly.

Agreed.  If you’re building a single image then yes, the wic tasks will typically happen at the end with nothing else in parallel. But this is not a guarantee and killing pseudo whilst other recipes might be mid-build is not acceptable.

Ross
diff mbox series

Patch

diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
index bf2c34d594..82d754835c 100644
--- a/scripts/lib/wic/partition.py
+++ b/scripts/lib/wic/partition.py
@@ -131,7 +131,7 @@  class Partition():
         partition command parameters.
         """
         self.updated_fstab_path = updated_fstab_path
-        if self.updated_fstab_path and not (self.fstype.startswith("ext") or self.fstype == "msdos"):
+        if self.updated_fstab_path:
             self.update_fstab_in_rootfs = True
 
         if not self.source:
@@ -295,15 +295,6 @@  class Partition():
             (self.fstype, extraopts, rootfs, label_str, self.fsuuid, rootfs_dir)
         exec_native_cmd(mkfs_cmd, native_sysroot, pseudo=pseudo)
 
-        if self.updated_fstab_path and self.has_fstab and not self.no_fstab_update:
-            debugfs_script_path = os.path.join(cr_workdir, "debugfs_script")
-            with open(debugfs_script_path, "w") as f:
-                f.write("cd etc\n")
-                f.write("rm fstab\n")
-                f.write("write %s fstab\n" % (self.updated_fstab_path))
-            debugfs_cmd = "debugfs -w -f %s %s" % (debugfs_script_path, rootfs)
-            exec_native_cmd(debugfs_cmd, native_sysroot)
-
         mkfs_cmd = "fsck.%s -pvfD %s" % (self.fstype, rootfs)
         exec_native_cmd(mkfs_cmd, native_sysroot, pseudo=pseudo)
 
@@ -400,10 +391,6 @@  class Partition():
         mcopy_cmd = "mcopy -i %s -s %s/* ::/" % (rootfs, rootfs_dir)
         exec_native_cmd(mcopy_cmd, native_sysroot)
 
-        if self.updated_fstab_path and self.has_fstab and not self.no_fstab_update:
-            mcopy_cmd = "mcopy -m -i %s %s ::/etc/fstab" % (rootfs, self.updated_fstab_path)
-            exec_native_cmd(mcopy_cmd, native_sysroot)
-
         chmod_cmd = "chmod 644 %s" % rootfs
         exec_cmd(chmod_cmd)
 
diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py
index e29f3a4c2f..09446baef2 100644
--- a/scripts/lib/wic/plugins/source/rootfs.py
+++ b/scripts/lib/wic/plugins/source/rootfs.py
@@ -225,12 +225,21 @@  class RootfsPlugin(SourcePlugin):
                 fstab_path = os.path.join(new_rootfs, "etc/fstab")
                 # Assume that fstab should always be owned by root with fixed permissions
                 install_cmd = "install -m 0644 -p %s %s" % (part.updated_fstab_path, fstab_path)
+                exec_native_cmd(install_cmd, native_sysroot)
+ 
                 if new_pseudo:
-                    pseudo = cls.__get_pseudo(native_sysroot, new_rootfs, new_pseudo)
-                else:
-                    pseudo = None
-                exec_native_cmd(install_cmd, native_sysroot, pseudo)
-
+                    pseudo_prefix = cls.__get_pseudo(native_sysroot, new_rootfs, new_pseudo)
+                   
+                    # Shutdown any existing pseudo server
+                    # This ensures no other processes are using the database
+                    shutdown_cmd = "%s -S" % pseudo_prefix
+                    exec_native_cmd(shutdown_cmd, native_sysroot)
+                   
+                    # Database is not locked anymore
+                    # Repairs inodes related to fstab. This way xattrs and metadata is correctly applied to the new file
+                    repair_cmd = "%s -B" % pseudo_prefix
+                    exec_native_cmd(repair_cmd, native_sysroot)
+ 
         part.prepare_rootfs(cr_workdir, oe_builddir,
                             new_rootfs or part.rootfs_dir, native_sysroot,
                             pseudo_dir = new_pseudo or pseudo_dir)