Add support for decoding erofs into lcfs_node, and some tooling using it #185

alexlarsson · 2023-09-08T11:53:18Z

This will help with fixing
#182

alexlarsson · 2023-09-11T08:26:09Z

I updated the composefs-info escaping to be a bit more strict.

The dump format is very simple and readable, yet efficiently streamable, I wonder if that is a good format for streaming data to mkcomposefs. Its maybe not super extensible though...

libcomposefs/lcfs-writer-erofs.c

tools/composefs-info.c

tools/composefs-dump.c

alexlarsson · 2023-09-12T13:34:04Z

Also, this PR needs work once #175 and #187 lands.

alexlarsson · 2023-09-13T15:10:27Z

Rebased on latest and fixed some minor issues. This is still not ready.

alexlarsson · 2023-09-13T15:18:38Z

TODO:

Make cfs-fuse and image-to-node handle the new inlined files
Share more code between cfs-fuse and image-to-node
Add some more testing
Handle encoded whiteouts

Signed-off-by: Alexander Larsson <[email protected]>

trusted.overlay.verity doesn't exist anymore, its just trusted.overlay.metacopy. Signed-off-by: Alexander Larsson <[email protected]>

Signed-off-by: Alexander Larsson <[email protected]>

Right now these are only used from cfs-fuse.c, but they will be reused more later. Signed-off-by: Alexander Larsson <[email protected]>

Signed-off-by: Alexander Larsson <[email protected]>

This allows a whiteout in the composefs mount be used as a whiteout both with a regular and an userxattr overlayfs mount. Signed-off-by: Alexander Larsson <[email protected]>

We want to be able to read back everything that we can set. Signed-off-by: Alexander Larsson <[email protected]>

This is useful to figure out the content of a composefs iamge. Signed-off-by: Alexander Larsson <[email protected]>

This lets you list the content of an image, and dump the objects it references. Signed-off-by: Alexander Larsson <[email protected]>

For all the images we generate from json, try also to regenerated them and ensure we get the same bits. To do the dumping we add the internal composefs-dump tool. Signed-off-by: Alexander Larsson <[email protected]>

This lets you test lcfs_node_set_content() Signed-off-by: Alexander Larsson <[email protected]>

Signed-off-by: Alexander Larsson <[email protected]>

This tesets various special things like device nodes, escaped xattrs whiteouts and inlined files. Signed-off-by: Alexander Larsson <[email protected]>

This may catch any weird stuff we might generate. Signed-off-by: Alexander Larsson <[email protected]>

Now uses more of the new helpers and code. Also supports inlined data. Signed-off-by: Alexander Larsson <[email protected]>

Signed-off-by: Alexander Larsson <[email protected]>

alexlarsson · 2023-09-14T16:01:09Z

I rebased on master, implemented missing features in fuse, generally cleaned up the code and added a few more tests.

I think with this in we're ready to freeze the on-disk format.

Once this is in I will focus on adding even more tests.

cgwalters

A lot of code here, I gave this a semi-skim, only found one actual minor bug (that can be fixed in a followup even); the rest is just nits/questions.

cgwalters · 2023-09-14T17:21:09Z

libcomposefs/lcfs-erofs-internal.h

+	res = malloc(prefix_len + name_len + 1);
+	if (res == NULL) {
+		errno = ENOMEM;
+		return NULL;
+	}
+	memcpy(res, prefix, prefix_len);
+	memcpy(res + prefix_len, name, name_len);
+	res[prefix_len + name_len] = 0;
+
+	return res;


This is all just return asprintf("%s%.*s", prefix, name_len, name) right?

Yeah, but I'm not super found of asprintf like this, because I always have to look up the length modifier in the manpage to get it right.

And also, I always worry about the integer format conversion in varargs calls like this.
Like, name_len is size_t, which could be larger than int in some compilers, and the field width must be int. So, do we need to cast? etc.

cgwalters · 2023-09-14T17:22:38Z

libcomposefs/lcfs-utils.h

+	return s2;
+}
+
+static inline char *str_join(const char *a, const char *b)


optional: This is just code motion, so OK, but same question re asprintf - can be a followup.

cgwalters · 2023-09-14T17:23:03Z

libcomposefs/lcfs-internal.h

@@ -29,6 +29,28 @@
 */
 #define LCFS_BUILD_INLINE_FILE_SIZE_LIMIT 64

+#define OVERLAY_XATTR_USER_PREFIX "user."


Aweseome, I think this helps a lot to have these. Then we can also followup with some links to the kernel sources.

cgwalters · 2023-09-14T17:24:12Z

libcomposefs/lcfs-writer-erofs.c

@@ -1116,12 +1116,20 @@ static int add_overlayfs_xattrs(struct lcfs_node_s *node)
 					  "", 0);
 		if (ret < 0)
 			return ret;
