[PATCH] pseudo: Remove mismatched inodes from the db

[PATCH] pseudo: Remove mismatched inodes from the db

[Richard Purdie]

Currently, where pseudo finds a database entry for an inode but the path
doesn't match, it reuses that database entry metadata. This is causing
real world "corruption" of file attributes.

See [YOCTO #14057] for an example of this.

This can happen when files are deleted outside of pseudo context and the
inode is reused by a new file which pseduo then "sees".

Its possible the opposite could happen, it needs to reuse attributes
but this change would prevent it. As far as I can tell, we don't want
pseuod to reuse these attributes though so this code should be safer
and avoid bugs like the above.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 .../pseudo/files/delete_mismatches.patch      | 25 +++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_git.bb    |  1 +
 2 files changed, 26 insertions(+)
 create mode 100644 meta/recipes-devtools/pseudo/files/delete_mismatches.patch

diff --git a/meta/recipes-devtools/pseudo/files/delete_mismatches.patch b/meta/recipes-devtools/pseudo/files/delete_mismatches.patch
new file mode 100644
index 00000000000..59241671767
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/files/delete_mismatches.patch
@@ -0,0 +1,25 @@
+When we see cases where the inode no longer matches the file path, pseudo notices
+but currently reuses the database entry for the file.
+
+Change this to delete the likely stale database entry instead. We're
+seeing bugs where inode reuse for deleted files causes permission corruption.
+(See bug #14057 for example).
+
+Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
+Upstream-Status: Pending
+
+Index: git/pseudo.c
+===================================================================
+--- git.orig/pseudo.c
++++ git/pseudo.c
+@@ -705,6 +705,10 @@ pseudo_op(pseudo_msg_t *msg, const char
+ 						(unsigned long long) msg_header.ino,
+ 						path_by_ino ? path_by_ino : "no path",
+ 						msg->path);
++					if (msg->nlink == 1) {
++						pdb_unlink_file_dev(msg);
++						found_ino = 0;
++					}
+ 				}
+ 			}
+ 		} else {
diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
index 3b623d8bd77..7eb72f0eab3 100644
--- a/meta/recipes-devtools/pseudo/pseudo_git.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
@@ -2,6 +2,7 @@ require pseudo.inc
 
 SRC_URI = "git://git.yoctoproject.org/pseudo;branch=oe-core \
            file://0001-configure-Prune-PIE-flags.patch \
+           file://delete_mismatches.patch \
            file://fallback-passwd \
            file://fallback-group \
            "
-- 
2.25.1

Re: [OE-core] [PATCH] pseudo: Remove mismatched inodes from the db

[Seebs]

On Tue, 22 Sep 2020 14:18:24 +0100
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> This can happen when files are deleted outside of pseudo context and
> the inode is reused by a new file which pseduo then "sees".

In terms of the original design: This would be considered a usage bug,
and pseudo issues diagnostics for that. Once files are owned by an
instance of pseudo, it expects to "see" every change to those files,
and if you don't do that, then yes, the database is corrupt. But in
some cases, the behavior was something like "a move within a pseudo
filesystem" or something similar, and losing the data would also be
bad.

Basically, the original design philosophy is that you should *never*
be modifying things pseudo owns outside of pseudo. And that does impose
a performance cost on things, *but*, it also gives us a lot of
confidence in results.

So my preference on "deleting files outside of pseudo context, inode
gets reused" would be "don't do that then"; this is why pseudo reports
diagnostics for those. Being able to tell you what the old path was
in such cases was actually one of the primary reasons pseudo exists.

-s

Re: [OE-core] [PATCH] pseudo: Remove mismatched inodes from the db

[Richard Purdie]

On Tue, 2020-09-22 at 11:06 -0500, Seebs wrote:
> On Tue, 22 Sep 2020 14:18:24 +0100
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
> > This can happen when files are deleted outside of pseudo context and
> > the inode is reused by a new file which pseduo then "sees".
> 
> In terms of the original design: This would be considered a usage bug,
> and pseudo issues diagnostics for that. Once files are owned by an
> instance of pseudo, it expects to "see" every change to those files,
> and if you don't do that, then yes, the database is corrupt. But in
> some cases, the behavior was something like "a move within a pseudo
> filesystem" or something similar, and losing the data would also be
> bad.
> 
> Basically, the original design philosophy is that you should *never*
> be modifying things pseudo owns outside of pseudo. And that does impose
> a performance cost on things, *but*, it also gives us a lot of
> confidence in results.

The "fun" here is that "make install" is rewriting Makefiles within the
source code tree. The install step runs under pseudo, earlier ones do
not.

If we rerun earlier non-pseudo steps, the Makefiles get reset to their
original state, then "make install" modifies them again.

We don't run unpack/configure/compile under pseudo for performance
reasons, these steps don't do anything we care about from a pseudo
perspective.

> So my preference on "deleting files outside of pseudo context, inode
> gets reused" would be "don't do that then"; this is why pseudo reports
> diagnostics for those. Being able to tell you what the old path was
> in such cases was actually one of the primary reasons pseudo exists.

We're not intentionally doing it. Perl's makefiles are doing crazy
things. I'm not sure we can:

a) afford to run everything under pseudo

