[Buildroot] [PATCH 0/6 v2] fs: fix and better handle capabilities

[Buildroot] [PATCH 0/6 v2] fs: fix and better handle capabilities

[Yann E. MORIN]

Hello All!

As reported by Ricardo in #11216, and recently noticed thanks to the
runtime tests he added, handling file capabilities is broken.

Ricardo did a very good job at pinpointing the issue, and that is caused
by the recent-ish split of the filesystem infra with the use of the
intermediate tarball.

It turns out that playing with fakeroot, tar, and capabilities is a lost
game, as fakeroot behaves badly with the special handling tar does with
the security.capability extended attribute.

This series fixes the issue with the first patch, in which we simply get
rid of the intermediate tarball, and directly rsync from the global
target/ directory to populate the per-filesystem target directory. This
means that any common fincalisation under fakaeroot is no longer
possible, so they have to be replicated for each filesystem instead.

The only remaining common part is now the generation of the various
tables: devices, permisions and users.

Then, the tar filesystem is fixed to store the xattrs.

Then there are tow clean-up patches, which pave the way to two fixes.

First, we allow permissions provided by users to override the internal
permissions, as we do for all user-provided settings. Second, we fix the
static device creation conditions.


Regards,
Yann E. MORIN.


The following changes since commit cd0ca09e43be8bd87fa35c96fa099a338b85de36

  {linux, linux-headers}: bump 4.{4, 9, 14, 18}.x series (2018-11-11 22:11:04 +0100)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to ae80646fbb9d0e7f40434b16ebe5173ee3a4654c

  fs: make static device nodes creation more logical (2018-11-12 18:28:28 +0100)


----------------------------------------------------------------
Yann E. MORIN (6):
      fs: get over the intermediate tarball
      fs/tar: add support for xattrs (thus capabilties)
      fs: rename internal variable
      fs: split devices and permissions tables
      fs: allow user provided permissions to override packages permissions
      fs: make static device nodes creation more logical

 fs/common.mk                                    | 92 +++++++++++--------------
 fs/tar/tar.mk                                   |  2 +-
 support/testing/tests/core/test_post_scripts.py |  4 +-
 3 files changed, 44 insertions(+), 54 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/6 v2] fs: get over the intermediate tarball

[Yann E. MORIN]

Since 118534fe54b (fs: use a common tarball as base for the other
filesystems), the filesystem creation is split in two steps, using an
intermediate tarball to carry the generic, common finalisations to the
per-filesystem finalisation and image creation.

However, this intermediate tarball causes an issue with capabilities,
which are entirely missing in the generated filesystems.

Capabilties are stored in the extended attribute security.capabilti,
which tar by default will not store/restore, unless explicitly told to,
e.g. with --xattrs-include='*', which we don't pass.

Now, passing this option when creating and extracting the intermediate
tarball, both done under fakeroot, will cause fakeroot to report an
invalid filetype for files with capabilities. mksquashfs would report
such unknown files as a warning, while mkfs.ext2 would fail (with a
similar error message), e.g.:

    File [...]/usr/sbin/getcap has unrecognised filetype 0, ignoring

This is due to a poor interaction between tar and fakeroot; running as
root the exact same commands we run under fakeroot, works as expected.
Unfortunately, short of fixing fakeroot (which would first require
understanding the problem in there), we don't have much options.

The intermediate tarball was made to avoid redoing the same actions over
and over again for each filesystem to build. However, most of the time,
only one or two such filesystems would be enabled [0], and those actions
are usually pretty lightweight. So, using an intermediate tarball does
not provide a big optimisation.

What is interesting in the intermediate tarball, however, is that it
allowed to postpone per-filesystem finalisations to be applied only for
the corresponding filesystem, not for all of them.

So, we get rid of the intermediate tarball, and simply move all of the
code to run under fakeroot to the per-filesystem fakeroot script.
Instead of extracting the intermediate tarball, we just rsync the
original target/ directory, and apply the filesystem finalisations on
that copy.

Fixes: #11216

Note: an alternate solution would have been to keep the intermediate
tarball to keep most of the common finalisations, and move only the
permissions to each filesystem, but that was getting a bit more complex
and changed the ordering of permissions and post-fakeroot scripts. Once
we bite the bullet of having some common finalisation done in each
filesystem, so move all of them.