+		ret = lcfs_node_set_xattr(node, OVERLAY_XATTR_USERXATTR_WHITEOUT,


Hum, interesting...but can you help me understand this? Do we need the userxattrs for unprivileged overlayfs mounts? This seems tricky because it's redundant data in the privileged case, no? Or maybe it isn't much data and doesn't matter.

So, a traditional whiteout is a chardev(0,0), and having that in the lowerdir will work as a whiteout for both regular and userxattr overlayfs mounts. Unfortunately they don't work in composefs, because the outer overlayfs eats them.

So, we have a new type of whiteout. If we set an escaped whiteout xattr like trusted.overlay.overlay.whiteout on a file in the erofs, then in the overlayfs mount it will show up as trusted.overlay.whiteout, and a (new) regular overlayfs mount will treat it as a whiteout. However, to a userxattr overlayfs mount it will not be special in any way, because it looks for user.overlay.whiteout files. So, composefs needs to set both of these for the resulting file to always work as a whiteout.

In terms of redundancy. Its not ideal, but it is what we need, and I don't think we'll have tons of whiteouts in practice.

cgwalters · 2023-09-14T17:26:24Z

libcomposefs/lcfs-writer.c

+
+	node = lcfs_load_node_from_image(image_data, image_data_size);
+	if (node == NULL) {
+		errsv = errno;


We have a handy PROTECT_ERRNO macro btw.

cgwalters · 2023-09-14T17:27:42Z

tools/composefs-info.c

+
+static void digest_to_string(const uint8_t *csum, char *buf)
+{
+	static const char hexchars[] = "0123456789abcdef";


As far as I can tell, this would literally be the fourth copy of this in tree...

cgwalters · 2023-09-14T17:28:31Z

tools/composefs-info.c

+	get_objects(node, ht);
+
+	size_t n_objects = hash_get_n_entries(ht);
+	char **objects = calloc(n_objects, sizeof(char *));


Missing NULL check

cgwalters · 2023-09-14T17:29:41Z

tools/composefs-info.c

+	const char *bin = argv[0];
+	int fd;
+	cleanup_node struct lcfs_node_s *root = NULL;
+	const char *image_path = NULL;
+	const char *command;


So not really related to this exactly but...can I arm twist you into using declare-and-initialize style in this codebase? I find it much more readable than "big dump of variables at the top of the function". It plays more nicely with __attribute__(cleanup) too because there's no footgun around forgetting to init to NULL etc.

cgwalters · 2023-09-14T17:31:28Z

tools/cfs-fuse.c

+	const erofs_inode *cino = cfs_get_erofs_inode(ino);
+	uint32_t mode;
+	uint64_t file_size;
+	uint16_t xattr_icount;
+	size_t xattr_size;
+	size_t isize;
+	uint64_t n_blocks;
+	uint64_t last_oob_block;
+	bool tailpacked;
+	size_t tail_size;
+	uint32_t raw_blkaddr;
+	off_t oob_size;
+	const uint8_t *tail_data;
+	const uint8_t *oob_data;
+	struct iovec iov[2];
+	int i;


(IMO, this is crying out for declare-and-initialize...)

Not to completely belabor this point but...basically all other sane modern languages allow declaring (and initializing) a variable inside a function.

In the case of this particular function, it would probably be half the size if done this way.

I know you work on the Linux kernel which isn't yet using this style, and I know even some others like systemd haven't really adopted it but IMO it really just makes things much more readable. Particularly in this example!

Actually, the kernel is moving toward using attribute(cleanup), and as a result will be moving towards this style too (at least partially)

https://lwn.net/ml/linux-kernel/CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com/

I was just reading the code and I have a mental checklist item for "invoking open without O_CLOEXEC" that triggered here. (See also e.g. containers/composefs#185 (comment) ) It has security-relevant properties for us, xref CVE-2024-21626 for example. This isn't the only missing variant of this in this codebase, just using this targeted PR to test the waters for more PRs. Signed-off-by: Colin Walters <[email protected]>

alexlarsson force-pushed the decode-erofs branch from 538c031 to b15de57 Compare September 11, 2023 08:23

alexlarsson force-pushed the decode-erofs branch from b15de57 to 512aff6 Compare September 11, 2023 09:18

cgwalters reviewed Sep 12, 2023

View reviewed changes

alexlarsson force-pushed the decode-erofs branch from 512aff6 to 8fbe163 Compare September 13, 2023 15:10

alexlarsson force-pushed the decode-erofs branch from 8fbe163 to 1d2e384 Compare September 13, 2023 15:16

alexlarsson marked this pull request as draft September 13, 2023 15:17

alexlarsson added 19 commits September 14, 2023 12:06

tests/dumpdir: Print mode in octal, makes it easer to read

7fa6107

Signed-off-by: Alexander Larsson <[email protected]>

Fix minor distcheck issues

1c69f93

Signed-off-by: Alexander Larsson <[email protected]>

lib: Drop reference to unused overlayfs xattr

1b0e9f3

trusted.overlay.verity doesn't exist anymore, its just trusted.overlay.metacopy. Signed-off-by: Alexander Larsson <[email protected]>

lcfs-internal.h; Add cleanup_node macro

fc30bb6

Signed-off-by: Alexander Larsson <[email protected]>

Use cleanup_node to simplify some code

6ca622f

Signed-off-by: Alexander Larsson <[email protected]>

Move erofs helpers to new lcfs-erofs-internal.h

ccb7374

Right now these are only used from cfs-fuse.c, but they will be reused more later. Signed-off-by: Alexander Larsson <[email protected]>

Move some utility functions to lcfs-utils.h

55067ad

Signed-off-by: Alexander Larsson <[email protected]>

Add (and use) defines for overlayfs xattr names

d186f4c

Signed-off-by: Alexander Larsson <[email protected]>

When escaping whiteouts, also set the userxattrs xatrs.

fe81a66

This allows a whiteout in the composefs mount be used as a whiteout both with a regular and an userxattr overlayfs mount. Signed-off-by: Alexander Larsson <[email protected]>

lib: Add lcfs_node_get_payload/hardlink_target()

30bcbe5

We want to be able to read back everything that we can set. Signed-off-by: Alexander Larsson <[email protected]>

lib: Add support to load image to lcfs_node

0366de5

This is useful to figure out the content of a composefs iamge. Signed-off-by: Alexander Larsson <[email protected]>

tools: Add composefs-info tool

cc73fbf

This lets you list the content of an image, and dump the objects it references. Signed-off-by: Alexander Larsson <[email protected]>

Test ability to reproduce parsed images

1340829

For all the images we generate from json, try also to regenerated them and ensure we get the same bits. To do the dumping we add the internal composefs-dump tool. Signed-off-by: Alexander Larsson <[email protected]>

composefs-from-json: Add new x-content key type

e958ded

This lets you test lcfs_node_set_content() Signed-off-by: Alexander Larsson <[email protected]>

Move tests from tools/test-assets to tests/assets

f0594cb

Signed-off-by: Alexander Larsson <[email protected]>

Add special.json test asset

90676ee

This tesets various special things like device nodes, escaped xattrs whiteouts and inlined files. Signed-off-by: Alexander Larsson <[email protected]>

tests: Run fsck.erofs (if available) on generated erofs

7c65eb0

This may catch any weird stuff we might generate. Signed-off-by: Alexander Larsson <[email protected]>

Update composefs-fuse

4110ef4

Now uses more of the new helpers and code. Also supports inlined data. Signed-off-by: Alexander Larsson <[email protected]>

Add some more unit tests to make check

fbdf4e4

Signed-off-by: Alexander Larsson <[email protected]>

alexlarsson force-pushed the decode-erofs branch from 1d2e384 to fbdf4e4 Compare September 14, 2023 15:59

alexlarsson marked this pull request as ready for review September 14, 2023 16:02

cgwalters approved these changes Sep 14, 2023

View reviewed changes

alexlarsson merged commit ca46d09 into containers:main Sep 15, 2023
8 checks passed

cgwalters mentioned this pull request Jun 4, 2024

chunked: Set O_CLOEXEC containers/storage#1947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for decoding erofs into lcfs_node, and some tooling using it #185

Add support for decoding erofs into lcfs_node, and some tooling using it #185

alexlarsson commented Sep 8, 2023

alexlarsson commented Sep 11, 2023

alexlarsson commented Sep 12, 2023

alexlarsson commented Sep 13, 2023

alexlarsson commented Sep 13, 2023 •

edited

Loading

alexlarsson commented Sep 14, 2023

cgwalters left a comment

cgwalters Sep 14, 2023

alexlarsson Sep 15, 2023

cgwalters Sep 14, 2023

cgwalters Sep 14, 2023

cgwalters Sep 14, 2023

alexlarsson Sep 15, 2023

cgwalters Sep 14, 2023

cgwalters Sep 14, 2023

alexlarsson Sep 15, 2023

cgwalters Sep 14, 2023

cgwalters Sep 14, 2023

alexlarsson Sep 15, 2023

cgwalters Sep 14, 2023

cgwalters Sep 14, 2023

alexlarsson Sep 15, 2023

alexlarsson Sep 15, 2023

Add support for decoding erofs into lcfs_node, and some tooling using it #185

Add support for decoding erofs into lcfs_node, and some tooling using it #185

Conversation

alexlarsson commented Sep 8, 2023

alexlarsson commented Sep 11, 2023

alexlarsson commented Sep 12, 2023

alexlarsson commented Sep 13, 2023

alexlarsson commented Sep 13, 2023 • edited Loading

alexlarsson commented Sep 14, 2023

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlarsson commented Sep 13, 2023 •

edited

Loading