Deterministic initramfs

[joachifm]

Extracted from #2281, some details in commit messages.

[mention-bot]

@joachifm, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @edolstra and @alexanderkjeldaas to be potential reviewers.

[abbradar]

Not sure what the Perl change does, so can't review it.

[abbradar]

Why is >> changed to > here? Also, why change : to . (though this shouldn't change semantics)? BTW, from cpio documentation:

              To  avoid  the lookup and ensure that arguments are treated as numeric values, prefix them with a plus
              sign, e.g.: -R +0:+0.

Can't hurt either.

[joachifm]

Thanks. I failed to notice those minute differences while squashing; I'll ensure that the squashed version is in line with the modern code.

[joachifm]

Actually the change from >> to > would break the prepend stuff. Nice catch!

[abbradar]

Seems good to me now! I also vaguely get what does the Perl code do now -- seems OK for my limited understanding.

Personally I'm glad we finally started to work on merging this big PR, at least parts -- thanks!

EDIT: ninja'd you -- your description matches mine thoughts.

[joachifm]

@abbradar as far as I can tell, the change to clean-cpio is to make explicit that entries are assigned sequential inode numbers based on input order and not taking into account hardlinking at all. If B is a hardlink to A, they'd receive different inode numbers, whereas ordinarily they'd have the same inode (I guess).

[joachifm]

Then again #2281 (comment) mentions boot problems due to inode reassignment. Hrm, I can't see any addendum to that comment (but the thread is large, so I might be missing something).
EDIT: perhaps they're referring to the changes to perl itself, makes a bit more sense :)

[joachifm]

Nevermind, I think I simply misunderstood the linked comment.

[abbradar]

This comment is bad news, either we need to ask @alexanderkjeldaas what boot problems did he have with that patch, or do extensive testing by ourselves. I'm willing to volunteer and cherry-pick it later when I have time, but maybe you need some complex initrd to bump into those problems.

From your description I'm not sure what would "having hardlinks have different inode numbers" mean in CPIO. If those were files, that would mean you have essentially made a copy (if I'm incorrect, please point that out!). Would it work that way here automatically, just by changing inode number in some map? I'm pointing to that because if there are indeed problems maybe it has something to do with them.

(to the new comment) Why do you think you have misunderstood it? It seems to mean boot problems with this patch to me.

[joachifm]

Regarding inode numbers, https://github.com/libarchive/libarchive/wiki/ManPageCpio5 says

dev, ino
The device and inode numbers from the disk. These are used by programs that read cpio archives to determine when two entries refer to the same file. Programs that synthesize cpio archives should be careful to set these to distinct values for each entry.

Thus, setting different ino would indicate that two files are distinct even if they were originally hardlinked (i.e., the same file).

It also says

Hardlinked files are not given special treatment; the full file contents are included with each copy of the file.

My reading of that is that the only difference is in the archive header, not the actual payload.

(In the description of the new ascii header format it says hardlinked files are handled by setting the filesize to 0; I assume they're referring to the filesize header field and that the actual files themselves are still included as before).

(EDIT: ofc, seeing as we're creating files using the new ascii header format, it seems like messing with inos to avoid stuff being marked as hardlinks would be the wrong way to go about it, but then I guess that's not the primary purpose of the remapping).

[joachifm]

Anyway, I think it's prudent to hold off on this until somebody gives a proper explanation.

[alexanderkjeldaas]

The > vs >> is a bug.

I might remember wrong, but I think I first tried setting ino == 0 on all files in the cleanup, but then a more conservative fix is to just remap all inodes from whatever they are in the file-system to a consecutive range starting from 1.

So the question is just whether having a slightly invalid cpio file with ino == 0 works because nothing really uses this information, or whether a mapping like what this code does, is needed.

I don't remember exactly at which point I had boot issues, and whether they were exactly related to this issue.

[alexanderkjeldaas]

@joachifm

as far as I can tell, the change to clean-cpio is to make explicit that entries are assigned sequential inode numbers based on input order and not taking into account hardlinking at all. If B is a hardlink to A, they'd receive different inode numbers, whereas ordinarily they'd have the same inode (I guess).

No, this line:
$ino_remap{$e->{inode}} = $ino++ unless exists $ino_remap{$e->{inode}};

only allocates a new inode number if none has been allocated for $e->{inode} previously. So if B and A are hardlinks, they should be hardlinks in the remapped version as well.

That's the idea, at least.

[abbradar]

@alexanderkjeldaas Thank you for your answers! Between this and re-reading the code again after a fresh sleep this seems pretty safe to me.

[joachifm]

@alexanderkjeldaas cool, but just so I can understand better: is it just about sequential numbering? I figured it had to do with hardlinking due to the mention of nlink. How does nlink figure into it?

[edolstra]

Have you tried cpio --reproducible? It might make the Perl script unnecessary. Maybe--renumber-inodes is also necessary.

[joachifm]

--reproducible alone appears insufficient; it works when combined with cpio-clean.

[joachifm]

Anyway, I've cleaned up the commits, so if we're happy with this, it should be ready to go.

[alexanderkjeldaas]

@edolstra I don't think I've seen --renumber-inodes before.

@joachifm where do you see this nlink thing? If the inode number is the same, then by definition it is a hardlink.

[joachifm]

@alexanderkjeldaas the original commit message said "Do not set nlink when cleaning cpio archives", I assumed it referred to the nlink field.

[alexanderkjeldaas]

Ok, so I probably set that as well, and I'm guessing it was unneeded (it shouldn't be needed if the `ino` renumbering deals with hardlinks). I think this might have been a squashed commit.

…

On Mon, Dec 19, 2016 at 11:31 PM, Joachim F. ***@***.***> wrote: @alexanderkjeldaas <https://github.com/alexanderkjeldaas> the original commit message said "Do not set nlink when cleaning cpio archives", I assumed it referred to the nlink field. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21273 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUtqQEKztz2HouPv90zRLvfaCPgq0bUks5rJwXbgaJpZM4LQQUM> .

[joachifm]

@alexanderkjeldaas thank you for clearing that up for me; I interpreted the message to mean that renumbering somehow indirectly affected nlink, hence my confusion :) I have amended the commit message accordingly.

[dezgeg]

I guess the remaining part besides cpio --reproducible is the mtimes. This works for me at least (and gets rid of cpio-clean.pl):

diff --git a/pkgs/build-support/kernel/make-initrd.sh b/pkgs/build-support/kernel/make-initrd.sh
index 89021ca..a9be7e5 100644
--- a/pkgs/build-support/kernel/make-initrd.sh
+++ b/pkgs/build-support/kernel/make-initrd.sh
@@ -39,7 +39,9 @@ mkdir -p $out
 for PREP in $prepend; do
   cat $PREP >> $out/initrd
 done
-(cd root && find * -print0 | cpio -o -H newc -R 0:0 --null | perl $cpioClean | $compressor >> $out/initrd)
+
+find root -print0 | xargs -0r touch --no-dereference --date="@1"
+(cd root && find * -print0 | sort -z | cpio -o -H newc -R +0:+0 --reproducible --null | $compressor >> $out/initrd)
 
 if [ -n "$makeUInitrd" ]; then
     mv $out/initrd $out/initrd.gz

Stolen from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=845034.

[joachifm]

@dezgeg cool, perhaps you'd care to commit something to that effect, then this PR could be closed in deference to it.

[joachifm]

@dezgeg I took the liberty of implementing your proposal in a different PR; I'll merge that one instead of this. Hope that's okay, if not feel free to commit your own version :)