[0] Most probsably, users would enable the real filesystem to put on
their device, plus the 'tar' filesystem, to be able to easily inspect
the content on their development machine.

Reported-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Peter Korsgaard <peter@korsgaard.com>

---
Changes v1 -> v2:
  - move all the code to the per-fs steps, drop the intermediate tarball
---
 fs/common.mk                                    | 59 ++++++++-----------------
 support/testing/tests/core/test_post_scripts.py |  4 +-
 2 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index 2a5a202a89..358801d44f 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -40,48 +40,21 @@ define ROOTFS_REPRODUCIBLE
 endef
 endif
 
-ROOTFS_COMMON_TAR = $(FS_DIR)/rootfs.common.tar
-
-# Command to create the common tarball from the base target directory.
-define ROOTFS_COMMON_TAR_CMD
-	tar cf $(ROOTFS_COMMON_TAR) --numeric-owner \
-		--exclude=$(notdir $(TARGET_DIR_WARNING_FILE)) \
-		-C $(TARGET_DIR) .
-endef
-
-# Command to extract the common tarball into the per-rootfs target directory
-define ROOTFS_COMMON_UNTAR_CMD
-	mkdir -p $(TARGET_DIR)
-	tar xf $(ROOTFS_COMMON_TAR) -C $(TARGET_DIR)
-endef
-
-.PHONY: rootfs-common
-rootfs-common: $(ROOTFS_COMMON_TAR)
-
-# Emulate being in a filesystem, so that we can have our own TARGET_DIR.
-ROOTFS_COMMON_TARGET_DIR = $(FS_DIR)/target
-
 ROOTFS_COMMON_DEPENDENCIES = \
 	host-fakeroot host-makedevs \
 	$(BR2_TAR_HOST_DEPENDENCY) \
 	$(if $(PACKAGES_USERS)$(ROOTFS_USERS_TABLES),host-mkpasswd)
 
-$(ROOTFS_COMMON_TAR): ROOTFS=COMMON
-$(ROOTFS_COMMON_TAR): FAKEROOT_SCRIPT=$(FS_DIR)/fakeroot.fs
-$(ROOTFS_COMMON_TAR): $(ROOTFS_COMMON_DEPENDENCIES) target-finalize
-	@$(call MESSAGE,"Generating common rootfs tarball")
+.PHONY: rootfs-common
+rootfs-common: $(ROOTFS_COMMON_DEPENDENCIES) target-finalize
+	@$(call MESSAGE,"Generating root filesystems common tables")
 	rm -rf $(FS_DIR)
 	mkdir -p $(FS_DIR)
-	rsync -auH $(BASE_TARGET_DIR)/ $(TARGET_DIR)
-	echo '#!/bin/sh' > $(FAKEROOT_SCRIPT)
-	echo "set -e" >> $(FAKEROOT_SCRIPT)
-	echo "chown -h -R 0:0 $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
 
 	$(call PRINTF,$(PACKAGES_USERS)) >> $(USERS_TABLE)
 ifneq ($(ROOTFS_USERS_TABLES),)
 	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
 endif
-	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
 ifneq ($(ROOTFS_DEVICE_TABLES),)
 	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
 ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
@@ -89,16 +62,6 @@ ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
 endif
 endif
 	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