or

b) catch every case of code which modifies files in unexpected places


Part of the trouble here is that we can't tell pseudo "only monitor
files <here>". Realistically, it can't expect to monitor every file on
the system either.

We are breaking pseudo's original design, but in general it seems to
work out fine (and we've been doing pretty this for many years now). We
may need to accept we're not operating entirely as designed and
consider if we could/should be changing some of the responses to these
corner cases accordingly as I don't think we can realistically avoid
them?

Not sure where this leaves us :(

Cheers,

Richard

Re: [OE-core] [PATCH] pseudo: Remove mismatched inodes from the db

[Seebs]

On Tue, 22 Sep 2020 18:22:18 +0100
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> The "fun" here is that "make install" is rewriting Makefiles within
> the source code tree. The install step runs under pseudo, earlier
> ones do not.

> If we rerun earlier non-pseudo steps, the Makefiles get reset to their
> original state, then "make install" modifies them again.

ugh.

Okay, so. A vague thought about how to perhaps approach this: A
post-install step, which basically does:
	pseudo cp Makefile Makefile.pseudo
	pseudo rm Makefile
	[not-pseudo] cp Makefile.pseudo Makefile
	pseudo rm Makefile.pseudo

So that, at the end of the post-install, the files pseudo doesn't want
to know about are forgotten. Possibly there ought to be a "forget about
this directory" option/command.

-s

Re: [OE-core] [PATCH] pseudo: Remove mismatched inodes from the db

[Richard Purdie]

On Tue, 2020-09-22 at 12:52 -0500, Seebs wrote:
> On Tue, 22 Sep 2020 18:22:18 +0100
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
> > The "fun" here is that "make install" is rewriting Makefiles within
> > the source code tree. The install step runs under pseudo, earlier
> > ones do not.
> > If we rerun earlier non-pseudo steps, the Makefiles get reset to
> > their
> > original state, then "make install" modifies them again.
> 
> ugh.
> 
> Okay, so. A vague thought about how to perhaps approach this: A
> post-install step, which basically does:
> 	pseudo cp Makefile Makefile.pseudo
> 	pseudo rm Makefile
> 	[not-pseudo] cp Makefile.pseudo Makefile
> 	pseudo rm Makefile.pseudo
>
> So that, at the end of the post-install, the files pseudo doesn't
> want
> to know about are forgotten. Possibly there ought to be a "forget
> about
> this directory" option/command.

We're not going to know the exact files unfortunately so it would
likely need to be an "all paths starting with XXX, YYY" type command.

A quick scan of my TMPDIR shows 17,000 of these path mismatches and I'm
not sure there is a single path in there we care about, it does mean a
lot of permissions are being mangled though :(

Cheers,

Richard