Message ID | 4f9a31b0fb8f87ded9c788cb92d727b6763859ac.1540626349.git.yann.morin.1998@free.fr |
---|---|
State | Changes Requested |
Headers | show |
Series | [1/3] fs: apply permissions late | expand |
Yann, On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > The combination of fakeroot, tar, and capabilities is broken, because > fakeroot currently badly handles capabilities, which are currently > simply ignored. > > As described in #11216, asking tar to explicitly store and restore > capabilities ends up with a failling build, when tar actually tries to failling -> failing > restore the capabilities. Adding support for capabilities to fakeroot > (by adding host-libcap as dependency) does not fix the problem. > > Capabilities are stored in the extended attribute security.capabilty. Capabilities are stored in the extended attribute security capability. > It turns out that tar does have special handling when extracting and > restoring that extended attribute, and that fails miserably when running > under fakeroot... > > We fix that by offloading the permissions handling down to individual > filesystems. > > This needs a split of the makedevs call, with the current and first one > now only responsible for creating the pseudo devices, while the new, > second call does only set the permissions. > > Fixes: #11216 > > This changes the order of steps, and post-fakeroot scripts are now > called before the permissions are set. This could mean breaking existing > setups, but more probably, this woudl sovle some, where files created in setups, but more probably, this would solve some, where files created in > post-fakeroot scripts can now see their permissions appropriately set. > > This also slightly breaks the idea behind the intermediate image, which > was supposed to gather all actions common to all filesystems, so that > they are not repeated. Still, most actions are still created only once, > and moving just this is purely a practical and pragmatic workaround. > > 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: Matthew Weber <matthew.weber@rockwellcollins.com> Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com> > --- > fs/common.mk | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/fs/common.mk b/fs/common.mk > index 453da6010a..569e5d60c5 100644 > --- a/fs/common.mk > +++ b/fs/common.mk > @@ -29,8 +29,8 @@ > > 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)) > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > USERS_TABLE = $(FS_DIR)/users_table.txt > ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) > > @@ -81,14 +81,13 @@ 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) > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) > + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) > ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) > $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) > endif > -endif > - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) > echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) > +endif > $(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)) > @@ -108,6 +107,7 @@ define inner-rootfs > > ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) > ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt > > ROOTFS_$(2)_DEPENDENCIES += rootfs-common > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) > echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) > echo "set -e" >> $$(FAKEROOT_SCRIPT) > $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) > +ifneq ($$(ROOTFS_PERMISSION_TABLES),) > + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) > +endif > + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) If a package duplicates an entry and is below a user provided rootfs permissions table similar item, I assume makedev uses the last entry as the one to set? If so, should the two lines above be flipped so the "user provided" can always fixup/override the package default? Matt
Matt, All, On 2018-10-27 08:14 -0500, Matthew Weber spake thusly: > On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > > The combination of fakeroot, tar, and capabilities is broken, because > > fakeroot currently badly handles capabilities, which are currently > > simply ignored. > > > > As described in #11216, asking tar to explicitly store and restore > > capabilities ends up with a failling build, when tar actually tries to > failling -> failing > > > restore the capabilities. Adding support for capabilities to fakeroot > > (by adding host-libcap as dependency) does not fix the problem. > > > > Capabilities are stored in the extended attribute security.capabilty. > Capabilities are stored in the extended attribute security capability. Thanks for the fixes! :-) Yet, the extended attribute is really named "security.capabilty" (i.e. with a dot in-between the two words): https://linux.die.net/man/7/capabilities Since kernel 2.6.24, the kernel supports associating capability sets [...] stored in an extended attribute (see setxattr(2)) named security.capability.o Regards, Yann E. MORIN. > > It turns out that tar does have special handling when extracting and > > restoring that extended attribute, and that fails miserably when running > > under fakeroot... > > > > We fix that by offloading the permissions handling down to individual > > filesystems. > > > > This needs a split of the makedevs call, with the current and first one > > now only responsible for creating the pseudo devices, while the new, > > second call does only set the permissions. > > > > Fixes: #11216 > > > > This changes the order of steps, and post-fakeroot scripts are now > > called before the permissions are set. This could mean breaking existing > > setups, but more probably, this woudl sovle some, where files created in > setups, but more probably, this would solve some, where files created in > > > post-fakeroot scripts can now see their permissions appropriately set. > > > > This also slightly breaks the idea behind the intermediate image, which > > was supposed to gather all actions common to all filesystems, so that > > they are not repeated. Still, most actions are still created only once, > > and moving just this is purely a practical and pragmatic workaround. > > > > 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: Matthew Weber <matthew.weber@rockwellcollins.com> > > Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com> > > > --- > > fs/common.mk | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/fs/common.mk b/fs/common.mk > > index 453da6010a..569e5d60c5 100644 > > --- a/fs/common.mk > > +++ b/fs/common.mk > > @@ -29,8 +29,8 @@ > > > > 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)) > > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) > > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > > USERS_TABLE = $(FS_DIR)/users_table.txt > > ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) > > > > @@ -81,14 +81,13 @@ 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) > > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) > > + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) > > ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) > > $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) > > endif > > -endif > > - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) > > echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) > > +endif > > $(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)) > > @@ -108,6 +107,7 @@ define inner-rootfs > > > > ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) > > ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target > > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt > > > > ROOTFS_$(2)_DEPENDENCIES += rootfs-common > > > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) > > echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) > > echo "set -e" >> $$(FAKEROOT_SCRIPT) > > $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) > > +ifneq ($$(ROOTFS_PERMISSION_TABLES),) > > + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) > > +endif > > + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) > > If a package duplicates an entry and is below a user provided rootfs > permissions table similar item, I assume makedev uses the last entry > as the one to set? If so, should the two lines above be flipped so > the "user provided" can always fixup/override the package default? > > Matt
Yann, On Tue, Oct 30, 2018 at 3:23 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Matt, All, > > On 2018-10-27 08:14 -0500, Matthew Weber spake thusly: > > On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > > > > The combination of fakeroot, tar, and capabilities is broken, because > > > fakeroot currently badly handles capabilities, which are currently > > > simply ignored. > > > > > > As described in #11216, asking tar to explicitly store and restore > > > capabilities ends up with a failling build, when tar actually tries to > > failling -> failing > > > > > restore the capabilities. Adding support for capabilities to fakeroot > > > (by adding host-libcap as dependency) does not fix the problem. > > > > > > Capabilities are stored in the extended attribute security.capabilty. > > Capabilities are stored in the extended attribute security capability. > > Thanks for the fixes! :-) > > Yet, the extended attribute is really named "security.capabilty" (i.e. > with a dot in-between the two words): https://linux.die.net/man/7/capabilities > > Since kernel 2.6.24, the kernel supports associating capability sets > [...] stored in an extended attribute (see setxattr(2)) named > security.capability.o :-) Fair, security.capabilty -> security.capability > > Regards, > Yann E. MORIN. > > > > It turns out that tar does have special handling when extracting and > > > restoring that extended attribute, and that fails miserably when running > > > under fakeroot... > > > > > > We fix that by offloading the permissions handling down to individual > > > filesystems. > > > > > > This needs a split of the makedevs call, with the current and first one > > > now only responsible for creating the pseudo devices, while the new, > > > second call does only set the permissions. > > > > > > Fixes: #11216 > > > > > > This changes the order of steps, and post-fakeroot scripts are now > > > called before the permissions are set. This could mean breaking existing > > > setups, but more probably, this woudl sovle some, where files created in > > setups, but more probably, this would solve some, where files created in > > > > > post-fakeroot scripts can now see their permissions appropriately set. > > > > > > This also slightly breaks the idea behind the intermediate image, which > > > was supposed to gather all actions common to all filesystems, so that > > > they are not repeated. Still, most actions are still created only once, > > > and moving just this is purely a practical and pragmatic workaround. > > > > > > 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: Matthew Weber <matthew.weber@rockwellcollins.com> > > > > Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com> > > > > > --- > > > fs/common.mk | 17 +++++++++++------ > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/common.mk b/fs/common.mk > > > index 453da6010a..569e5d60c5 100644 > > > --- a/fs/common.mk > > > +++ b/fs/common.mk > > > @@ -29,8 +29,8 @@ > > > > > > 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)) > > > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) > > > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > > > USERS_TABLE = $(FS_DIR)/users_table.txt > > > ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) > > > > > > @@ -81,14 +81,13 @@ 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) > > > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) > > > + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) > > > ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) > > > $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) > > > endif > > > -endif > > > - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) > > > echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) > > > +endif > > > $(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)) > > > @@ -108,6 +107,7 @@ define inner-rootfs > > > > > > ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) > > > ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target > > > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt > > > > > > ROOTFS_$(2)_DEPENDENCIES += rootfs-common > > > > > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) > > > echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) > > > echo "set -e" >> $$(FAKEROOT_SCRIPT) > > > $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) > > > +ifneq ($$(ROOTFS_PERMISSION_TABLES),) > > > + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) > > > +endif > > > + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) > > > > If a package duplicates an entry and is below a user provided rootfs > > permissions table similar item, I assume makedev uses the last entry > > as the one to set? If so, should the two lines above be flipped so > > the "user provided" can always fixup/override the package default? > > > > Matt > > -- > .-----------------.--------------------.------------------.--------------------. > | 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. | > '------------------------------^-------^------------------^--------------------'
Matt, All, On 2018-10-30 20:18 -0500, Matthew Weber spake thusly: > On Tue, Oct 30, 2018 at 3:23 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Since kernel 2.6.24, the kernel supports associating capability sets > > [...] stored in an extended attribute (see setxattr(2)) named > > security.capability.o > :-) Fair, security.capabilty -> security.capability Muahaha! Thanks :-) Regards, Yann E. MORIN.
Yann, On Wed, Oct 31, 2018 at 4:49 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Matt, All, > > On 2018-10-30 20:18 -0500, Matthew Weber spake thusly: > > On Tue, Oct 30, 2018 at 3:23 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > Since kernel 2.6.24, the kernel supports associating capability sets > > > [...] stored in an extended attribute (see setxattr(2)) named > > > security.capability.o > > :-) Fair, security.capabilty -> security.capability > Tested-by: Matthew Weber <matthew.weber@rockwellcollins.com> Ran some scenaios as a regression on existing target builds before and after the change.
Hello, On Sat, 27 Oct 2018 09:45:58 +0200, Yann E. MORIN wrote: > This needs a split of the makedevs call, with the current and first one > now only responsible for creating the pseudo devices, while the new, > second call does only set the permissions. What happens if there are special permissions/extended attributes set on pseudo devices in the static device table ? > diff --git a/fs/common.mk b/fs/common.mk > index 453da6010a..569e5d60c5 100644 > --- a/fs/common.mk > +++ b/fs/common.mk > @@ -29,8 +29,8 @@ > > FS_DIR = $(BUILD_DIR)/buildroot-fs > FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt I no longer like the name FULL_DEVICE_TABLE, nor device_table.txt, because it's no longer the "full device table". > -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \ > - $(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > USERS_TABLE = $(FS_DIR)/users_table.txt > ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) > > @@ -81,14 +81,13 @@ 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) > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) > + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) > ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) > $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) > endif > -endif > - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) > echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) > +endif > $(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)) > @@ -108,6 +107,7 @@ define inner-rootfs > > ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) > ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt There is no reason for this variable to be filesystem-specific, nor for this permissions.txt file to be generated for each filesystem format. So I'd prefer to see something like this in fs/common.mk: ROOTFS_FULL_STATIC_DEVICE_TABLE = $(FS_DIR)/full_static_device_table.txt ROOTFS_FULL_PERMISSION_TABLE = $(FS_DIR)/full_permission_table.txt both are generated in fs/common.mk, but only ROOTFS_FULL_STATIC_DEVICE_TABLE is used in the makedevs call in fs/common.mk. > ROOTFS_$(2)_DEPENDENCIES += rootfs-common > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) > echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) > echo "set -e" >> $$(FAKEROOT_SCRIPT) > $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) > +ifneq ($$(ROOTFS_PERMISSION_TABLES),) > + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) > +endif > + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) i.e, those lines would migrate back into fs/common.mk. > + echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT) And this would use $(ROOTFS_FULL_PERMISSION_TABLE) This would make the whole thing more consistent IMO. Of course unless we decide that pseudo devices in the static device table can have extended attributes, in which case, the makedevs for static devices anyway needs to be moved to the per-filesystem code. Best regards, Thomas
On 27/10/18 09:45, Yann E. MORIN wrote: > The combination of fakeroot, tar, and capabilities is broken, because > fakeroot currently badly handles capabilities, which are currently > simply ignored. "simply ignored" can't be the case, otherwise it still wouldn't work after this patch. I *think* the real problem is that fakeroot forgets to wrap the setxattrat and lsetxattrat syscalls, and these are the ones that are used by tar. > As described in #11216, asking tar to explicitly store and restore > capabilities ends up with a failling build, when tar actually tries to > restore the capabilities. Adding support for capabilities to fakeroot > (by adding host-libcap as dependency) does not fix the problem. Just to be clear: with this patch, the 'tar' filesystem does still work because the creation of the tarball works OK, it's just the extraction that goes wrong, right? > Capabilities are stored in the extended attribute security.capabilty. > It turns out that tar does have special handling when extracting and > restoring that extended attribute, and that fails miserably when running > under fakeroot... > > We fix that by offloading the permissions handling down to individual > filesystems. > > This needs a split of the makedevs call, with the current and first one > now only responsible for creating the pseudo devices, while the new, > second call does only set the permissions. Why? Is that just to reduce the impact on post-fakeroot scripts? I'd rather move the entire makedevs call to the per-filesystem phase. > > Fixes: #11216 > > This changes the order of steps, and post-fakeroot scripts are now > called before the permissions are set. This could mean breaking existing > setups, but more probably, this woudl sovle some, where files created in > post-fakeroot scripts can now see their permissions appropriately set. Since extracting xattrs doesn't work correctly, this is not true. And normally in a post-fakeroot script, you would do any permission setting in the script itself and not use the permissions table. So I think this statement is utterly wrong. That said, I doubt that post-build scripts would be affected by the permissions tables anyway. > This also slightly breaks the idea behind the intermediate image, which > was supposed to gather all actions common to all filesystems, so that > they are not repeated. Still, most actions are still created only once, > and moving just this is purely a practical and pragmatic workaround. Still, I'd prefer to fix fakeroot :-) > > 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: Matthew Weber <matthew.weber@rockwellcollins.com> > --- > fs/common.mk | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/fs/common.mk b/fs/common.mk > index 453da6010a..569e5d60c5 100644 > --- a/fs/common.mk > +++ b/fs/common.mk > @@ -29,8 +29,8 @@ > > 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)) > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > USERS_TABLE = $(FS_DIR)/users_table.txt > ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) > > @@ -81,14 +81,13 @@ 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) > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) > + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) > ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) > $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) > endif > -endif > - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) > echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) > +endif > $(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)) > @@ -108,6 +107,7 @@ define inner-rootfs > > ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) > ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt BTW I agree with Thomas that it's better to create this file only once and only do the makedevs call in the per-filesystem stage. Regards, Arnout > > ROOTFS_$(2)_DEPENDENCIES += rootfs-common > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) > echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) > echo "set -e" >> $$(FAKEROOT_SCRIPT) > $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) > +ifneq ($$(ROOTFS_PERMISSION_TABLES),) > + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) > +endif > + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) > + echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT) > $$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),\ > $$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT)$$(sep)) > $$(call PRINTF,$$(ROOTFS_REPRODUCIBLE)) >> $$(FAKEROOT_SCRIPT) >
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > The combination of fakeroot, tar, and capabilities is broken, because > fakeroot currently badly handles capabilities, which are currently > simply ignored. > As described in #11216, asking tar to explicitly store and restore > capabilities ends up with a failling build, when tar actually tries to > restore the capabilities. Adding support for capabilities to fakeroot > (by adding host-libcap as dependency) does not fix the problem. > Capabilities are stored in the extended attribute security.capabilty. > It turns out that tar does have special handling when extracting and > restoring that extended attribute, and that fails miserably when running > under fakeroot... Hmm, playing a bit around with tar here (1.29, Debian) it looks like it doesn't actually extract the security.capability xattrs when --xattrs is used unless --xattrs-include='*.*' is used: getcap /usr/bin/i3status /usr/bin/i3status = cap_net_admin+ep sudo tar -cvvvf foo.tar /usr/bin/i3status tar: Removing leading `/' from member names -rwxr-xr-x root/root 84888 2017-01-21 15:42 /usr/bin/i3status hexdump -C foo.tar | grep -A2 ecu No security.capability in the .tar sudo tar --xattrs -cvvvf foo.tar /usr/bin/i3status tar: Removing leading `/' from member names -rwxr-xr-x root/root 84888 2017-01-21 15:42 /usr/bin/i3status hexdump -C foo.tar | grep -A2 ecu 00000240 43 48 49 4c 59 2e 78 61 74 74 72 2e 73 65 63 75 |CHILY.xattr.secu| 00000250 72 69 74 79 2e 63 61 70 61 62 69 6c 69 74 79 3d |rity.capability=| 00000260 01 00 00 02 00 10 00 00 00 00 00 00 00 00 00 00 |................| With --xattrs they are included but not listed in the verbose output: sudo tar --xattrs --xattrs-include='*.*' -cvvvf foo.tar /usr/bin/i3status tar: Removing leading `/' from member names -rwxr-xr-x* root/root 84888 2017-01-21 15:42 /usr/bin/i3status x: 20 security.capability hexdump -C foo.tar | grep -A2 ecu 00000240 43 48 49 4c 59 2e 78 61 74 74 72 2e 73 65 63 75 |CHILY.xattr.secu| 00000250 72 69 74 79 2e 63 61 70 61 62 69 6c 69 74 79 3d |rity.capability=| 00000260 01 00 00 02 00 10 00 00 00 00 00 00 00 00 00 00 |................| Same .tar content but now also listed in the verbose output. For extraction: tar -xvvvf ../foo.tar && getcap usr/bin/i3status -rwxr-xr-x root/root 84888 2017-01-21 15:42 usr/bin/i3status No capability. tar --xattrs -xvvvf ../foo.tar && getcap usr/bin/i3status -rwxr-xr-x root/root 84888 2017-01-21 15:42 usr/bin/i3status Still no capability. tar --xattrs --xattrs-include='*.*' -xvvvf ../foo.tar && getcap usr/bin/i3status -rwxr-xr-x* root/root 84888 2017-01-21 15:42 usr/bin/i3status x: 20 security.capability usr/bin/i3status = cap_net_admin+ep Now it works. I don't see where we are passing --xattrs-include. Are we sure this is a fakeroot issue after all?
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes: Hi, > tar --xattrs --xattrs-include='*.*' -xvvvf ../foo.tar && getcap usr/bin/i3status > -rwxr-xr-x* root/root 84888 2017-01-21 15:42 usr/bin/i3status > x: 20 security.capability > usr/bin/i3status = cap_net_admin+ep > Now it works. > I don't see where we are passing --xattrs-include. Are we sure this is a > fakeroot issue after all? Digging further, this is apparently expected (if confusing) behaviour: https://bugzilla.redhat.com/show_bug.cgi?id=771927#c21
On 08/11/2018 00:43, Arnout Vandecappelle wrote: > I *think* the real problem is that fakeroot forgets to wrap the setxattrat and > lsetxattrat syscalls, and these are the ones that are used by tar. So this was a stupid mistake - there is no setxattrat syscall, it's a function that is defined by tar itself. Peter's analysis looks a lot more accurate. Regards, Arnout
Peter, All, On 2018-11-08 23:58 +0100, Peter Korsgaard spake thusly: > >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > > > The combination of fakeroot, tar, and capabilities is broken, because > > fakeroot currently badly handles capabilities, which are currently > > simply ignored. > > > As described in #11216, asking tar to explicitly store and restore > > capabilities ends up with a failling build, when tar actually tries to > > restore the capabilities. Adding support for capabilities to fakeroot > > (by adding host-libcap as dependency) does not fix the problem. > > > Capabilities are stored in the extended attribute security.capabilty. > > It turns out that tar does have special handling when extracting and > > restoring that extended attribute, and that fails miserably when running > > under fakeroot... > > Hmm, playing a bit around with tar here (1.29, Debian) it looks like it > doesn't actually extract the security.capability xattrs when --xattrs is > used unless --xattrs-include='*.*' is used: > > getcap /usr/bin/i3status > /usr/bin/i3status = cap_net_admin+ep > > sudo tar -cvvvf foo.tar /usr/bin/i3status Sorry, but as discussed on IRC, your analysis is incorrect, because you are really root, by mean of sudo. The issue manifests itself only under fakeroot. [...] > I don't see where we are passing --xattrs-include. We are not, and this is all the grief reported in BZ-11216. > Are we sure this is a > fakeroot issue after all? So, here is a report of a bit more experimentations. 'master' is 6a5e9a7ac6. The configuration is from support/testing/tests/core/test_file_capabilities.py The tar that is used is the one from my system: tar (GNU tar) 1.29 master no capability in rootfs master tar with --xattrs-include='*' no capability in rootfs master untar with --xattrs-include='*' no capability in rootfs master tar with --xattrs-include='*' untar with --xattrs-include='*' File [...]/usr/sbin/getcap has unrecognised filetype 0, ignoring /usr/sbin/getcap is missing from rootfs master fakeroot with libcap no capability in rootfs master fakeroot with libcap tar with --xattrs-include='*' untar with --xattrs-include='*' File [...]/usr/sbin/getcap has unrecognised filetype 0, ignoring /usr/sbin/getcap is missing from rootfs To be noted: the rootfs in that config is a squashfs. mksquashfs simply ignores the unknown filetype, so getcap is missing in the filesystem. However, should one switch to using ext4 for the test, then mkfs.ext4 would choke on that unknown filetype, and error out, which we would catch (same behaviour whether fakeroot is linked with libcap or not): Copying files into the device: __populate_fs: ignoring entry "getcap" getcap: File not found by ext2_lookup while looking up "getcap" mkfs.ext4: File not found by ext2_lookup while populating file system *** Maybe you need to increase the filesystem size (BR2_TARGET_ROOTFS_EXT2_SIZE) fs/ext2/ext2.mk:46: recipe for target '/home/ymorin/dev/buildroot/O/images/rootfs.ext2' failed make[1]: *** [/home/ymorin/dev/buildroot/O/images/rootfs.ext2] Error 1 Makefile:23: recipe for target '_all' failed make: *** [_all] Error 2 Note that only the first three lines are messages from mkfs.ext4. It also fails for ext2 and ext3, btw, and possibly some other filesystems. Regards, Yann E. MORIN.
Arnout, All, On 2018-11-09 21:13 +0100, Arnout Vandecappelle spake thusly: > On 08/11/2018 00:43, Arnout Vandecappelle wrote: > > I *think* the real problem is that fakeroot forgets to wrap the setxattrat and > > lsetxattrat syscalls, and these are the ones that are used by tar. > > So this was a stupid mistake - there is no setxattrat syscall, it's a function > that is defined by tar itself. ACK, thanks for the feedback. > Peter's analysis looks a lot more accurate. Except he was not using fakeroot, but sudo, which does work (as explained in BZ-11216). Regards, Yann E. MORIN.
Thomas, All, On 2018-11-03 14:38 +0100, Thomas Petazzoni spake thusly: > On Sat, 27 Oct 2018 09:45:58 +0200, Yann E. MORIN wrote: > > > This needs a split of the makedevs call, with the current and first one > > now only responsible for creating the pseudo devices, while the new, > > second call does only set the permissions. > > What happens if there are special permissions/extended attributes set > on pseudo devices in the static device table ? This is of no consequence, because that is not possible to do with our makedevs tool: it only ever supports setting capabilties; even though we claim xattr, it's really capabilties: https://git.buildroot.org/buildroot/tree/package/makedevs/makedevs.c#n355 Capabilities are 100% useless on device nodes, because they are only used for read/write/ioctl. capabilties are used to give a process more permissions than it would otherwise have, i.e. capabilities are to be set on executables. So it does not make sense to set capabilties on device nodes in /dev... > > diff --git a/fs/common.mk b/fs/common.mk > > index 453da6010a..569e5d60c5 100644 > > --- a/fs/common.mk > > +++ b/fs/common.mk > > @@ -29,8 +29,8 @@ > > > > FS_DIR = $(BUILD_DIR)/buildroot-fs > > FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt > > I no longer like the name FULL_DEVICE_TABLE, nor device_table.txt, > because it's no longer the "full device table". Well, now at least, it only contains devices, and not permissions. And yes, it is the full device table now, because it does contain all the _devices_ that are to be created: those from the static table(S), and those from packages, and nothing more. One could argue that the previous denomination was in fact wrong, and that it is now correct. ;-) However, see below... > > -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \ > > - $(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) > > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) > > USERS_TABLE = $(FS_DIR)/users_table.txt > > ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) > > > > @@ -81,14 +81,13 @@ 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) > > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) > > + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) > > ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) > > $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) > > endif > > -endif > > - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) > > echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) > > +endif > > $(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)) > > @@ -108,6 +107,7 @@ define inner-rootfs > > > > ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) > > ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target > > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt > > There is no reason for this variable to be filesystem-specific, nor for > this permissions.txt file to be generated for each filesystem format. > > So I'd prefer to see something like this in fs/common.mk: > > ROOTFS_FULL_STATIC_DEVICE_TABLE = $(FS_DIR)/full_static_device_table.txt > ROOTFS_FULL_PERMISSION_TABLE = $(FS_DIR)/full_permission_table.txt > > both are generated in fs/common.mk, but only > ROOTFS_FULL_STATIC_DEVICE_TABLE is used in the makedevs call in > fs/common.mk. ACK. > > ROOTFS_$(2)_DEPENDENCIES += rootfs-common > > > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) > > echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) > > echo "set -e" >> $$(FAKEROOT_SCRIPT) > > $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) > > +ifneq ($$(ROOTFS_PERMISSION_TABLES),) > > + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) > > +endif > > + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) > > i.e, those lines would migrate back into fs/common.mk. > > > + echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT) > > And this would use $(ROOTFS_FULL_PERMISSION_TABLE) ACK > This would make the whole thing more consistent IMO. Of course unless > we decide that pseudo devices in the static device table can have > extended attributes, in which case, the makedevs for static devices > anyway needs to be moved to the per-filesystem code. As I explained above, I believe it does not make sense. However, as we previously discussed, we maybe should drop the inter,mediate rootfs step, and revert to duplicating everything in each filesystem steps, each starting off by copying the origian target/ to their own copy and hack that. I'm not sure which is the simplest to do to be ready for this release cycle, I'll be investigating this more by the end of the WE... Regards, Yann E. MORIN.
Arnout, All, On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly: > On 27/10/18 09:45, Yann E. MORIN wrote: > > The combination of fakeroot, tar, and capabilities is broken, because > > fakeroot currently badly handles capabilities, which are currently > > simply ignored. > > "simply ignored" can't be the case, otherwise it still wouldn't work after this > patch. No, because fakeroot really badly handles capabilties, as demonstrated in another mail in that thread. And yes, capabilties *are* currently simply ignored, and that's on purpose (not that changing would fix it anyway, and I'm not even totally sure this is related, and neither am I really sure to understand what it even really means), added in c994c3db1f7d (fakeroot: disable capabilities): https://git.buildroot.org/buildroot/tree/package/fakeroot/fakeroot.mk#n12 # Force capabilities detection off # For now these are process capabilities (faked) rather than file # so they're of no real use HOST_FAKEROOT_CONF_ENV = \ ac_cv_header_sys_capability_h=no \ ac_cv_func_capset=no [--SNIP--] > > As described in #11216, asking tar to explicitly store and restore > > capabilities ends up with a failling build, when tar actually tries to > > restore the capabilities. Adding support for capabilities to fakeroot > > (by adding host-libcap as dependency) does not fix the problem. > > Just to be clear: with this patch, the 'tar' filesystem does still work because > the creation of the tarball works OK, it's just the extraction that goes wrong, > right? Oh, you poinpoint an issue: the tar filesystem must be also modified to add --xattrs-include='*' otherwise it would not contain any xattrs, and even less so, any capabilty. So, with this patch, the 'tar' filesystem is not fixed, but this is not a regression either, as it did not have capabilties, and it still does not. Now, should we fix the tar filesystem, a tarball of the rootfs is supposed to be extracted by root, the real root, not a fake root, and as Peter demonstrated, there is no problem with capabilities in a tarball that is extracted by root. > > Capabilities are stored in the extended attribute security.capabilty. > > It turns out that tar does have special handling when extracting and > > restoring that extended attribute, and that fails miserably when running > > under fakeroot... > > > > We fix that by offloading the permissions handling down to individual > > filesystems. > > > > This needs a split of the makedevs call, with the current and first one > > now only responsible for creating the pseudo devices, while the new, > > second call does only set the permissions. > > Why? Is that just to reduce the impact on post-fakeroot scripts? The idea of the common, intermediate tarball was to include all the common actions, to avoid repeating them for each filesystem. Since the creating of device nodes in /dev is not impacted by the capabilties, I left that in the common part, and only moved the offending part (setting permissions, which includes capabilties) to the per-filesystem actions. It had about nothing to do with the post-fakeroot scripts that I had thought about. > I'd rather move the entire makedevs call to the per-filesystem phase. I'm still not convinced, as this is not needed (capabilties don't make sense on device nodes), except if we were to entirely drop the intemediate tarball altogether. > > Fixes: #11216 > > > > This changes the order of steps, and post-fakeroot scripts are now > > called before the permissions are set. This could mean breaking existing > > setups, but more probably, this woudl sovle some, where files created in > > post-fakeroot scripts can now see their permissions appropriately set. > > Since extracting xattrs doesn't work correctly, this is not true.i Sorry, re-reading this, I see where you choked on my explanations. The underlying reasoning I had, was that files created in a post-fakeroot scripts could have appropriate capabilties by providing a permissions table, which is _now_ applied after those files are created. So, before this patch, capabilities created in a post-fakeroot scripts were not working, and there was no way to set capabilties on those files. With this patch, they are not working either, but there is a workaround by providing a permissions table. > And normally > in a post-fakeroot script, you would do any permission setting in the script > itself and not use the permissions table. Except that was not working so far, and nobody complained, so nobody was doing that anyway. > So I think this statement is utterly > wrong. s/utterly// makes it easier to read, thanks. And yes, it was wrong, except it did not properly convey my thinking. I hope the above explanations make it a bit less wrong. > That said, I doubt that post-build scripts would be affected by the > permissions tables anyway. > > This also slightly breaks the idea behind the intermediate image, which > > was supposed to gather all actions common to all filesystems, so that > > they are not repeated. Still, most actions are still created only once, > > and moving just this is purely a practical and pragmatic workaround. > > Still, I'd prefer to fix fakeroot :-) Then, please be my guest! ;-) I already fixed something in fakeroot, and I dread working on that code base any more... Regards, Yann E. MORIN.
On 10/11/2018 18:57, Yann E. MORIN wrote: > Arnout, All, > > On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly: [snip] >> Still, I'd prefer to fix fakeroot :-) > > Then, please be my guest! ;-) I already fixed something in fakeroot, and > I dread working on that code base any more... Time to switch to pseudo? The .11-rc period is ideal for that, isn't it? :-) Regards, Arnout
Arnout, All, On 2018-11-11 17:02 +0100, Arnout Vandecappelle spake thusly: > On 10/11/2018 18:57, Yann E. MORIN wrote: > > Arnout, All, > > > > On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly: > [snip] > >> Still, I'd prefer to fix fakeroot :-) > > > > Then, please be my guest! ;-) I already fixed something in fakeroot, and > > I dread working on that code base any more... > > Time to switch to pseudo? The .11-rc period is ideal for that, isn't it? :-) I already made that joke on IRC the other day! ;-) 2018-11-09 22:01 < y_morin> Maybe we will eventually have to switch to an alternative (pseudo?) NOOOO!!!!!! :-] Regards, Yann E. MORIN.
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: Hi, >> Just to be clear: with this patch, the 'tar' filesystem does still work because >> the creation of the tarball works OK, it's just the extraction that goes wrong, >> right? > Oh, you poinpoint an issue: the tar filesystem must be also modified to > add --xattrs-include='*' otherwise it would not contain any xattrs, and > even less so, any capabilty. I believe my tests showed that --xattrs-include isn't needed when creating the tarball (but _IS_ needed when extracting), as long as --xattrs is passed.
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > On 10/11/2018 18:57, Yann E. MORIN wrote: >> Arnout, All, >> >> On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly: > [snip] >>> Still, I'd prefer to fix fakeroot :-) >> >> Then, please be my guest! ;-) I already fixed something in fakeroot, and >> I dread working on that code base any more... > Time to switch to pseudo? The .11-rc period is ideal for that, isn't it? :-) I see the smiley, but does pseudo correctly handle xattrs/capabilities?
On 2018-11-11 21:02 +0100, Peter Korsgaard spake thusly: > >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > > Oh, you poinpoint an issue: the tar filesystem must be also modified to > > add --xattrs-include='*' otherwise it would not contain any xattrs, and > > even less so, any capabilty. > > I believe my tests showed that --xattrs-include isn't needed when > creating the tarball (but _IS_ needed when extracting), as long as > --xattrs is passed. Right, but the tar filesystem does have to be fixed, anyway. I don't like it that the extraction is not symetric to the creation, and since this has puzzled us (and a lot of other people), I'd prefer if we were explicit which xattrs we ionclude. Regards, Yann E. MORIN.
diff --git a/fs/common.mk b/fs/common.mk index 453da6010a..569e5d60c5 100644 --- a/fs/common.mk +++ b/fs/common.mk @@ -29,8 +29,8 @@ 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)) +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE)) USERS_TABLE = $(FS_DIR)/users_table.txt ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES)) @@ -81,14 +81,13 @@ 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) +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),) + cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE) ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y) $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE) endif -endif - $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE) echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT) +endif $(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)) @@ -108,6 +107,7 @@ define inner-rootfs ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1) ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt ROOTFS_$(2)_DEPENDENCIES += rootfs-common @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES) echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT) echo "set -e" >> $$(FAKEROOT_SCRIPT) $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT) +ifneq ($$(ROOTFS_PERMISSION_TABLES),) + cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE) +endif + $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE) + echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT) $$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),\ $$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT)$$(sep)) $$(call PRINTF,$$(ROOTFS_REPRODUCIBLE)) >> $$(FAKEROOT_SCRIPT)
The combination of fakeroot, tar, and capabilities is broken, because fakeroot currently badly handles capabilities, which are currently simply ignored. As described in #11216, asking tar to explicitly store and restore capabilities ends up with a failling build, when tar actually tries to restore the capabilities. Adding support for capabilities to fakeroot (by adding host-libcap as dependency) does not fix the problem. Capabilities are stored in the extended attribute security.capabilty. It turns out that tar does have special handling when extracting and restoring that extended attribute, and that fails miserably when running under fakeroot... We fix that by offloading the permissions handling down to individual filesystems. This needs a split of the makedevs call, with the current and first one now only responsible for creating the pseudo devices, while the new, second call does only set the permissions. Fixes: #11216 This changes the order of steps, and post-fakeroot scripts are now called before the permissions are set. This could mean breaking existing setups, but more probably, this woudl sovle some, where files created in post-fakeroot scripts can now see their permissions appropriately set. This also slightly breaks the idea behind the intermediate image, which was supposed to gather all actions common to all filesystems, so that they are not repeated. Still, most actions are still created only once, and moving just this is purely a practical and pragmatic workaround. 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: Matthew Weber <matthew.weber@rockwellcollins.com> --- fs/common.mk | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)