* [PATCH] create_inode: set xattrs to the root directory as well
@ 2020-07-01 15:34 Antoine Tenart
2020-07-17 7:29 ` Antoine Tenart
2020-07-17 7:46 ` Andreas Dilger
0 siblings, 2 replies; 4+ messages in thread
From: Antoine Tenart @ 2020-07-01 15:34 UTC (permalink / raw)
To: linux-ext4, tytso; +Cc: Antoine Tenart, matthew.weber, thomas.petazzoni
__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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] create_inode: set xattrs to the root directory as well
2020-07-01 15:34 [PATCH] create_inode: set xattrs to the root directory as well Antoine Tenart
@ 2020-07-17 7:29 ` Antoine Tenart
2020-07-17 7:46 ` Andreas Dilger
1 sibling, 0 replies; 4+ messages in thread
From: Antoine Tenart @ 2020-07-17 7:29 UTC (permalink / raw)
To: linux-ext4, tytso; +Cc: matthew.weber, thomas.petazzoni
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] create_inode: set xattrs to the root directory as well
2020-07-01 15:34 [PATCH] create_inode: set xattrs to the root directory as well Antoine Tenart
2020-07-17 7:29 ` Antoine Tenart
@ 2020-07-17 7:46 ` Andreas Dilger
2020-07-17 8:24 ` Antoine Tenart
1 sibling, 1 reply; 4+ messages in thread
From: Andreas Dilger @ 2020-07-17 7:46 UTC (permalink / raw)
To: Antoine Tenart
Cc: Ext4 Developers List, tytso, matthew.weber, thomas.petazzoni
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] create_inode: set xattrs to the root directory as well
2020-07-17 7:46 ` Andreas Dilger
@ 2020-07-17 8:24 ` Antoine Tenart
0 siblings, 0 replies; 4+ messages in thread
From: Antoine Tenart @ 2020-07-17 8:24 UTC (permalink / raw)
To: Andreas Dilger
Cc: Ext4 Developers List, tytso, matthew.weber, thomas.petazzoni
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-17 8:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-01 15:34 [PATCH] create_inode: set xattrs to the root directory as well Antoine Tenart
2020-07-17 7:29 ` Antoine Tenart
2020-07-17 7:46 ` Andreas Dilger
2020-07-17 8:24 ` Antoine Tenart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).