| Message ID | 20200701153404.1647002-1-antoine.tenart@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | create_inode: set xattrs to the root directory as well | expand |
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 >
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
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 --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;
__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(-)