Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
Quoting Rob Landley (2019-05-22 12:26:43) > > > On 5/22/19 11:17 AM, h...@zytor.com wrote: > > On May 20, 2019 2:39:46 AM PDT, Roberto Sassu > > wrote: > >> On 5/18/2019 12:17 AM, Arvind Sankar wrote: > >>> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote: > On 5/17/19 2:02 PM, Arvind Sankar wrote: > > On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: > >> > >> Ok... I just realized this does not work for a modular initramfs, > >> composed at load time from multiple files, which is a very real > >> problem. Should be easy enough to deal with: instead of one large file, > >> use one companion file per source file, perhaps something like > >> filename..xattrs (suggesting double dots to make it less likely to > >> conflict with a "real" file.) No leading dot, as it makes it more > >> likely that archivers will sort them before the file proper. > > This version of the patch was changed from the previous one exactly > >> to deal with this case -- > > it allows for the bootloader to load multiple initramfs archives, > >> each > > with its own .xattr-list file, and to have that work properly. > > Could you elaborate on the issue that you see? > > > > Well, for one thing, how do you define "cpio archive", each with its > >> own > .xattr-list file? Second, that would seem to depend on the ordering, > >> no, > in which case you depend critically on .xattr-list file following > >> the > files, which most archivers won't do. > > Either way it seems cleaner to have this per file; especially if/as > >> it > can be done without actually mucking up the format. > > I need to run, but I'll post a more detailed explanation of what I > >> did > in a little bit. > > -hpa > > >>> Not sure what you mean by how do I define it? Each cpio archive will > >>> contain its own .xattr-list file with signatures for the files within > >>> it, that was the idea. > >>> > >>> You need to review the code more closely I think -- it does not > >> depend > >>> on the .xattr-list file following the files to which it applies. > >>> > >>> The code first extracts .xattr-list as though it was a regular file. > >> If > >>> a later dupe shows up (presumably from a second archive, although the > >>> patch will actually allow a second one in the same archive), it will > >>> then process the existing .xattr-list file and apply the attributes > >>> listed within it. It then will proceed to read the second one and > >>> overwrite the first one with it (this is the normal behaviour in the > >>> kernel cpio parser). At the end once all the archives have been > >>> extracted, if there is an .xattr-list file in the rootfs it will be > >>> parsed (it would've been the last one encountered, which hasn't been > >>> parsed yet, just extracted). > >>> > >>> Regarding the idea to use the high 16 bits of the mode field in > >>> the header that's another possibility. It would just require > >> additional > >>> support in the program that actually creates the archive though, > >> which > >>> the current patch doesn't. > >> > >> Yes, for adding signatures for a subset of files, no changes to the ram > >> disk generator are necessary. Everything is done by a custom module. To > >> support a generic use case, it would be necessary to modify the > >> generator to execute getfattr and the awk script after files have been > >> placed in the temporary directory. > >> > >> If I understood the new proposal correctly, it would be task for cpio > >> to > >> read file metadata after the content and create a new record for each > >> file with mode 0x18000, type of metadata encoded in the file name and > >> metadata as file content. I don't know how easy it would be to modify > >> cpio. Probably the amount of changes would be reasonable. > > I could make toybox cpio do it in a weekend, and could probably throw a patch > at > usr/gen_init_cpio.c while I'm at it. I prototyped something like that a couple > years ago, it's not hard. > > The real question is scripts/gen_initramfs_list.sh and the text format it > produces. We can currently generate cpio files with different ownership and > permissions than the host system can represent (when not building as root, on > a > filesystem that may not support xattrs or would get unhappy about conflicting > selinux annotations). We work around it by having the metadata represented > textually in the initramfs_list file gen_initramfs_list.sh produces and > gen_init_cpio.c consumes. > > xattrs are a terrible idea the Macintosh invented so Finder could remember > where > you moved a file's icon in its folder without having to modify the file, and > then things like OS/2 copied it and Windows picked it up from there and went > "Of > course, this is a security mechanism!" and... sigh. > > This is "data that is not data", it's metadata of unbounded size. It seems > like > it should go in
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/22/19 11:17 AM, h...@zytor.com wrote: > On May 20, 2019 2:39:46 AM PDT, Roberto Sassu > wrote: >> On 5/18/2019 12:17 AM, Arvind Sankar wrote: >>> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote: On 5/17/19 2:02 PM, Arvind Sankar wrote: > On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: >> >> Ok... I just realized this does not work for a modular initramfs, >> composed at load time from multiple files, which is a very real >> problem. Should be easy enough to deal with: instead of one large file, >> use one companion file per source file, perhaps something like >> filename..xattrs (suggesting double dots to make it less likely to >> conflict with a "real" file.) No leading dot, as it makes it more >> likely that archivers will sort them before the file proper. > This version of the patch was changed from the previous one exactly >> to deal with this case -- > it allows for the bootloader to load multiple initramfs archives, >> each > with its own .xattr-list file, and to have that work properly. > Could you elaborate on the issue that you see? > Well, for one thing, how do you define "cpio archive", each with its >> own .xattr-list file? Second, that would seem to depend on the ordering, >> no, in which case you depend critically on .xattr-list file following >> the files, which most archivers won't do. Either way it seems cleaner to have this per file; especially if/as >> it can be done without actually mucking up the format. I need to run, but I'll post a more detailed explanation of what I >> did in a little bit. -hpa >>> Not sure what you mean by how do I define it? Each cpio archive will >>> contain its own .xattr-list file with signatures for the files within >>> it, that was the idea. >>> >>> You need to review the code more closely I think -- it does not >> depend >>> on the .xattr-list file following the files to which it applies. >>> >>> The code first extracts .xattr-list as though it was a regular file. >> If >>> a later dupe shows up (presumably from a second archive, although the >>> patch will actually allow a second one in the same archive), it will >>> then process the existing .xattr-list file and apply the attributes >>> listed within it. It then will proceed to read the second one and >>> overwrite the first one with it (this is the normal behaviour in the >>> kernel cpio parser). At the end once all the archives have been >>> extracted, if there is an .xattr-list file in the rootfs it will be >>> parsed (it would've been the last one encountered, which hasn't been >>> parsed yet, just extracted). >>> >>> Regarding the idea to use the high 16 bits of the mode field in >>> the header that's another possibility. It would just require >> additional >>> support in the program that actually creates the archive though, >> which >>> the current patch doesn't. >> >> Yes, for adding signatures for a subset of files, no changes to the ram >> disk generator are necessary. Everything is done by a custom module. To >> support a generic use case, it would be necessary to modify the >> generator to execute getfattr and the awk script after files have been >> placed in the temporary directory. >> >> If I understood the new proposal correctly, it would be task for cpio >> to >> read file metadata after the content and create a new record for each >> file with mode 0x18000, type of metadata encoded in the file name and >> metadata as file content. I don't know how easy it would be to modify >> cpio. Probably the amount of changes would be reasonable. I could make toybox cpio do it in a weekend, and could probably throw a patch at usr/gen_init_cpio.c while I'm at it. I prototyped something like that a couple years ago, it's not hard. The real question is scripts/gen_initramfs_list.sh and the text format it produces. We can currently generate cpio files with different ownership and permissions than the host system can represent (when not building as root, on a filesystem that may not support xattrs or would get unhappy about conflicting selinux annotations). We work around it by having the metadata represented textually in the initramfs_list file gen_initramfs_list.sh produces and gen_init_cpio.c consumes. xattrs are a terrible idea the Macintosh invented so Finder could remember where you moved a file's icon in its folder without having to modify the file, and then things like OS/2 copied it and Windows picked it up from there and went "Of course, this is a security mechanism!" and... sigh. This is "data that is not data", it's metadata of unbounded size. It seems like it should go in gen_initramfs_list.sh but as what, keyword=value pairs that might have embedded newlines in them? A base64 encoding? Something else? >> The kernel will behave in a similar way. It will call do_readxattrs() >> in >> do_copy() for each file. Since the only difference between
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/22/2019 6:17 PM, h...@zytor.com wrote:
On May 20, 2019 2:39:46 AM PDT, Roberto Sassu wrote:
On 5/18/2019 12:17 AM, Arvind Sankar wrote:
On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote:
On 5/17/19 2:02 PM, Arvind Sankar wrote:
On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote:
Ok... I just realized this does not work for a modular initramfs,
composed at load time from multiple files, which is a very real
problem. Should be easy enough to deal with: instead of one large file,
use one companion file per source file, perhaps something like
filename..xattrs (suggesting double dots to make it less likely to
conflict with a "real" file.) No leading dot, as it makes it more
likely that archivers will sort them before the file proper.
This version of the patch was changed from the previous one exactly
to deal with this case --
it allows for the bootloader to load multiple initramfs archives,
each
with its own .xattr-list file, and to have that work properly.
Could you elaborate on the issue that you see?
Well, for one thing, how do you define "cpio archive", each with its
own
.xattr-list file? Second, that would seem to depend on the ordering,
no,
in which case you depend critically on .xattr-list file following
the
files, which most archivers won't do.
Either way it seems cleaner to have this per file; especially if/as
it
can be done without actually mucking up the format.
I need to run, but I'll post a more detailed explanation of what I
did
in a little bit.
-hpa
Not sure what you mean by how do I define it? Each cpio archive will
contain its own .xattr-list file with signatures for the files within
it, that was the idea.
You need to review the code more closely I think -- it does not
depend
on the .xattr-list file following the files to which it applies.
The code first extracts .xattr-list as though it was a regular file.
If
a later dupe shows up (presumably from a second archive, although the
patch will actually allow a second one in the same archive), it will
then process the existing .xattr-list file and apply the attributes
listed within it. It then will proceed to read the second one and
overwrite the first one with it (this is the normal behaviour in the
kernel cpio parser). At the end once all the archives have been
extracted, if there is an .xattr-list file in the rootfs it will be
parsed (it would've been the last one encountered, which hasn't been
parsed yet, just extracted).
Regarding the idea to use the high 16 bits of the mode field in
the header that's another possibility. It would just require
additional
support in the program that actually creates the archive though,
which
the current patch doesn't.
Yes, for adding signatures for a subset of files, no changes to the ram
disk generator are necessary. Everything is done by a custom module. To
support a generic use case, it would be necessary to modify the
generator to execute getfattr and the awk script after files have been
placed in the temporary directory.
If I understood the new proposal correctly, it would be task for cpio
to
read file metadata after the content and create a new record for each
file with mode 0x18000, type of metadata encoded in the file name and
metadata as file content. I don't know how easy it would be to modify
cpio. Probably the amount of changes would be reasonable.
The kernel will behave in a similar way. It will call do_readxattrs()
in
do_copy() for each file. Since the only difference between the current
and the new proposal would be two additional calls to do_readxattrs()
in
do_name() and unpack_to_rootfs(), maybe we could support both.
Roberto
The nice thing with explicit metadata is that it doesn't have to contain the
filename per se, and each file is self-contained. There is a reason why each
cpio header starts with the magic number: each cpio record is formally
independent and can be processed in isolation. The TRAILER!!! thing is a huge
wart in the format, although in practice TRAILER!!! always has a mode of 0 and
so can be distinguished from an actual file.
The use of mode 0x18000 for metadata allows for optional backwards
compatibility for extraction; for encoding this can be handled with very simple
postprocessing.
So my suggestion would be to have mode 0x18000 indicate extended file metadata,
with the filename of the form:
optional_filename!X!
... where X indicates the type of metadata (e.g. !XATTR!). The
optional_filename prefix allows an unaware decoder to extract to a well-defined
name; simple postprocessing would be able to either remove (for size) or add
(for compatibility) this prefix. It would be an error for this prefix, if
present, to not match the name of the previous file.
Actually, I defined '..metadata..' as special name to indicate that the
file contains metadata. Then, the content of the file is a set of:
struct metadata_hdr {
char c_size[8]; /* total size
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On May 17, 2019 7:16:04 PM PDT, Rob Landley wrote: >On 5/17/19 4:41 PM, H. Peter Anvin wrote: >> On 5/17/19 1:18 PM, h...@zytor.com wrote: >>> >>> Ok... I just realized this does not work for a modular initramfs, >composed at load time from multiple files, which is a very real >problem. Should be easy enough to deal with: instead of one large file, >use one companion file per source file, perhaps something like >filename..xattrs (suggesting double dots to make it less likely to >conflict with a "real" file.) No leading dot, as it makes it more >likely that archivers will sort them before the file proper. >>> >>> A side benefit is that the format can be simpler as there is no need >to encode the filename. >>> >>> A technically cleaner solution still, but which would need archiver >modifications, would be to encode the xattrs as an optionally nameless >file (just an empty string) with a new file mode value, immediately >following the original file. The advantage there is that the archiver >itself could support xattrs and other extended metadata (which has been >requested elsewhere); the disadvantage obviously is that that it >requires new support in the archiver. However, at least it ought to be >simpler since it is still a higher protocol level than the cpio archive >itself. >>> >>> There's already one special case in cpio, which is the >"!!!TRAILER!!!" filename; although I don't think it is part of the >formal spec, to the extent there is one, I would expect that in >practice it is always encoded with a mode of 0, which incidentally >could be used to unbreak the case where such a filename actually >exists. So one way to support such extended metadata would be to set >mode to 0 and use the filename to encode the type of metadata. I wonder >how existing GNU or BSD cpio (the BSD one is better maintained these >days) would deal with reading such a file; it would at least not be a >regression if it just read it still, possibly with warnings. It could >also be possible to use bits 17:16 in the mode, which are traditionally >always zero (mode_t being 16 bits), but I believe are present in most >or all of the cpio formats for historical reasons. It might be accepted >better by existing implementations to use one of these high bits >combined with S_IFREG, I dont know. >> >> >> Correction: it's just !!!TRAILER!!!. > >We documented it as "TRAILER!!!" without leading !!!, and that its >purpose is to >flush hardlinks: > >https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt > >That's what toybox cpio has been producing. Kernel consumes it just >fine. Just >checked busybox cpio and that's what they're producing as well... > >Rob Yes, TRAILER!!! is correct. Somehow I managed to get it wrong twice. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On May 20, 2019 2:39:46 AM PDT, Roberto Sassu wrote: >On 5/18/2019 12:17 AM, Arvind Sankar wrote: >> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote: >>> On 5/17/19 2:02 PM, Arvind Sankar wrote: On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: > > Ok... I just realized this does not work for a modular initramfs, >composed at load time from multiple files, which is a very real >problem. Should be easy enough to deal with: instead of one large file, >use one companion file per source file, perhaps something like >filename..xattrs (suggesting double dots to make it less likely to >conflict with a "real" file.) No leading dot, as it makes it more >likely that archivers will sort them before the file proper. This version of the patch was changed from the previous one exactly >to deal with this case -- it allows for the bootloader to load multiple initramfs archives, >each with its own .xattr-list file, and to have that work properly. Could you elaborate on the issue that you see? >>> >>> Well, for one thing, how do you define "cpio archive", each with its >own >>> .xattr-list file? Second, that would seem to depend on the ordering, >no, >>> in which case you depend critically on .xattr-list file following >the >>> files, which most archivers won't do. >>> >>> Either way it seems cleaner to have this per file; especially if/as >it >>> can be done without actually mucking up the format. >>> >>> I need to run, but I'll post a more detailed explanation of what I >did >>> in a little bit. >>> >>> -hpa >>> >> Not sure what you mean by how do I define it? Each cpio archive will >> contain its own .xattr-list file with signatures for the files within >> it, that was the idea. >> >> You need to review the code more closely I think -- it does not >depend >> on the .xattr-list file following the files to which it applies. >> >> The code first extracts .xattr-list as though it was a regular file. >If >> a later dupe shows up (presumably from a second archive, although the >> patch will actually allow a second one in the same archive), it will >> then process the existing .xattr-list file and apply the attributes >> listed within it. It then will proceed to read the second one and >> overwrite the first one with it (this is the normal behaviour in the >> kernel cpio parser). At the end once all the archives have been >> extracted, if there is an .xattr-list file in the rootfs it will be >> parsed (it would've been the last one encountered, which hasn't been >> parsed yet, just extracted). >> >> Regarding the idea to use the high 16 bits of the mode field in >> the header that's another possibility. It would just require >additional >> support in the program that actually creates the archive though, >which >> the current patch doesn't. > >Yes, for adding signatures for a subset of files, no changes to the ram >disk generator are necessary. Everything is done by a custom module. To >support a generic use case, it would be necessary to modify the >generator to execute getfattr and the awk script after files have been >placed in the temporary directory. > >If I understood the new proposal correctly, it would be task for cpio >to >read file metadata after the content and create a new record for each >file with mode 0x18000, type of metadata encoded in the file name and >metadata as file content. I don't know how easy it would be to modify >cpio. Probably the amount of changes would be reasonable. > >The kernel will behave in a similar way. It will call do_readxattrs() >in >do_copy() for each file. Since the only difference between the current >and the new proposal would be two additional calls to do_readxattrs() >in >do_name() and unpack_to_rootfs(), maybe we could support both. > >Roberto The nice thing with explicit metadata is that it doesn't have to contain the filename per se, and each file is self-contained. There is a reason why each cpio header starts with the magic number: each cpio record is formally independent and can be processed in isolation. The TRAILER!!! thing is a huge wart in the format, although in practice TRAILER!!! always has a mode of 0 and so can be distinguished from an actual file. The use of mode 0x18000 for metadata allows for optional backwards compatibility for extraction; for encoding this can be handled with very simple postprocessing. So my suggestion would be to have mode 0x18000 indicate extended file metadata, with the filename of the form: optional_filename!X! ... where X indicates the type of metadata (e.g. !XATTR!). The optional_filename prefix allows an unaware decoder to extract to a well-defined name; simple postprocessing would be able to either remove (for size) or add (for compatibility) this prefix. It would be an error for this prefix, if present, to not match the name of the previous file. I do agree that the delayed processing of an .xattr-list as you describe ought to work
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/18/2019 12:17 AM, Arvind Sankar wrote: On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote: On 5/17/19 2:02 PM, Arvind Sankar wrote: On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: Ok... I just realized this does not work for a modular initramfs, composed at load time from multiple files, which is a very real problem. Should be easy enough to deal with: instead of one large file, use one companion file per source file, perhaps something like filename..xattrs (suggesting double dots to make it less likely to conflict with a "real" file.) No leading dot, as it makes it more likely that archivers will sort them before the file proper. This version of the patch was changed from the previous one exactly to deal with this case -- it allows for the bootloader to load multiple initramfs archives, each with its own .xattr-list file, and to have that work properly. Could you elaborate on the issue that you see? Well, for one thing, how do you define "cpio archive", each with its own .xattr-list file? Second, that would seem to depend on the ordering, no, in which case you depend critically on .xattr-list file following the files, which most archivers won't do. Either way it seems cleaner to have this per file; especially if/as it can be done without actually mucking up the format. I need to run, but I'll post a more detailed explanation of what I did in a little bit. -hpa Not sure what you mean by how do I define it? Each cpio archive will contain its own .xattr-list file with signatures for the files within it, that was the idea. You need to review the code more closely I think -- it does not depend on the .xattr-list file following the files to which it applies. The code first extracts .xattr-list as though it was a regular file. If a later dupe shows up (presumably from a second archive, although the patch will actually allow a second one in the same archive), it will then process the existing .xattr-list file and apply the attributes listed within it. It then will proceed to read the second one and overwrite the first one with it (this is the normal behaviour in the kernel cpio parser). At the end once all the archives have been extracted, if there is an .xattr-list file in the rootfs it will be parsed (it would've been the last one encountered, which hasn't been parsed yet, just extracted). Regarding the idea to use the high 16 bits of the mode field in the header that's another possibility. It would just require additional support in the program that actually creates the archive though, which the current patch doesn't. Yes, for adding signatures for a subset of files, no changes to the ram disk generator are necessary. Everything is done by a custom module. To support a generic use case, it would be necessary to modify the generator to execute getfattr and the awk script after files have been placed in the temporary directory. If I understood the new proposal correctly, it would be task for cpio to read file metadata after the content and create a new record for each file with mode 0x18000, type of metadata encoded in the file name and metadata as file content. I don't know how easy it would be to modify cpio. Probably the amount of changes would be reasonable. The kernel will behave in a similar way. It will call do_readxattrs() in do_copy() for each file. Since the only difference between the current and the new proposal would be two additional calls to do_readxattrs() in do_name() and unpack_to_rootfs(), maybe we could support both. Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/17/2019 10:18 PM, h...@zytor.com wrote:
On May 17, 2019 9:55:19 AM PDT, Roberto Sassu wrote:
This patch adds support for an alternative method to add xattrs to
files in
the rootfs filesystem. Instead of extracting them directly from the ram
disk image, they are extracted from a regular file called .xattr-list,
that
can be added by any ram disk generator available today. The file format
is:
\0
\0
.xattr-list can be generated by executing:
$ getfattr --absolute-names -d -h -R -e hex -m - \
| xattr.awk -b > ${initdir}/.xattr-list
where the content of the xattr.awk script is:
#! /usr/bin/awk -f
{
if (!length($0)) {
printf("%.10x%s\0", len, file);
for (x in xattr) {
printf("%.8x%s\0", xattr_len[x], x);
for (i = 0; i < length(xattr[x]) / 2; i++) {
printf("%c", strtonum("0x"substr(xattr[x], i * 2 + 1, 2)));
}
}
i = 0;
delete xattr;
delete xattr_len;
next;
};
if (i == 0) {
file=$3;
len=length(file) + 8 + 1;
}
if (i > 0) {
split($0, a, "=");
xattr[a[1]]=substr(a[2], 3);
xattr_len[a[1]]=length(a[1]) + 1 + 8 + length(xattr[a[1]]) / 2;
len+=xattr_len[a[1]];
};
i++;
}
Signed-off-by: Roberto Sassu
---
init/initramfs.c | 99
1 file changed, 99 insertions(+)
diff --git a/init/initramfs.c b/init/initramfs.c
index 0c6dd1d5d3f6..6ec018c6279a 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -13,6 +13,8 @@
#include
#include
+#define XATTR_LIST_FILENAME ".xattr-list"
+
static ssize_t __init xwrite(int fd, const char *p, size_t count)
{
ssize_t out = 0;
@@ -382,6 +384,97 @@ static int __init __maybe_unused do_setxattrs(char
*pathname)
return 0;
}
+struct path_hdr {
+ char p_size[10]; /* total size including p_size field */
+ char p_data[]; /* \0 */
+};
+
+static int __init do_readxattrs(void)
+{
+ struct path_hdr hdr;
+ char *path = NULL;
+ char str[sizeof(hdr.p_size) + 1];
+ unsigned long file_entry_size;
+ size_t size, path_size, total_size;
+ struct kstat st;
+ struct file *file;
+ loff_t pos;
+ int ret;
+
+ ret = vfs_lstat(XATTR_LIST_FILENAME, );
+ if (ret < 0)
+ return ret;
+
+ total_size = st.size;
+
+ file = filp_open(XATTR_LIST_FILENAME, O_RDONLY, 0);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ pos = file->f_pos;
+
+ while (total_size) {
+ size = kernel_read(file, (char *), sizeof(hdr), );
+ if (size != sizeof(hdr)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ total_size -= size;
+
+ str[sizeof(hdr.p_size)] = 0;
+ memcpy(str, hdr.p_size, sizeof(hdr.p_size));
+ ret = kstrtoul(str, 16, _entry_size);
+ if (ret < 0)
+ goto out;
+
+ file_entry_size -= sizeof(sizeof(hdr.p_size));
+ if (file_entry_size > total_size) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ path = vmalloc(file_entry_size);
+ if (!path) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ size = kernel_read(file, path, file_entry_size, );
+ if (size != file_entry_size) {
+ ret = -EIO;
+ goto out_free;
+ }
+
+ total_size -= size;
+
+ path_size = strnlen(path, file_entry_size);
+ if (path_size == file_entry_size) {
+ ret = -EINVAL;
+ goto out_free;
+ }
+
+ xattr_buf = path + path_size + 1;
+ xattr_len = file_entry_size - path_size - 1;
+
+ ret = do_setxattrs(path);
+ vfree(path);
+ path = NULL;
+
+ if (ret < 0)
+ break;
+ }
+out_free:
+ vfree(path);
+out:
+ fput(file);
+
+ if (ret < 0)
+ error("Unable to parse xattrs");
+
+ return ret;
+}
+
static __initdata int wfd;
static int __init do_name(void)
@@ -391,6 +484,11 @@ static int __init do_name(void)
if (strcmp(collected, "TRAILER!!!") == 0) {
free_hash();
return 0;
+ } else if (strcmp(collected, XATTR_LIST_FILENAME) == 0) {
+ struct kstat st;
+
+ if (!vfs_lstat(collected, ))
+ do_readxattrs();
}
clean_path(collected, mode);
if (S_ISREG(mode)) {
@@ -562,6 +660,7 @@ static char * __init unpack_to_rootfs(char *buf,
unsigned long len)
buf += my_inptr;
len -= my_inptr;
}
+ do_readxattrs();
dir_utime();
kfree(name_buf);
kfree(symlink_buf);
Ok... I just
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/17/2019 11:10 PM, Arvind Sankar wrote: On Fri, May 17, 2019 at 05:02:20PM -0400, Arvind Sankar wrote: On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: Ok... I just realized this does not work for a modular initramfs, composed at load time from multiple files, which is a very real problem. Should be easy enough to deal with: instead of one large file, use one companion file per source file, perhaps something like filename..xattrs (suggesting double dots to make it less likely to conflict with a "real" file.) No leading dot, as it makes it more likely that archivers will sort them before the file proper. This version of the patch was changed from the previous one exactly to deal with this case -- it allows for the bootloader to load multiple initramfs archives, each with its own .xattr-list file, and to have that work properly. Could you elaborate on the issue that you see? Roberto, are you missing a changelog entry for v2->v3 change? The changelog for v1->v2 is missing. Thanks Roberto -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Jian LI, Yanli SHI
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
Hi Roberto, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v5.1 next-20190517] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/initramfs-set-extended-attributes/20190518-055846 reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag Reported-by: kbuild test robot sparse warnings: (new ones prefixed by >>) init/initramfs.c:24:45: sparse: sparse: incorrect type in argument 2 (different address spaces) @@expected char const [noderef] *buf @@ got f] *buf @@ init/initramfs.c:24:45: sparse:expected char const [noderef] *buf init/initramfs.c:24:45: sparse:got char const *p init/initramfs.c:115:36: sparse: sparse: incorrect type in argument 2 (different address spaces) @@expected char const [noderef] *filename @@got n:1> *filename @@ init/initramfs.c:115:36: sparse:expected char const [noderef] *filename init/initramfs.c:115:36: sparse:got char *filename init/initramfs.c:303:24: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *name @@ got n:1> *name @@ init/initramfs.c:303:24: sparse:expected char const [noderef] *name init/initramfs.c:303:24: sparse:got char *path init/initramfs.c:305:36: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *pathname @@got n:1> *pathname @@ init/initramfs.c:305:36: sparse:expected char const [noderef] *pathname init/initramfs.c:305:36: sparse:got char *path init/initramfs.c:307:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *pathname @@got n:1> *pathname @@ init/initramfs.c:307:37: sparse:expected char const [noderef] *pathname init/initramfs.c:307:37: sparse:got char *path init/initramfs.c:317:43: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *oldname @@got n:1> *oldname @@ init/initramfs.c:317:43: sparse:expected char const [noderef] *oldname init/initramfs.c:317:43: sparse:got char *old init/initramfs.c:317:48: sparse: sparse: incorrect type in argument 2 (different address spaces) @@expected char const [noderef] *newname @@got char char const [noderef] *newname @@ init/initramfs.c:317:48: sparse:expected char const [noderef] *newname init/initramfs.c:317:48: sparse:got char *static [toplevel] [assigned] collected init/initramfs.c:404:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *name @@ got n:1> *name @@ init/initramfs.c:404:25: sparse:expected char const [noderef] *name init/initramfs.c:404:25: sparse:got char * >> init/initramfs.c:490:32: sparse: sparse: incorrect type in argument 1 >> (different address spaces) @@expected char const [noderef] *name >> @@got char char const [noderef] *name @@ init/initramfs.c:490:32: sparse:expected char const [noderef] *name init/initramfs.c:490:32: sparse:got char *static [toplevel] [assigned] collected init/initramfs.c:500:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *filename @@got char char const [noderef] *filename @@ init/initramfs.c:500:41: sparse:expected char const [noderef] *filename init/initramfs.c:500:41: sparse:got char *static [toplevel] [assigned] collected init/initramfs.c:512:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *pathname @@got char char const [noderef] *pathname @@ init/initramfs.c:512:28: sparse:expected char const [noderef] *pathname init/initramfs.c:512:28: sparse:got char *static [toplevel] [assigned] collected init/initramfs.c:513:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *filename @@got char char const [noderef] *filename @@ init/initramfs.c:513:28: sparse:expected char const [noderef] *filename init/initramfs.c:513:28: sparse:got char *static [toplevel] [assigned] collected init/initramfs.c:514:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@expected char const [noderef] *filename @@got char char const [noderef] *filename @@ init/initramfs.c:514:28: sparse:expected char const [noderef] *filename init/initramfs.c:514:28: sparse:got char *static [toplevel] [assigned] collected
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/17/19 4:41 PM, H. Peter Anvin wrote: > On 5/17/19 1:18 PM, h...@zytor.com wrote: >> >> Ok... I just realized this does not work for a modular initramfs, composed >> at load time from multiple files, which is a very real problem. Should be >> easy enough to deal with: instead of one large file, use one companion file >> per source file, perhaps something like filename..xattrs (suggesting double >> dots to make it less likely to conflict with a "real" file.) No leading dot, >> as it makes it more likely that archivers will sort them before the file >> proper. >> >> A side benefit is that the format can be simpler as there is no need to >> encode the filename. >> >> A technically cleaner solution still, but which would need archiver >> modifications, would be to encode the xattrs as an optionally nameless file >> (just an empty string) with a new file mode value, immediately following the >> original file. The advantage there is that the archiver itself could support >> xattrs and other extended metadata (which has been requested elsewhere); the >> disadvantage obviously is that that it requires new support in the archiver. >> However, at least it ought to be simpler since it is still a higher protocol >> level than the cpio archive itself. >> >> There's already one special case in cpio, which is the "!!!TRAILER!!!" >> filename; although I don't think it is part of the formal spec, to the >> extent there is one, I would expect that in practice it is always encoded >> with a mode of 0, which incidentally could be used to unbreak the case where >> such a filename actually exists. So one way to support such extended >> metadata would be to set mode to 0 and use the filename to encode the type >> of metadata. I wonder how existing GNU or BSD cpio (the BSD one is better >> maintained these days) would deal with reading such a file; it would at >> least not be a regression if it just read it still, possibly with warnings. >> It could also be possible to use bits 17:16 in the mode, which are >> traditionally always zero (mode_t being 16 bits), but I believe are present >> in most or all of the cpio formats for historical reasons. It might be >> accepted better by existing implementations to use one of these high bits >> combined with S_IFREG, I dont know. > > > Correction: it's just !!!TRAILER!!!. We documented it as "TRAILER!!!" without leading !!!, and that its purpose is to flush hardlinks: https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt That's what toybox cpio has been producing. Kernel consumes it just fine. Just checked busybox cpio and that's what they're producing as well... Rob
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
Hi Roberto,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.1 next-20190517]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Roberto-Sassu/initramfs-set-extended-attributes/20190518-055846
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All error/warnings (new ones prefixed by >>):
init/initramfs.c: In function 'do_readxattrs':
init/initramfs.c:437:10: error: implicit declaration of function 'vmalloc';
did you mean 'kvmalloc'? [-Werror=implicit-function-declaration]
path = vmalloc(file_entry_size);
^~~
kvmalloc
init/initramfs.c:437:8: warning: assignment makes pointer from integer
without a cast [-Wint-conversion]
path = vmalloc(file_entry_size);
^
init/initramfs.c:461:3: error: implicit declaration of function 'vfree'; did
you mean 'kvfree'? [-Werror=implicit-function-declaration]
vfree(path);
^
kvfree
In file included from include/asm-generic/io.h:891:0,
from arch/m68k/include/asm/io.h:11,
from include/linux/kexec.h:19,
from init/initramfs.c:694:
include/linux/vmalloc.h: At top level:
>> include/linux/vmalloc.h:77:14: error: conflicting types for 'vmalloc'
extern void *vmalloc(unsigned long size);
^~~
init/initramfs.c:437:10: note: previous implicit declaration of 'vmalloc'
was here
path = vmalloc(file_entry_size);
^~~
In file included from include/asm-generic/io.h:891:0,
from arch/m68k/include/asm/io.h:11,
from include/linux/kexec.h:19,
from init/initramfs.c:694:
>> include/linux/vmalloc.h:102:13: warning: conflicting types for 'vfree'
extern void vfree(const void *addr);
^
init/initramfs.c:461:3: note: previous implicit declaration of 'vfree' was
here
vfree(path);
^
cc1: some warnings being treated as errors
vim +/vmalloc +77 include/linux/vmalloc.h
db64fe02 Nick Piggin 2008-10-18 76
^1da177e Linus Torvalds2005-04-16 @77 extern void *vmalloc(unsigned long
size);
e1ca7788 Dave Young2010-10-26 78 extern void *vzalloc(unsigned long
size);
83342314 Nick Piggin 2006-06-23 79 extern void *vmalloc_user(unsigned
long size);
930fc45a Christoph Lameter 2005-10-29 80 extern void *vmalloc_node(unsigned
long size, int node);
e1ca7788 Dave Young2010-10-26 81 extern void *vzalloc_node(unsigned
long size, int node);
^1da177e Linus Torvalds2005-04-16 82 extern void *vmalloc_exec(unsigned
long size);
^1da177e Linus Torvalds2005-04-16 83 extern void *vmalloc_32(unsigned
long size);
83342314 Nick Piggin 2006-06-23 84 extern void
*vmalloc_32_user(unsigned long size);
dd0fc66f Al Viro 2005-10-07 85 extern void *__vmalloc(unsigned
long size, gfp_t gfp_mask, pgprot_t prot);
d0a21265 David Rientjes2011-01-13 86 extern void
*__vmalloc_node_range(unsigned long size, unsigned long align,
d0a21265 David Rientjes2011-01-13 87 unsigned long
start, unsigned long end, gfp_t gfp_mask,
cb9e3c29 Andrey Ryabinin 2015-02-13 88 pgprot_t prot,
unsigned long vm_flags, int node,
cb9e3c29 Andrey Ryabinin 2015-02-13 89 const void
*caller);
1f5307b1 Michal Hocko 2017-05-08 90 #ifndef CONFIG_MMU
a7c3e901 Michal Hocko 2017-05-08 91 extern void
*__vmalloc_node_flags(unsigned long size, int node, gfp_t flags);
8594a21c Michal Hocko 2017-05-12 92 static inline void
*__vmalloc_node_flags_caller(unsigned long size, int node,
8594a21c Michal Hocko 2017-05-12 93
gfp_t flags, void *caller)
1f5307b1 Michal Hocko 2017-05-08 94 {
8594a21c Michal Hocko 2017-05-12 95 return
__vmalloc_node_flags(size, node, flags);
1f5307b1 Michal Hocko 2017-05-08 96 }
8594a21c Michal Hocko 2017-05-12 97 #else
8594a21c Michal Hocko 2017-05-12 98 extern void
*__vmalloc_node_flags_caller(unsigned long size,
8594a21c Michal Hocko 2017-05-12 99
int node, gfp_t flags, void *caller);
1f5307b1 Michal Hocko 2017-05-08 100 #endif
cb9e3c29 Andrey Ryabinin 2015-02-13 101
b3bdda02 Christoph Lameter 2008-02-04 @102 extern void vfree(const void *addr);
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
Hi Roberto,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.1 next-20190517]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Roberto-Sassu/initramfs-set-extended-attributes/20190518-055846
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sparc64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All error/warnings (new ones prefixed by >>):
init/initramfs.c: In function 'do_readxattrs':
>> init/initramfs.c:437:10: error: implicit declaration of function 'vmalloc';
>> did you mean 'kvmalloc'? [-Werror=implicit-function-declaration]
path = vmalloc(file_entry_size);
^~~
kvmalloc
>> init/initramfs.c:437:8: warning: assignment makes pointer from integer
>> without a cast [-Wint-conversion]
path = vmalloc(file_entry_size);
^
>> init/initramfs.c:461:3: error: implicit declaration of function 'vfree'; did
>> you mean 'kvfree'? [-Werror=implicit-function-declaration]
vfree(path);
^
kvfree
cc1: some warnings being treated as errors
vim +437 init/initramfs.c
391
392 static int __init do_readxattrs(void)
393 {
394 struct path_hdr hdr;
395 char *path = NULL;
396 char str[sizeof(hdr.p_size) + 1];
397 unsigned long file_entry_size;
398 size_t size, path_size, total_size;
399 struct kstat st;
400 struct file *file;
401 loff_t pos;
402 int ret;
403
404 ret = vfs_lstat(XATTR_LIST_FILENAME, );
405 if (ret < 0)
406 return ret;
407
408 total_size = st.size;
409
410 file = filp_open(XATTR_LIST_FILENAME, O_RDONLY, 0);
411 if (IS_ERR(file))
412 return PTR_ERR(file);
413
414 pos = file->f_pos;
415
416 while (total_size) {
417 size = kernel_read(file, (char *), sizeof(hdr),
);
418 if (size != sizeof(hdr)) {
419 ret = -EIO;
420 goto out;
421 }
422
423 total_size -= size;
424
425 str[sizeof(hdr.p_size)] = 0;
426 memcpy(str, hdr.p_size, sizeof(hdr.p_size));
427 ret = kstrtoul(str, 16, _entry_size);
428 if (ret < 0)
429 goto out;
430
431 file_entry_size -= sizeof(sizeof(hdr.p_size));
432 if (file_entry_size > total_size) {
433 ret = -EINVAL;
434 goto out;
435 }
436
> 437 path = vmalloc(file_entry_size);
438 if (!path) {
439 ret = -ENOMEM;
440 goto out;
441 }
442
443 size = kernel_read(file, path, file_entry_size, );
444 if (size != file_entry_size) {
445 ret = -EIO;
446 goto out_free;
447 }
448
449 total_size -= size;
450
451 path_size = strnlen(path, file_entry_size);
452 if (path_size == file_entry_size) {
453 ret = -EINVAL;
454 goto out_free;
455 }
456
457 xattr_buf = path + path_size + 1;
458 xattr_len = file_entry_size - path_size - 1;
459
460 ret = do_setxattrs(path);
> 461 vfree(path);
462 path = NULL;
463
464 if (ret < 0)
465 break;
466 }
467 out_free:
468 vfree(path);
469 out:
470 fput(file);
471
472 if (ret < 0)
473 error("Unable to parse xattrs");
474
475 return ret;
476 }
477
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote: > On 5/17/19 2:02 PM, Arvind Sankar wrote: > > On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: > >> > >> Ok... I just realized this does not work for a modular initramfs, composed > >> at load time from multiple files, which is a very real problem. Should be > >> easy enough to deal with: instead of one large file, use one companion > >> file per source file, perhaps something like filename..xattrs (suggesting > >> double dots to make it less likely to conflict with a "real" file.) No > >> leading dot, as it makes it more likely that archivers will sort them > >> before the file proper. > > This version of the patch was changed from the previous one exactly to deal > > with this case -- > > it allows for the bootloader to load multiple initramfs archives, each > > with its own .xattr-list file, and to have that work properly. > > Could you elaborate on the issue that you see? > > > > Well, for one thing, how do you define "cpio archive", each with its own > .xattr-list file? Second, that would seem to depend on the ordering, no, > in which case you depend critically on .xattr-list file following the > files, which most archivers won't do. > > Either way it seems cleaner to have this per file; especially if/as it > can be done without actually mucking up the format. > > I need to run, but I'll post a more detailed explanation of what I did > in a little bit. > > -hpa > Not sure what you mean by how do I define it? Each cpio archive will contain its own .xattr-list file with signatures for the files within it, that was the idea. You need to review the code more closely I think -- it does not depend on the .xattr-list file following the files to which it applies. The code first extracts .xattr-list as though it was a regular file. If a later dupe shows up (presumably from a second archive, although the patch will actually allow a second one in the same archive), it will then process the existing .xattr-list file and apply the attributes listed within it. It then will proceed to read the second one and overwrite the first one with it (this is the normal behaviour in the kernel cpio parser). At the end once all the archives have been extracted, if there is an .xattr-list file in the rootfs it will be parsed (it would've been the last one encountered, which hasn't been parsed yet, just extracted). Regarding the idea to use the high 16 bits of the mode field in the header that's another possibility. It would just require additional support in the program that actually creates the archive though, which the current patch doesn't.
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/17/19 2:02 PM, Arvind Sankar wrote: > On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: >> >> Ok... I just realized this does not work for a modular initramfs, composed >> at load time from multiple files, which is a very real problem. Should be >> easy enough to deal with: instead of one large file, use one companion file >> per source file, perhaps something like filename..xattrs (suggesting double >> dots to make it less likely to conflict with a "real" file.) No leading dot, >> as it makes it more likely that archivers will sort them before the file >> proper. > This version of the patch was changed from the previous one exactly to deal > with this case -- > it allows for the bootloader to load multiple initramfs archives, each > with its own .xattr-list file, and to have that work properly. > Could you elaborate on the issue that you see? > Well, for one thing, how do you define "cpio archive", each with its own .xattr-list file? Second, that would seem to depend on the ordering, no, in which case you depend critically on .xattr-list file following the files, which most archivers won't do. Either way it seems cleaner to have this per file; especially if/as it can be done without actually mucking up the format. I need to run, but I'll post a more detailed explanation of what I did in a little bit. -hpa
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/17/19 1:18 PM, h...@zytor.com wrote: > > Ok... I just realized this does not work for a modular initramfs, composed at > load time from multiple files, which is a very real problem. Should be easy > enough to deal with: instead of one large file, use one companion file per > source file, perhaps something like filename..xattrs (suggesting double dots > to make it less likely to conflict with a "real" file.) No leading dot, as it > makes it more likely that archivers will sort them before the file proper. > > A side benefit is that the format can be simpler as there is no need to > encode the filename. > > A technically cleaner solution still, but which would need archiver > modifications, would be to encode the xattrs as an optionally nameless file > (just an empty string) with a new file mode value, immediately following the > original file. The advantage there is that the archiver itself could support > xattrs and other extended metadata (which has been requested elsewhere); the > disadvantage obviously is that that it requires new support in the archiver. > However, at least it ought to be simpler since it is still a higher protocol > level than the cpio archive itself. > > There's already one special case in cpio, which is the "!!!TRAILER!!!" > filename; although I don't think it is part of the formal spec, to the extent > there is one, I would expect that in practice it is always encoded with a > mode of 0, which incidentally could be used to unbreak the case where such a > filename actually exists. So one way to support such extended metadata would > be to set mode to 0 and use the filename to encode the type of metadata. I > wonder how existing GNU or BSD cpio (the BSD one is better maintained these > days) would deal with reading such a file; it would at least not be a > regression if it just read it still, possibly with warnings. It could also be > possible to use bits 17:16 in the mode, which are traditionally always zero > (mode_t being 16 bits), but I believe are present in most or all of the cpio > formats for historical reasons. It might be accepted better by existing > implementations to use one of these high bits combined with S_IFREG, I dont > know. > Correction: it's just !!!TRAILER!!!. I tested with GNU cpio, BSD cpio, scpio and pax. With a mode of 0: - GNU cpio errors, but extracts all the other files. - BSD cpio extracts them as regular files. - scpio and pax abort. With a mode of 0x18000 (bit 16 + S_IFREG), all of them happily extracted the data as regular files. -hpa
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On 5/17/19 3:18 PM, h...@zytor.com wrote: > Ok... I just realized this does not work for a modular initramfs, composed at > load time from multiple files, which is a very real problem. Should be easy > enough to deal with: instead of one large file, use one companion file per > source file, perhaps something like filename..xattrs (suggesting double dots > to make it less likely to conflict with a "real" file.) No leading dot, as it > makes it more likely that archivers will sort them before the file proper. > > A side benefit is that the format can be simpler as there is no need to > encode the filename. > > A technically cleaner solution still, but which would need archiver > modifications, would be to encode the xattrs as an optionally nameless file > (just an empty string) with a new file mode value, immediately following the > original file. The advantage there is that the archiver itself could support > xattrs and other extended metadata (which has been requested elsewhere); the > disadvantage obviously is that that it requires new support in the archiver. > However, at least it ought to be simpler since it is still a higher protocol > level than the cpio archive itself. > > There's already one special case in cpio, which is the "!!!TRAILER!!!" > filename; although I don't think it is part of the formal spec, to the extent > there is one, I would expect that in practice it is always encoded with a > mode of 0, which incidentally could be used to unbreak the case where such a > filename actually exists. So one way to support such extended metadata would > be to set mode to 0 and use the filename to encode the type of metadata. I > wonder how existing GNU or BSD cpio (the BSD one is better maintained these > days) would deal with reading such a file; it would at least not be a > regression if it just read it still, possibly with warnings. It could also be > possible to use bits 17:16 in the mode, which are traditionally always zero > (mode_t being 16 bits), but I believe are present in most or all of the cpio > formats for historical reasons. It might be accepted better by existing > implementations to use one of these high bits combined with S_IFREG, I dont > know. > I'll happily modify toybox cpio to understand xattrs (compress and decompress), the android guys do a lot with xattrs already. I tapped out of _this_ discussion from disgust with the proposed encoding. Rob
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On Fri, May 17, 2019 at 05:02:20PM -0400, Arvind Sankar wrote: > On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: > > > > Ok... I just realized this does not work for a modular initramfs, composed > > at load time from multiple files, which is a very real problem. Should be > > easy enough to deal with: instead of one large file, use one companion file > > per source file, perhaps something like filename..xattrs (suggesting double > > dots to make it less likely to conflict with a "real" file.) No leading > > dot, as it makes it more likely that archivers will sort them before the > > file proper. > This version of the patch was changed from the previous one exactly to deal > with this case -- > it allows for the bootloader to load multiple initramfs archives, each > with its own .xattr-list file, and to have that work properly. > Could you elaborate on the issue that you see? Roberto, are you missing a changelog entry for v2->v3 change?
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On Fri, May 17, 2019 at 01:18:11PM -0700, h...@zytor.com wrote: > > Ok... I just realized this does not work for a modular initramfs, composed at > load time from multiple files, which is a very real problem. Should be easy > enough to deal with: instead of one large file, use one companion file per > source file, perhaps something like filename..xattrs (suggesting double dots > to make it less likely to conflict with a "real" file.) No leading dot, as it > makes it more likely that archivers will sort them before the file proper. This version of the patch was changed from the previous one exactly to deal with this case -- it allows for the bootloader to load multiple initramfs archives, each with its own .xattr-list file, and to have that work properly. Could you elaborate on the issue that you see?
Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
On May 17, 2019 9:55:19 AM PDT, Roberto Sassu wrote:
>This patch adds support for an alternative method to add xattrs to
>files in
>the rootfs filesystem. Instead of extracting them directly from the ram
>disk image, they are extracted from a regular file called .xattr-list,
>that
>can be added by any ram disk generator available today. The file format
>is:
>
>\0
>\0
>
>.xattr-list can be generated by executing:
>
>$ getfattr --absolute-names -d -h -R -e hex -m - \
> | xattr.awk -b > ${initdir}/.xattr-list
>
>where the content of the xattr.awk script is:
>
>#! /usr/bin/awk -f
>{
> if (!length($0)) {
>printf("%.10x%s\0", len, file);
>for (x in xattr) {
> printf("%.8x%s\0", xattr_len[x], x);
> for (i = 0; i < length(xattr[x]) / 2; i++) {
>printf("%c", strtonum("0x"substr(xattr[x], i * 2 + 1, 2)));
> }
>}
>i = 0;
>delete xattr;
>delete xattr_len;
>next;
> };
> if (i == 0) {
>file=$3;
>len=length(file) + 8 + 1;
> }
> if (i > 0) {
>split($0, a, "=");
>xattr[a[1]]=substr(a[2], 3);
>xattr_len[a[1]]=length(a[1]) + 1 + 8 + length(xattr[a[1]]) / 2;
>len+=xattr_len[a[1]];
> };
> i++;
>}
>
>Signed-off-by: Roberto Sassu
>---
> init/initramfs.c | 99
> 1 file changed, 99 insertions(+)
>
>diff --git a/init/initramfs.c b/init/initramfs.c
>index 0c6dd1d5d3f6..6ec018c6279a 100644
>--- a/init/initramfs.c
>+++ b/init/initramfs.c
>@@ -13,6 +13,8 @@
> #include
> #include
>
>+#define XATTR_LIST_FILENAME ".xattr-list"
>+
> static ssize_t __init xwrite(int fd, const char *p, size_t count)
> {
> ssize_t out = 0;
>@@ -382,6 +384,97 @@ static int __init __maybe_unused do_setxattrs(char
>*pathname)
> return 0;
> }
>
>+struct path_hdr {
>+ char p_size[10]; /* total size including p_size field */
>+ char p_data[]; /* \0 */
>+};
>+
>+static int __init do_readxattrs(void)
>+{
>+ struct path_hdr hdr;
>+ char *path = NULL;
>+ char str[sizeof(hdr.p_size) + 1];
>+ unsigned long file_entry_size;
>+ size_t size, path_size, total_size;
>+ struct kstat st;
>+ struct file *file;
>+ loff_t pos;
>+ int ret;
>+
>+ ret = vfs_lstat(XATTR_LIST_FILENAME, );
>+ if (ret < 0)
>+ return ret;
>+
>+ total_size = st.size;
>+
>+ file = filp_open(XATTR_LIST_FILENAME, O_RDONLY, 0);
>+ if (IS_ERR(file))
>+ return PTR_ERR(file);
>+
>+ pos = file->f_pos;
>+
>+ while (total_size) {
>+ size = kernel_read(file, (char *), sizeof(hdr), );
>+ if (size != sizeof(hdr)) {
>+ ret = -EIO;
>+ goto out;
>+ }
>+
>+ total_size -= size;
>+
>+ str[sizeof(hdr.p_size)] = 0;
>+ memcpy(str, hdr.p_size, sizeof(hdr.p_size));
>+ ret = kstrtoul(str, 16, _entry_size);
>+ if (ret < 0)
>+ goto out;
>+
>+ file_entry_size -= sizeof(sizeof(hdr.p_size));
>+ if (file_entry_size > total_size) {
>+ ret = -EINVAL;
>+ goto out;
>+ }
>+
>+ path = vmalloc(file_entry_size);
>+ if (!path) {
>+ ret = -ENOMEM;
>+ goto out;
>+ }
>+
>+ size = kernel_read(file, path, file_entry_size, );
>+ if (size != file_entry_size) {
>+ ret = -EIO;
>+ goto out_free;
>+ }
>+
>+ total_size -= size;
>+
>+ path_size = strnlen(path, file_entry_size);
>+ if (path_size == file_entry_size) {
>+ ret = -EINVAL;
>+ goto out_free;
>+ }
>+
>+ xattr_buf = path + path_size + 1;
>+ xattr_len = file_entry_size - path_size - 1;
>+
>+ ret = do_setxattrs(path);
>+ vfree(path);
>+ path = NULL;
>+
>+ if (ret < 0)
>+ break;
>+ }
>+out_free:
>+ vfree(path);
>+out:
>+ fput(file);
>+
>+ if (ret < 0)
>+ error("Unable to parse xattrs");
>+
>+ return ret;
>+}
>+
> static __initdata int wfd;
>
> static int __init do_name(void)
>@@ -391,6 +484,11 @@ static int __init do_name(void)
> if (strcmp(collected, "TRAILER!!!") == 0) {
> free_hash();
> return 0;
>+ } else if (strcmp(collected, XATTR_LIST_FILENAME) == 0) {
>+ struct kstat st;
>+
>+ if (!vfs_lstat(collected, ))
>+ do_readxattrs();
> }
> clean_path(collected, mode);
> if (S_ISREG(mode)) {
>@@ -562,6 +660,7 @@ static char * __init unpack_to_rootfs(char *buf,
>unsigned long len)
> buf += my_inptr;
> len -= my_inptr;
> }
>+ do_readxattrs();
> dir_utime();
>