-	echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
-	$(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
-		echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
-		echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
-	$(foreach hook,$(ROOTFS_PRE_CMD_HOOKS),\
-		$(call PRINTF,$($(hook))) >> $(FAKEROOT_SCRIPT)$(sep))
-	$(call PRINTF,$(ROOTFS_COMMON_TAR_CMD)) >> $(FAKEROOT_SCRIPT)
-	chmod a+x $(FAKEROOT_SCRIPT)
-	PATH=$(BR_PATH) $(HOST_DIR)/bin/fakeroot -- $(FAKEROOT_SCRIPT)
-	$(Q)rm -rf $(TARGET_DIR)
 
 rootfs-common-show-depends:
 	@echo $(ROOTFS_COMMON_DEPENDENCIES)
@@ -147,9 +110,23 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
+	rsync -auH \
+		--exclude=/$$(notdir $$(TARGET_DIR_WARNING_FILE)) \
+		$$(BASE_TARGET_DIR)/ \
+		$$(TARGET_DIR)
+
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
 	echo "set -e" >> $$(FAKEROOT_SCRIPT)
-	$$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
+
+	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
+	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
+	echo "$$(HOST_DIR)/bin/makedevs -d $$(FULL_DEVICE_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
+	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
+		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
+		echo $$(EXTRA_ENV) $$(s) $$(TARGET_DIR) $$(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $$(FAKEROOT_SCRIPT)$$(sep))
+	$$(foreach hook,$$(ROOTFS_PRE_CMD_HOOKS),\
+		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT)$$(sep))
+
 	$$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),\
 		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT)$$(sep))
 	$$(call PRINTF,$$(ROOTFS_REPRODUCIBLE)) >> $$(FAKEROOT_SCRIPT)
diff --git a/support/testing/tests/core/test_post_scripts.py b/support/testing/tests/core/test_post_scripts.py
index a0e5b6b454..40a36b7904 100644
--- a/support/testing/tests/core/test_post_scripts.py
+++ b/support/testing/tests/core/test_post_scripts.py
@@ -41,8 +41,8 @@ class TestPostScripts(infra.basetest.BRTest):
                                  os.path.join(self.builddir, "target"),
                                  os.path.join(self.builddir, "target"))
         self.check_post_log_file("post-fakeroot.log",
-                                 os.path.join(self.builddir, "build/buildroot-fs/target"),
-                                 os.path.join(self.builddir, "build/buildroot-fs/target"))
+                                 os.path.join(self.builddir, "build/buildroot-fs/tar/target"),
+                                 os.path.join(self.builddir, "build/buildroot-fs/tar/target"))
         self.check_post_log_file("post-image.log",
                                  os.path.join(self.builddir, "images"),
                                  os.path.join(self.builddir, "target"))
-- 
2.14.1

[Buildroot] [PATCH 1/6 v2] fs: get over the intermediate tarball

[Arnout Vandecappelle]

On 12/11/2018 18:33, Yann E. MORIN wrote:
> Since 118534fe54b (fs: use a common tarball as base for the other
> filesystems), the filesystem creation is split in two steps, using an
> intermediate tarball to carry the generic, common finalisations to the
> per-filesystem finalisation and image creation.
> 
> However, this intermediate tarball causes an issue with capabilities,
> which are entirely missing in the generated filesystems.
> 
> Capabilties are stored in the extended attribute security.capabilti,
> which tar by default will not store/restore, unless explicitly told to,
> e.g. with --xattrs-include='*', which we don't pass.
> 
> Now, passing this option when creating and extracting the intermediate
> tarball, both done under fakeroot, will cause fakeroot to report an
> invalid filetype for files with capabilities. mksquashfs would report
> such unknown files as a warning, while mkfs.ext2 would fail (with a
> similar error message), e.g.:
> 
>     File [...]/usr/sbin/getcap has unrecognised filetype 0, ignoring
> 
> This is due to a poor interaction between tar and fakeroot; running as
> root the exact same commands we run under fakeroot, works as expected.
> Unfortunately, short of fixing fakeroot (which would first require
> understanding the problem in there), we don't have much options.
> 
> The intermediate tarball was made to avoid redoing the same actions over
> and over again for each filesystem to build. However, most of the time,
> only one or two such filesystems would be enabled [0], and those actions
> are usually pretty lightweight. So, using an intermediate tarball does
> not provide a big optimisation.
> 
> What is interesting in the intermediate tarball, however, is that it
> allowed to postpone per-filesystem finalisations to be applied only for
> the corresponding filesystem, not for all of them.
> 
> So, we get rid of the intermediate tarball, and simply move all of the
> code to run under fakeroot to the per-filesystem fakeroot script.
> Instead of extracting the intermediate tarball, we just rsync the
> original target/ directory, and apply the filesystem finalisations on
> that copy.
> 
> Fixes: #11216

 A full URL is more convenient to find it back. So I changed that, as well as
some spelling fixes, and applied to master, thanks.

 Regards,
 Arnout

