[PATCH] create_inode: set xattrs to the root directory as well

[PATCH] create_inode: set xattrs to the root directory as well

[Antoine Tenart]

__populate_fs do copy the xattrs for all files and directories, but the
root directory is skipped and as a result its extended attributes aren't
set. This is an issue when using mkfs to build a full system image that
can be used with SElinux in enforcing mode without making any runtime
fix at first boot.

This patch adds logic to set the root directory's extended attributes.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 misc/create_inode.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/misc/create_inode.c b/misc/create_inode.c
index e8d1df6b55a5..0a6e4dc23d16 100644
--- a/misc/create_inode.c
+++ b/misc/create_inode.c
@@ -820,7 +820,29 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 
 	for (i = 0; i < num_dents; free(dent[i]), i++) {
 		name = dent[i]->d_name;
-		if ((!strcmp(name, ".")) || (!strcmp(name, "..")))
+		if (!strcmp(name, ".")) {
+			retval = ext2fs_namei(fs, root, parent_ino, ".", &ino);
+			if (retval) {
+				com_err(name, retval, 0);
+					goto out;
+			}
+
+			/*
+			 * Take special care for the root directory, to copy its
+			 * extended attributes.
+			 */
+			if (ino == root) {
+				retval = set_inode_xattr(fs, ino, ".");
+				if (retval) {
+					com_err(__func__, retval,
+						_("while setting xattrs for ."));
+					goto out;
+				}
+			}
+
+			continue;
+		}
+		if (!strcmp(name, ".."))
 			continue;
 		if (lstat(name, &st)) {
 			retval = errno;
-- 
2.26.2

Re: [PATCH] create_inode: set xattrs to the root directory as well

[Antoine Tenart]

Hello,

Gentle ping. Does anyone have a comment on this patch?

Thanks!
Antoine

Quoting Antoine Tenart (2020-07-01 17:34:04)
> __populate_fs do copy the xattrs for all files and directories, but the
> root directory is skipped and as a result its extended attributes aren't
> set. This is an issue when using mkfs to build a full system image that
> can be used with SElinux in enforcing mode without making any runtime
> fix at first boot.
> 
> This patch adds logic to set the root directory's extended attributes.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  misc/create_inode.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/create_inode.c b/misc/create_inode.c
> index e8d1df6b55a5..0a6e4dc23d16 100644
> --- a/misc/create_inode.c
> +++ b/misc/create_inode.c
> @@ -820,7 +820,29 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
>  
>         for (i = 0; i < num_dents; free(dent[i]), i++) {
>                 name = dent[i]->d_name;
> -               if ((!strcmp(name, ".")) || (!strcmp(name, "..")))
> +               if (!strcmp(name, ".")) {
> +                       retval = ext2fs_namei(fs, root, parent_ino, ".", &ino);
> +                       if (retval) {
> +                               com_err(name, retval, 0);
> +                                       goto out;
> +                       }
> +
> +                       /*
> +                        * Take special care for the root directory, to copy its
> +                        * extended attributes.
> +                        */
> +                       if (ino == root) {
> +                               retval = set_inode_xattr(fs, ino, ".");
> +                               if (retval) {
> +                                       com_err(__func__, retval,
> +                                               _("while setting xattrs for ."));
> +                                       goto out;
> +                               }
> +                       }
> +
> +                       continue;
> +               }
> +               if (!strcmp(name, ".."))
>                         continue;
>                 if (lstat(name, &st)) {
>                         retval = errno;
> -- 
> 2.26.2
> 

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] create_inode: set xattrs to the root directory as well

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

On Jul 1, 2020, at 9:34 AM, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> 
> __populate_fs do copy the xattrs for all files and directories, but the
> root directory is skipped and as a result its extended attributes aren't
> set. This is an issue when using mkfs to build a full system image that
> can be used with SElinux in enforcing mode without making any runtime
> fix at first boot.
> 
> This patch adds logic to set the root directory's extended attributes.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
> misc/create_inode.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/create_inode.c b/misc/create_inode.c
> index e8d1df6b55a5..0a6e4dc23d16 100644
> --- a/misc/create_inode.c
> +++ b/misc/create_inode.c
> @@ -820,7 +820,29 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
> 
> 	for (i = 0; i < num_dents; free(dent[i]), i++) {
> 		name = dent[i]->d_name;
> -		if ((!strcmp(name, ".")) || (!strcmp(name, "..")))
> +		if (!strcmp(name, ".")) {

(style) despite what was previously in the code, I think it is clearer
to write "if (strcmp(name, ".") == 0)", because it doesn't read like
"if not string compare" since that incorrectly seems like the strings
are *not* matching.

> +			retval = ext2fs_namei(fs, root, parent_ino, ".", &ino);
> +			if (retval) {
> +				com_err(name, retval, 0);
> +					goto out;
> +			}
> +
> +			/*
> +			 * Take special care for the root directory, to copy its
> +			 * extended attributes.
> +			 */
> +			if (ino == root) {

Rather than checking this for every directory, it would be more efficient
to copy the root xattrs only at the start of the copy in populate_fs2(),
before the tree walk has started.  Something like:

	file_info.path_len = 0;
	file_info.path_max_len = 255;
	file_info.path = calloc(file_info.path_max_len, 1);

+	retval = set_inode_xattr(fs, parent_ino, source_dir);
+	if (retval) {
+		com_err(__func__, retval,
+			_("while copying xattrs on root directory"));
+		goto out;
+	}
+
	retval = __populate_fs(fs, parent_ino, source_dir, root, &hdlinks,
                               &file_info, fs_callbacks);

That is an even less code added, which is always good.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] create_inode: set xattrs to the root directory as well

[Antoine Tenart]

Hello Andreas,

Quoting Andreas Dilger (2020-07-17 09:46:37)
> On Jul 1, 2020, at 9:34 AM, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> > 
> > __populate_fs do copy the xattrs for all files and directories, but the
> > root directory is skipped and as a result its extended attributes aren't
> > set. This is an issue when using mkfs to build a full system image that
> > can be used with SElinux in enforcing mode without making any runtime
> > fix at first boot.
> > 
> > This patch adds logic to set the root directory's extended attributes.
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > ---
> > misc/create_inode.c | 24 +++++++++++++++++++++++-
> > 1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/misc/create_inode.c b/misc/create_inode.c
> > index e8d1df6b55a5..0a6e4dc23d16 100644
> > --- a/misc/create_inode.c
> > +++ b/misc/create_inode.c
> > @@ -820,7 +820,29 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
> > 
> >       for (i = 0; i < num_dents; free(dent[i]), i++) {
> >               name = dent[i]->d_name;
> > -             if ((!strcmp(name, ".")) || (!strcmp(name, "..")))
> > +             if (!strcmp(name, ".")) {
> 
> (style) despite what was previously in the code, I think it is clearer
> to write "if (strcmp(name, ".") == 0)", because it doesn't read like
> "if not string compare" since that incorrectly seems like the strings
> are *not* matching.

I kept what was done elsewhere in the function, but I agree with you,
let's introduce clearer new code :)

> > +                     retval = ext2fs_namei(fs, root, parent_ino, ".", &ino);
> > +                     if (retval) {
> > +                             com_err(name, retval, 0);
> > +                                     goto out;
> > +                     }
> > +
> > +                     /*
> > +                      * Take special care for the root directory, to copy its
> > +                      * extended attributes.
> > +                      */
> > +                     if (ino == root) {
> 
> Rather than checking this for every directory, it would be more efficient
> to copy the root xattrs only at the start of the copy in populate_fs2(),
> before the tree walk has started.  Something like:
> 
>         file_info.path_len = 0;
>         file_info.path_max_len = 255;
>         file_info.path = calloc(file_info.path_max_len, 1);
> 
> +       retval = set_inode_xattr(fs, parent_ino, source_dir);
> +       if (retval) {
> +               com_err(__func__, retval,
> +                       _("while copying xattrs on root directory"));
> +               goto out;
> +       }
> +
>         retval = __populate_fs(fs, parent_ino, source_dir, root, &hdlinks,
>                                &file_info, fs_callbacks);
> 
> That is an even less code added, which is always good.

Thanks for the suggestion, I find it way better. I'll prepare a v2.

Thanks for the review!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com