[1/3] fs: apply permissions late

Message ID 4f9a31b0fb8f87ded9c788cb92d727b6763859ac.1540626349.git.yann.morin.1998@free.fr
State Changes Requested
Headers show
Series
  • [1/3] fs: apply permissions late
Related show

Commit Message

Yann E. MORIN Oct. 27, 2018, 7:45 a.m.
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(-)

Comments

Matthew Weber Oct. 27, 2018, 1:14 p.m. | #1
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
Yann E. MORIN Oct. 30, 2018, 8:23 p.m. | #2
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
Matthew Weber Oct. 31, 2018, 1:18 a.m. | #3
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.  |
> '------------------------------^-------^------------------^--------------------'
Yann E. MORIN Oct. 31, 2018, 9:49 p.m. | #4
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.
Matthew Weber Nov. 2, 2018, 8:29 p.m. | #5
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.
Thomas Petazzoni Nov. 3, 2018, 1:38 p.m. | #6
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
Arnout Vandecappelle Nov. 7, 2018, 11:43 p.m. | #7
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)
>
Peter Korsgaard Nov. 8, 2018, 10:58 p.m. | #8
>>>>> "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 Korsgaard Nov. 9, 2018, 8:55 a.m. | #9
>>>>> "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
Arnout Vandecappelle Nov. 9, 2018, 8:13 p.m. | #10
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
Yann E. MORIN Nov. 10, 2018, 5:07 p.m. | #11
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.
Yann E. MORIN Nov. 10, 2018, 5:08 p.m. | #12
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.
Yann E. MORIN Nov. 10, 2018, 5:17 p.m. | #13
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.
Yann E. MORIN Nov. 10, 2018, 5:57 p.m. | #14
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.
Arnout Vandecappelle Nov. 11, 2018, 4:02 p.m. | #15
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
Yann E. MORIN Nov. 11, 2018, 4:44 p.m. | #16
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.
Peter Korsgaard Nov. 11, 2018, 8:02 p.m. | #17
>>>>> "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.
Peter Korsgaard Nov. 11, 2018, 8:03 p.m. | #18
>>>>> "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?
Yann E. MORIN Nov. 12, 2018, 8:17 a.m. | #19
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.

Patch

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)