If a function throws ENOSPC halfway through an operation, the
transaction will get aborted so that the filesystem isn't left in an
inconsistent state. This means that the mount will go read-only, and
that the user will lose any uncommitted changes.
So, it's better if we detect ENOSPC before starting an operation. I
tried this before with commit a52b73ed97 ("Check free space before
starting a transaction"), but the code was messy and it would have
required constant updates, which I neglected of course. The problem was
that I wanted to be as accurate as possible with my calculations of
required space, but I'm not sure there is any point.
Instead, require a large fixed amount of space (512 KiB) for most
operations, and a much smaller but still almost certainly sufficient
amount of space (80 KiB) for deletions. Hopefully this will be enough
for users to maneuver around ENOSPC situations. It's not perfect, but no
CoW filesystem can be truly perfect in this regard.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Provide a proper error message on dmesg when a user attempts to create a
snapshot with a name that's already in use, otherwise "File exists" may
be a bit confusing.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
A user has encountered a volume with the dataless snapshots flag:
https://github.com/linux-apfs/linux-apfs-rw/issues/86
I'm not really sure what "dataless snapshots" are, and so far I have
failed to trigger them under macos. I can set the
SNAP_META_PENDING_DATALESS flag in a snapshot myself, and macos will
call it "dataless", but their fsck doesn't complain no matter what I do
with the other fields, so I can't tell what is expected.
I can't imagine any risk in mounting such a volume read-only, so allow
that much from now on. Keep writes banned just in case, and avoid
mounting the dataless snapshots themselves, since I have no idea what
that entails.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Attempting to create two snapshots with the same name will abort a
transaction and leave the filesystem in a read-only state. From the
comment it seems that I was aware of this issue when I first implemented
snapshots, but I guess I didn't like the obvious solution in this patch
at the time, so I decided to leave it for later.
Since taking snapshots is not expected to be a bottleneck, just run the
query for the name once more ahead of time, as soon as the transaction
opens. Now if the name already exists we don't need to abort, because no
changes have taken place.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've been working on making some very needed changes to how node splits
work. The plan is to always rerun queries after a split, to keep the
query chain valid. One problem I've encountered while implementing this
is that the search key is not really preserved by apfs_update_inode().
We have a pointer to it, but it was allocated in the stack of some other
function, so the data is just noise.
I've always thought it was silly to allocate both a query and a key
before a lookup. Instead, turn the search key into a field within the
query structure, and copy it to the children as the query gets run. This
will ensure that the key remains available for future reruns.
One thing to note is that, while this simplifies most callers a little
bit, some of them still allocate a key in the stack and later copy it to
the query. I didn't want to make any big changes and risk introducing
bugs at this point, since I'm busy with other stuff.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
The build appears to be broken for some configurations on the Linux 6.5
release candidate:
https://github.com/linux-apfs/linux-apfs-rw/issues/49
Some sort of static overflow check is going on for strcpy(), and several
of my calls are triggering false positives. Just be safe and switch them
all to strscpy(). The qstr stuff in particular looked a bit too weird
anyway.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Don't call strlen() on the name argument of the ioctl, because it may
not be null-terminated. I'm not sure what the consequences of this are,
but I don't think it should be exploitable. Use strnlen() instead.
Also move the length check outside the transaction: we don't want simple
user mistakes triggering aborts. And fix the returned error code: there
is no on-disk corruption going on, just user error.
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>
It seems that I messed up the cleanups during some late fixups to the
code in apfs_do_ioc_take_snapshot(): the transaction could be left open
in some cases. Fix it.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Right now the "inum" field of new snapshot metadata record gets set to
zero, and the official fsck is complaining about it. I was confused for
a while about this field because I thought it referred to an actual file
somewhere, but now I'm pretty sure it's just an identifier for the
snapshot, which gets picked from the same pool as regular inode numbers.
So, do that.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
There are two bugs in the way apfs_fs_alloc_count is calculated after a
snapshot. I had not noticed them so far because it randomly gets the
right number as long as no node splits happen, but the new test I'm
working on (called apfs/006 for now) does split nodes and reports the
corruption.
Anyway, the problems are that the volume superblock should never get
counted in apfs_fs_alloc_count, not even after a snapshot; and that
regular CoW of virtual nodes needs to increase apfs_fs_alloc_count if
the original is preserved in a snapshot. Fix both now.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
I've been forgetting to run sparse lately and as a result I introduced a
few endianness bugs in commit 97863ab914 ("Add support for taking
snapshots"). Fix them.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In apfs_do_ioc_take_snapshot(), don't read the on-disk volume superblock
before opening the transaction, or else our pointer may become stale. I
noticed this by chance when looking at the code, but I already had some
assertions in place for this very situation, so I was probably going to
notice during testing eventually.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Define an in-memory omap structure to be shared by all mounts of the
same volume, including both the current transaction and the snapshots.
The on-disk omap is already shared by all of them, so this will make for
saner code.
The patch fixes two existing issues. One is a regression introduced by
39878b3753be ("Preserve omap records in snapshots"), which was causing
regular failures in generic/013 of xfstests. The problem was with the
hacky way in which I told apart container and volume omaps, by setting
sbi->s_vsb_raw to NULL before calling apfs_map_volume_super(); it seems
that the dentry cache or something needs this to be set at all times.
Now that the "latest_snap" field has been moved from the sbi to the omap
struct, we no longer need to handle containers and volumes differently:
container omaps are the same as volume omaps without snapshots.
The other issue is that snapshot mounts only read the location of the
omap root once (on mount), but subsequent writes to the volume will free
this original block and potentially allocate it for something else. I've
never seen this in practice yet, but I would expect the snapshot mount
to start complaining about filesystem corruption. Now that the omap root
node is shared by all mounts of the volume, this problem should be gone.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
In order to test the creation and preservation of snapshots, it's
important that we can actually check the contents ourselves. Add a new
'snap' mount option to specify the label of the wanted snapshot.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
With the previous patch in place we can now take snapshots, but they
become immediately corrupted when a new transaction begins and changes
the omap records. This happens even if file contents aren't modified,
so it becomes impossible to test this new functionality at all.
So, always preserve omap records that belong to a snapshot. For this
check, the disk layout provides om_most_recent_snap in the object map;
add a matching s_latest_snap field to the in-memory superblock structure
to make it easier to work with.
Bigger changes are needed in apfs_omap_lookup_block(), because we need
to work with an omap record's xid, which has so far mostly been ignored.
Replace apfs_bno_from_query() with a new apfs_omap_map_from_query()
which doesn't discard so much valuable metadata. This change also allows
us to start preserving the flags in the omap record, which may also be
important.
Another problem in apfs_omap_lookup_block() is that we need a way to
tell apart volume omaps from the container omap, which won't be affected
by snapshots. This is all a little hacky, and it would be much cleaner
if I had an in-memory omap struct to pass around instead of the root
nodes. I should look into that in the future.
With this patch in place, my fsck reports no corruption after a single
snapshot is created and the volume is unmounted.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Implement an ioctl that creates a snapshot for the current transaction.
A matching userland tool has already been added to apfsprogs.
Note that, at this point, snapshots won't be handled correctly on later
writes at all. Being able to actually create snapshots for testing is
only the first step in that direction.
Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Ernesto A. Fernández
Woody Suwalski