> 
> Note: an alternate solution would have been to keep the intermediate
> tarball to keep most of the common finalisations, and move only the
> permissions to each filesystem, but that was getting a bit more complex
> and changed the ordering of permissions and post-fakeroot scripts. Once
> we bite the bullet of having some common finalisation done in each
> filesystem, so move all of them.
> 
> [0] Most probsably, users would enable the real filesystem to put on
> their device, plus the 'tar' filesystem, to be able to easily inspect
> the content on their development machine.
> 
> Reported-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> diff --git a/fs/common.mk b/fs/common.mk

[Buildroot] [PATCH 2/6 v2] fs/tar: add support for xattrs (thus capabilties)

[Yann E. MORIN]

By default, tar will not include any extended attribute (xattr) when
creating archives, and thus will not store capabilties either (as they
are stored in the xattr 'security.capability').

Using option --xattrs is enough to create a tarball with all the xattrs
attached to a file. However, extracting all xattrs from a tarball
requires that --xattrs-include='*' be used. This is not symetric (but on
purpose, as per the documentation), and so is confusing to some.

So, we use --xattrs-include='*' to create the archive, so as to be
explicit that we want all xattrs to be stored.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 fs/tar/tar.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/tar/tar.mk b/fs/tar/tar.mk
index 68149e9eb7..4c6327ace8 100644
--- a/fs/tar/tar.mk
+++ b/fs/tar/tar.mk
@@ -10,7 +10,7 @@ ROOTFS_TAR_DEPENDENCIES = $(BR2_TAR_HOST_DEPENDENCY)
 
 define ROOTFS_TAR_CMD
 	(cd $(TARGET_DIR); find -print0 | LC_ALL=C sort -z | \
-		tar $(TAR_OPTS) -cf $@ --null --no-recursion -T - --numeric-owner)
+		tar $(TAR_OPTS) -cf $@ --null --xattrs-include='*' --no-recursion -T - --numeric-owner)
 endef
 
 $(eval $(rootfs))
-- 
2.14.1

[Buildroot] [PATCH 2/6 v2] fs/tar: add support for xattrs (thus capabilties)

[Arnout Vandecappelle]

On 12/11/2018 18:33, Yann E. MORIN wrote:
> By default, tar will not include any extended attribute (xattr) when
> creating archives, and thus will not store capabilties either (as they
> are stored in the xattr 'security.capability').

 So... currently capabilities are not included at all in the tarball, since
--xattrs is not passed at the moment?

 But after this patch, they are still not included? Or does --xattrs-include
imply --xattrs?

> 
> Using option --xattrs is enough to create a tarball with all the xattrs
> attached to a file. However, extracting all xattrs from a tarball
> requires that --xattrs-include='*' be used. This is not symetric (but on
> purpose, as per the documentation), and so is confusing to some.
> 
> So, we use --xattrs-include='*' to create the archive, so as to be
> explicit that we want all xattrs to be stored.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> ---
>  fs/tar/tar.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/tar/tar.mk b/fs/tar/tar.mk
> index 68149e9eb7..4c6327ace8 100644
> --- a/fs/tar/tar.mk
> +++ b/fs/tar/tar.mk
> @@ -10,7 +10,7 @@ ROOTFS_TAR_DEPENDENCIES = $(BR2_TAR_HOST_DEPENDENCY)
>  
>  define ROOTFS_TAR_CMD
>  	(cd $(TARGET_DIR); find -print0 | LC_ALL=C sort -z | \
> -		tar $(TAR_OPTS) -cf $@ --null --no-recursion -T - --numeric-owner)
> +		tar $(TAR_OPTS) -cf $@ --null --xattrs-include='*' --no-recursion -T - --numeric-owner)

 I think --xattrs-include should come before TAR_OPTS, so it can still be
overridden. Ah, darn, never mind, it can't be overridden, several
--xattrs-include options are cumulative and their order doesn't matter.

 Regards,
 Arnout

>  endef
>  
>  $(eval $(rootfs))
> 

[Buildroot] [PATCH 2/6 v2] fs/tar: add support for xattrs (thus capabilties)

[Arnout Vandecappelle]

