diff mbox series

create_inode: set xattrs to the root directory as well

Message ID 20200701153404.1647002-1-antoine.tenart@bootlin.com
State New
Headers show
Series create_inode: set xattrs to the root directory as well | expand

Commit Message

Antoine Tenart July 1, 2020, 3:34 p.m. UTC
__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(-)

Comments

Antoine Tenart July 17, 2020, 7:29 a.m. UTC | #1
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
>
Andreas Dilger July 17, 2020, 7:46 a.m. UTC | #2
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
Antoine Tenart July 17, 2020, 8:24 a.m. UTC | #3
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
diff mbox series

Patch

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;