The build is broken for redhat kernels:
https://github.com/linux-apfs/linux-apfs-rw/issues/89
This seems to have been the case for more than a year, but nobody had
reported it so far, so I guess the driver isn't getting much use over
there.
We had encountered a similar problem before in commit 1a0b9fb8af ("Fix
build for RHEL 9 kernels"), but back then I thought it was just a
one-off thing. I was wrong: redhat kernels often backport patches that
break internal apis. I don't know how other out-of-tree drivers handle
this, but for now I will have to rely on user reports.
So, go through all RHEL 9 kernels one by one, find out which api changes
were applied to each, and add the corresponding macro version checks to
the driver.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
As mentioned in the code, my implementation of apfs_compress_readpage()
was mostly copied from ext4_read_inline_page() because it was simple
enough and I didn't know what I was doing. As it turns out, I picked the
wrong function to copy: ext4_read_inline_page() only reads in-memory
data and doesn't go to sleep. We do read from disk, and we do go to
sleep, so newer kernels flood the console with warnings.
Replace kmap_atomic() with kmap(). The only expected negative effect of
this change would be worse performance on 32-bit archs, but I haven't
even tested those in years. The alternative kmap_local_page() is not
available in old kernels.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Some code that wasn't originally written by me has a style that differs
slightly from the rest of the module. Since I've been refactoring
compress.c quite a lot lately, I have made the inconsistencies more
visible, so I think it's a good time to switch everything to the typical
kernel style.
For this purpose I'm running checkpatch.pl for the first time in years,
and it has detected a handful of other issues that I introduced myself.
Fix most of the issues now, but I prefer to leave the zero-length array
stuff for a separate patch.
For the record, at the moment I'm choosing to ignore style inconsistency
in the compression libraries, even in libzbitmap which I wrote myself. I
prefer to keep them as close to upstream as possible.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
For some reason I never remembered to take the big lock during reads of
compressed files. Do it. As always, my locking is coarse and hacky, and
I will have get back to it at some point.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
After the recent refactoring of the compression code, the fail_enomem
label feels a little unnecessary. Get rid of it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
While debugging the previous patch, I noticed that my implementation of
compressed ->readpage() from f9ca9061bb ("Implement mmap for
compressed files") has a serious issue: if something goes wrong during
the read, the error code is swallowed and stale garbage becomes visible
to the user.
I messed this up because I was following ext4_read_inline_page(), which
first calls ext4_read_inline_data() and then, without checking the
return code, calls SetPageUptodate(). But ext4_read_inline_data() can't
actually fail, so this is not a problem. In my version the function
called is apfs_compress_file_read_page(), which does fail sometimes,
leaving a stale page marked as updated. Fix it, and also return the
error code correctly.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently, compressed files that are stored directly in the decmpfs
xattr are handled separately: they get entirely uncompressed on open,
and on reads the contents simply get copied. To get rid of this code
duplication, use a new apfs_compressed_data struct to read from both
inline and dstream xattrs, and let the regular resource-compressed code
handle reads for both types.
These changes weren't made exclusively for code quality. A compressed
file may be too big to fit inside decmpfs, but still small enough to fit
inside a xattr. If that happens, the resulting resource fork will be an
inline xattr, and the dstream read code implemented with e4bb06ecef
("Avoid reading whole compressed files into memory") will break down. So
the apfs_compressed_data struct is strictly necessary for the resource
case; using the same struct for both cases simplifies testing, because
inline resource compressed files are rare (I've seen them, but right now
I don't even have an image with one of them).
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
After commit e4bb06ecef ("Avoid reading whole compressed files into
memory"), performance for compressed reads worsened by about 50%. The
problem is that, when we stopped reading the xattr all at once, we lost
the advantage of letting the kernel batch and schedule the buffer reads.
The ideal way to handle this would be to make a fake inode for the xattr
dstream, and let the kernel do readahead in the usual manner. I don't
feel very confident that I can get that right though, so I'm leaving it
for some other time.
For now, just request all the blocks as soon as a read begins (I would
have preferred to do it on open, but that's slower for some reason), and
ignore the results. Compressed data is supposed to be small so, as long
as there is enough memory, this has all the performance benefits of the
original implementation (in fact it's somewhat faster), but it still
avoids the problems with large virtual memory allocation.
Of course, unlike normal readahead, this will be useless if memory is
scarce, or if we don't read the whole file sequentially.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In commit e4bb06ecef ("Avoid reading whole compressed files into
memory"), I moved part of the code of apfs_compress_file_read_block()
into a separate apfs_compress_file_read_from_block() that must call it.
I made a mistake, though: the original version of the function would
reset the return value before returning, but this version no longer does
it. This doesn't play well with the decompression functions for some of
the algorithms, which return decompressed lengths. So we get a "failed
to read block into buffer" message and the decompression is abandoned.
Fix this.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When implementing compressed mmap I was forced to duplicate the code for
regular compressed reads, so that one version could copy to a user
buffer and the other to the kernel. To simplify this, get rid of the
regular version and instead use the generic read iterator, which will
make use of the same mmap code.
This should also allow regular compressed reads to take advantage of the
page cache.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
When compressed mmap was implemented, apfs_compress_file_mmap() was
added with the purpose of ignoring calls to ->page_mkwrite(). With the
last patch, I no longer allow compressed files to be opened for writing
at all, so I can just use regular ->mmap() for all files.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In commit 31df49c6d8 ("Ban writes to compressed files explicitly"), I
blocked writes to compressed files when they actually happen, not when
the file gets opened. My fear was that banning rw open entirely could
break other people's code, since I'm not sure who is using this driver
and how.
Now I've implemented compressed mmap, and blocking writes when they
happen is becoming a little too hacky. Do this properly, and wait to see
if anyone complains.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Attempts to mmap compressed files currently fail with ENODEV. I guess I
had forgotten about compressed mmap. Implement it now, in read-only mode
like all compressed operations so far.
Since the existing read code works with userland buffers, it can't be
reused to copy stuff into the kernel. For now I've been forced to
duplicate a lot of code, but I think it might be a good idea to use the
generic read implementations instead. I'll look into that soon.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The official implementation doesn't seem to truly support writes to
compressed files: when it's attempted, the file gets decompressed
transparently instead. I don't intend to that in this driver, unless
someone actually requests it. Users can just make a decompressed copy of
the file and replace the original.
By mistake, truncating compressed files is currently allowed, but since
compressed reads don't depend on the vfs-reported size, nothing seems to
happen. Other writes fail, but the error code is confusing and there is
no output on dmesg. Ban all writes explicitly, and explain the problem
in the console.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Currently, compressed files saved in xattrs are read into memory all at
once, and later decompressed as needed. This has always required a large
memory allocation, which can be a problem in general, but especially for
32-bit archs which can't request more than 128Mb from vmalloc(). As
mentioned in commit 1e3fa28c4f ("Increase limit for compressed size
of files"), we are beginning to encounter files bigger than that limit.
Using the new apfs_nonsparse_dstream_read() to read from arbitrary xattr
offsets, rework the compressed reads so that we only allocate memory for
one compressed block at a time. For now the change is only for resource
xattrs, since I don't expect inline xattrs to ever be this big.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
A recent cryptex image contains a file with a compressed size above the
current 128Mb limit. This limit exists because of vmalloc(), and it can
be increased (at least for 64-bit archs). Do that for now, but I really
should try to restructure the code to use smaller kmalloc() allocations
instead.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The driver is much closer to being usable, so I might start getting
subtler bug reports soon. To make them easier to handle, put error
messages all over the place. I should have done this from the beginning,
but I guess I didn't fully understand the need back then.
From now my general policy will be to use apfs_warn() for user errors or
unsupported features; apfs_err() for things that are probably corruption
or io errors; and apfs_alert() for things that are most likely bugs.
These last two should be rare, so the same error/alert will be thrown by
several layers in the callstack to provide as much information as
possible. Be careful and don't flood the console on normal situations.
Also, make messages with a log level lower than warning output their
function name and line number, which I think will help debugging more
than the actual messages.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Cleanup on some errors is incorrect for LZBITMAP attr compression. This
error came from copy-paste of the resource version of the code. Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Support for LZBITMAP was previously limited to resource compression,
simply because I had never encountered any attr-compressed files that I
could use for testing. Using the same tool from the previous patch[0]
and one of our iphone vms, I now managed to create one. Add support.
[0]: https://github.com/Siguza/fscmp
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Using the official LZFSE library (which I already dumped here in the
previous patch), add support for LZFSE compression. For testing I used
images created with the following tool:
https://github.com/Siguza/fscmp
I've tested both resource and attribute compression. For resource
compression, I also tested a file that had a single uncompressed block.
That block had 0xff in the first byte, but I don't know if that's a
general rule so it's better to just check for the first byte of the
LZFSE magic.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Use Apple's official LZFSE library[0] to add support for LZVN
compression. Their code for LZFSE compression proper is also dumped with
this patch, but it's not actually rigged up for the driver because I
have no image to test it on.
For the record, the build on Linux 5.15 produces the following objtool
warning:
lzfse/lzvn_decode_base.o: warning: objtool: lzvn_decode()+0x1b0: sibling call from callable instruction with modified stack frame
I'm not sure what this means, but there seems to be no sibling call in
that position, just a regular goto:
1b0: e9 00 00 00 00 jmp 1b5 <lzvn_decode+0x1b5>
I'll ignore it for now, but I guess I should report it if it's a false
positive.
[0]: https://github.com/lzfse/lzfse
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
We support several compression algorithms now, and I intend to add more
soon, so clean up the checks a little bit. Also make it easier for users
to report new algorithms: print out the number to dmesg.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
There is a weird compression mode that is not actually compressed at
all, add support for it. This patch is just a copy of Stan's code for
our internal tool.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