On 12/11/2018 23:09, Arnout Vandecappelle wrote:
>
> On 12/11/2018 18:33, Yann E. MORIN wrote:
>> By default, tar will not include any extended attribute (xattr) when
>> creating archives, and thus will not store capabilties either (as they
>> are stored in the xattr 'security.capability').
>  So... currently capabilities are not included at all in the tarball, since
> --xattrs is not passed at the moment?
>
>  But after this patch, they are still not included? Or does --xattrs-include
> imply --xattrs?

?Tested and confirmed: --xattrs-include implies --xattrs. At least in my tar
1.29. So

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>


?Regards,
?Arnout


>
>> Using option --xattrs is enough to create a tarball with all the xattrs
>> attached to a file. However, extracting all xattrs from a tarball
>> requires that --xattrs-include='*' be used. This is not symetric (but on
>> purpose, as per the documentation), and so is confusing to some.
>>
>> So, we use --xattrs-include='*' to create the archive, so as to be
>> explicit that we want all xattrs to be stored.
>>
>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
>> Cc: Peter Korsgaard <peter@korsgaard.com>
>> Cc: Arnout Vandecappelle <arnout@mind.be>
>> ---
>>  fs/tar/tar.mk | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/tar/tar.mk b/fs/tar/tar.mk
>> index 68149e9eb7..4c6327ace8 100644
>> --- a/fs/tar/tar.mk
>> +++ b/fs/tar/tar.mk
>> @@ -10,7 +10,7 @@ ROOTFS_TAR_DEPENDENCIES = $(BR2_TAR_HOST_DEPENDENCY)
>>  
>>  define ROOTFS_TAR_CMD
>>  	(cd $(TARGET_DIR); find -print0 | LC_ALL=C sort -z | \
>> -		tar $(TAR_OPTS) -cf $@ --null --no-recursion -T - --numeric-owner)
>> +		tar $(TAR_OPTS) -cf $@ --null --xattrs-include='*' --no-recursion -T - --numeric-owner)
>  I think --xattrs-include should come before TAR_OPTS, so it can still be
> overridden. Ah, darn, never mind, it can't be overridden, several
> --xattrs-include options are cumulative and their order doesn't matter.
>
>  Regards,
>  Arnout
>
>>  endef
>>  
>>  $(eval $(rootfs))
>>

[Buildroot] [PATCH 2/6 v2] fs/tar: add support for xattrs (thus capabilties)

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > By default, tar will not include any extended attribute (xattr) when
 > creating archives, and thus will not store capabilties either (as they
 > are stored in the xattr 'security.capability').

 > Using option --xattrs is enough to create a tarball with all the xattrs
 > attached to a file. However, extracting all xattrs from a tarball
 > requires that --xattrs-include='*' be used. This is not symetric (but on
 > purpose, as per the documentation), and so is confusing to some.

 > So, we use --xattrs-include='*' to create the archive, so as to be
 > explicit that we want all xattrs to be stored.

 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > Cc: Peter Korsgaard <peter@korsgaard.com>
 > Cc: Arnout Vandecappelle <arnout@mind.be>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 3/6 v2] fs: rename internal variable

[Yann E. MORIN]

In preparation of more renames, rename the variable that points to the
final users table.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 fs/common.mk | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index 358801d44f..d1b6a56913 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -31,9 +31,11 @@ FS_DIR = $(BUILD_DIR)/buildroot-fs
 FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
 ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
 	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
-USERS_TABLE = $(FS_DIR)/users_table.txt
+
 ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
 
+ROOTFS_FINAL_USERS_TABLE = $(FS_DIR)/full_users_table.txt
+
 ifeq ($(BR2_REPRODUCIBLE),y)
 define ROOTFS_REPRODUCIBLE
 	find $(TARGET_DIR) -print0 | xargs -0 -r touch -hd @$(SOURCE_DATE_EPOCH)
@@ -51,9 +53,9 @@ rootfs-common: $(ROOTFS_COMMON_DEPENDENCIES) target-finalize
 	rm -rf $(FS_DIR)
 	mkdir -p $(FS_DIR)
 
-	$(call PRINTF,$(PACKAGES_USERS)) >> $(USERS_TABLE)
+	$(call PRINTF,$(PACKAGES_USERS)) >> $(ROOTFS_FINAL_USERS_TABLE)
 ifneq ($(ROOTFS_USERS_TABLES),)
-	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
+	cat $(ROOTFS_USERS_TABLES) >> $(ROOTFS_FINAL_USERS_TABLE)
 endif
 ifneq ($(ROOTFS_DEVICE_TABLES),)
 	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
@@ -119,7 +121,7 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 	echo "set -e" >> $$(FAKEROOT_SCRIPT)
 
 	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
-	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
+	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(ROOTFS_FINAL_USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
 	echo "$$(HOST_DIR)/bin/makedevs -d $$(FULL_DEVICE_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
 	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
 		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
-- 
2.14.1

[Buildroot] [PATCH 3/6 v2] fs: rename internal variable

[Arnout Vandecappelle]

On 12/11/2018 18:33, Yann E. MORIN wrote:
> In preparation of more renames, rename the variable that points to the
> final users table.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  fs/common.mk | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 358801d44f..d1b6a56913 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -31,9 +31,11 @@ FS_DIR = $(BUILD_DIR)/buildroot-fs
>  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
>  ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
>  	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> -USERS_TABLE = $(FS_DIR)/users_table.txt
> +
>  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
>  
> +ROOTFS_FINAL_USERS_TABLE = $(FS_DIR)/full_users_table.txt

 Nitpicking: make sure the file name is the same as the variable name. Either
name is OK for me, with a light preference for full rather than final.

 With that:
  Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout

> +
>  ifeq ($(BR2_REPRODUCIBLE),y)
>  define ROOTFS_REPRODUCIBLE
>  	find $(TARGET_DIR) -print0 | xargs -0 -r touch -hd @$(SOURCE_DATE_EPOCH)
> @@ -51,9 +53,9 @@ rootfs-common: $(ROOTFS_COMMON_DEPENDENCIES) target-finalize
>  	rm -rf $(FS_DIR)
>  	mkdir -p $(FS_DIR)
>  
> -	$(call PRINTF,$(PACKAGES_USERS)) >> $(USERS_TABLE)
> +	$(call PRINTF,$(PACKAGES_USERS)) >> $(ROOTFS_FINAL_USERS_TABLE)
>  ifneq ($(ROOTFS_USERS_TABLES),)
> -	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
> +	cat $(ROOTFS_USERS_TABLES) >> $(ROOTFS_FINAL_USERS_TABLE)
>  endif
>  ifneq ($(ROOTFS_DEVICE_TABLES),)
>  	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> @@ -119,7 +121,7 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
>  	echo "set -e" >> $$(FAKEROOT_SCRIPT)
>  
>  	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
> -	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
> +	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(ROOTFS_FINAL_USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
>  	echo "$$(HOST_DIR)/bin/makedevs -d $$(FULL_DEVICE_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
>  	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
>  		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
> 

[Buildroot] [PATCH 3/6 v2] fs: rename internal variable

[Thomas Petazzoni]

Hello,

On Mon, 12 Nov 2018 18:33:13 +0100, Yann E. MORIN wrote:
> In preparation of more renames, rename the variable that points to the
> final users table.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  fs/common.mk | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Applied to master after changing the name to ROOTFS_FULL_USERS_TABLE as
suggested by Arnout.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 4/6 v2] fs: split devices and permissions tables

[Yann E. MORIN]

Currently, we conflate device tables and permissions tables, on the
premise they are both created and applied with the same tool, makedevs.

Split the two, in to their own final aggregated tables and their own
call to makedevs.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 fs/common.mk | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index d1b6a56913..e406554914 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -28,13 +28,16 @@
 # macro will automatically generate a compressed filesystem image.
 
 FS_DIR = $(BUILD_DIR)/buildroot-fs
-FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
-ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
-	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
 
+# BR2_ROOTFS_STATIC_DEVICE_TABLE is really for /dev nodes
+# BR2_ROOTFS_DEVICE_TABLE is only for permissions (legacy name...)
 ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
+ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
+ROOTFS_PERMISSIONS_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
 
 ROOTFS_FINAL_USERS_TABLE = $(FS_DIR)/full_users_table.txt
+ROOTFS_FINAL_STATIC_DEVICES_TABLE = $(FS_DIR)/full_static_devices_table.txt
+ROOTFS_FINAL_PERMISSIONS_TABLE = $(FS_DIR)/full_permissions_table.txt
 
 ifeq ($(BR2_REPRODUCIBLE),y)
 define ROOTFS_REPRODUCIBLE
@@ -57,13 +60,20 @@ rootfs-common: $(ROOTFS_COMMON_DEPENDENCIES) target-finalize
 ifneq ($(ROOTFS_USERS_TABLES),)
 	cat $(ROOTFS_USERS_TABLES) >> $(ROOTFS_FINAL_USERS_TABLE)
 endif
-ifneq ($(ROOTFS_DEVICE_TABLES),)
-	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
+
+	$(Q)touch $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
+ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
+	cat $(ROOTFS_STATIC_DEVICE_TABLES) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
 ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
-	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
+	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
 endif
 endif
-	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
+
+ifneq ($(ROOTFS_PERMISSIONS_TABLES),)
+	cat $(ROOTFS_PERMISSIONS_TABLES) >>$(ROOTFS_FINAL_PERMISSIONS_TABLE)
+endif
+	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(ROOTFS_FINAL_PERMISSIONS_TABLE)
+
 
 rootfs-common-show-depends:
 	@echo $(ROOTFS_COMMON_DEPENDENCIES)
@@ -122,7 +132,8 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 
 	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
 	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(ROOTFS_FINAL_USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
-	echo "$$(HOST_DIR)/bin/makedevs -d $$(FULL_DEVICE_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
+	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_FINAL_STATIC_DEVICES_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
+	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_FINAL_PERMISSIONS_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
 	$$(foreach s,$$(call qstrip,$$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
 		echo "echo '$$(TERM_BOLD)>>>   Executing fakeroot script $$(s)$$(TERM_RESET)'" >> $$(FAKEROOT_SCRIPT); \
 		echo $$(EXTRA_ENV) $$(s) $$(TARGET_DIR) $$(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $$(FAKEROOT_SCRIPT)$$(sep))
-- 
2.14.1

[Buildroot] [PATCH 4/6 v2] fs: split devices and permissions tables

[Arnout Vandecappelle]

On 12/11/2018 18:33, Yann E. MORIN wrote:
> Currently, we conflate device tables and permissions tables, on the
> premise they are both created and applied with the same tool, makedevs.
> 
> Split the two, in to their own final aggregated tables and their own
> call to makedevs.

 Sorry, I don't think there is a good reason to split them up. I don't think
this enables any other improvements either - the following two patches don't
depend on this, right? I also don't feel that this change makes the code easier
to understand (not more difficult either, but if it ain't broken, don't fix it).

 So nack from me.

 Regards,
 Arnout

> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> ---
>  fs/common.mk | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
[snip]

[Buildroot] [PATCH 5/6 v2] fs: allow user provided permissions to override packages permissions

[Yann E. MORIN]

Reported-by: Matthew Weber <matthew.weber@rockwellcollins.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 fs/common.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/common.mk b/fs/common.mk
index e406554914..d50698078b 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -69,10 +69,10 @@ ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
 endif
 endif
 
+	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(ROOTFS_FINAL_PERMISSIONS_TABLE)
 ifneq ($(ROOTFS_PERMISSIONS_TABLES),)
 	cat $(ROOTFS_PERMISSIONS_TABLES) >>$(ROOTFS_FINAL_PERMISSIONS_TABLE)
 endif
-	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(ROOTFS_FINAL_PERMISSIONS_TABLE)
 
 
 rootfs-common-show-depends:
-- 
2.14.1

[Buildroot] [PATCH 6/6 v2] fs: make static device nodes creation more logical

[Yann E. MORIN]

Currently, the creation of the static device nodes is under a weird (but
correct) condition: it depends on whether a statice device table is
defined or not. Since that option already depend on static /dev
management, that is coorrect, even if not totally obvious at first sight.

Yet, the creation of the per-package static devices is conditional to
using a static /dev management, which is already implied by the
enclosing condition, as explained above.

However, there is still a gotcha: if doing static /dev management, but
using no static device table [0], then the per-package static devices
are not created.

Fix that by inverting the logic, with the outter condition being on
using a static /dev management, conditionng per-package devices to that,
and with the inner condition on static device tables only conditioning
the use of said device tables.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 fs/common.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index d50698078b..ad9217bc78 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -62,10 +62,10 @@ ifneq ($(ROOTFS_USERS_TABLES),)
 endif
 
 	$(Q)touch $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
-ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
-	cat $(ROOTFS_STATIC_DEVICE_TABLES) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
 ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
 	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
+ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
+	cat $(ROOTFS_STATIC_DEVICE_TABLES) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
 endif
 endif
 
-- 
2.14.1

[Buildroot] [PATCH 6/6 v2] fs: make static device nodes creation more logical

[Thomas Petazzoni]

Hello,

On Mon, 12 Nov 2018 18:33:16 +0100, Yann E. MORIN wrote:

>  	$(Q)touch $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
> -ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> -	cat $(ROOTFS_STATIC_DEVICE_TABLES) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
>  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
>  	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
> +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> +	cat $(ROOTFS_STATIC_DEVICE_TABLES) >> $(ROOTFS_FINAL_STATIC_DEVICES_TABLE)
>  endif
>  endif

Ah, I thought this change was not correct. But in fact, it was correct
in your series (sorry!). It was correct because you did split the
permission table from the static device table, which was done in the
patch that Arnout didn't like.

In fact, there is a way to do the same without the split of
permission/device table. Let me propose something.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 0/6 v2] fs: fix and better handle capabilities

[Arnout Vandecappelle]

On 12/11/2018 18:32, Yann E. MORIN wrote:
> Hello All!
> 
> As reported by Ricardo in #11216, and recently noticed thanks to the
> runtime tests he added, handling file capabilities is broken.
> 
> Ricardo did a very good job at pinpointing the issue, and that is caused
> by the recent-ish split of the filesystem infra with the use of the
> intermediate tarball.
> 
> It turns out that playing with fakeroot, tar, and capabilities is a lost
> game, as fakeroot behaves badly with the special handling tar does with
> the security.capability extended attribute.
> 
> This series fixes the issue with the first patch, in which we simply get
> rid of the intermediate tarball, and directly rsync from the global
> target/ directory to populate the per-filesystem target directory. This
> means that any common fincalisation under fakaeroot is no longer
> possible, so they have to be replicated for each filesystem instead.
> 
> The only remaining common part is now the generation of the various
> tables: devices, permisions and users.

 Just to be clear: this is the only patch for master, right? The rest is for next?

> 
> Then, the tar filesystem is fixed to store the xattrs.

 Like this one, tar has never contained xattrs, right?

 Regards,
 Arnout

> 
> Then there are tow clean-up patches, which pave the way to two fixes.
> 
> First, we allow permissions provided by users to override the internal
> permissions, as we do for all user-provided settings. Second, we fix the
> static device creation conditions.
> 
> 
> Regards,
> Yann E. MORIN.
> 
> 
> The following changes since commit cd0ca09e43be8bd87fa35c96fa099a338b85de36
> 
>   {linux, linux-headers}: bump 4.{4, 9, 14, 18}.x series (2018-11-11 22:11:04 +0100)
> 
> 
> are available in the git repository at:
> 
>   git://git.buildroot.org/~ymorin/git/buildroot.git
> 
> for you to fetch changes up to ae80646fbb9d0e7f40434b16ebe5173ee3a4654c
> 
>   fs: make static device nodes creation more logical (2018-11-12 18:28:28 +0100)
> 
> 
> ----------------------------------------------------------------
> Yann E. MORIN (6):
>       fs: get over the intermediate tarball
>       fs/tar: add support for xattrs (thus capabilties)
>       fs: rename internal variable
>       fs: split devices and permissions tables
>       fs: allow user provided permissions to override packages permissions
>       fs: make static device nodes creation more logical
> 
>  fs/common.mk                                    | 92 +++++++++++--------------
>  fs/tar/tar.mk                                   |  2 +-
>  support/testing/tests/core/test_post_scripts.py |  4 +-
>  3 files changed, 44 insertions(+), 54 deletions(-)
>