0.3.11

This release prevents deadlock if a single volume gets mounted and unmounted at the same time. It also fixes two other (smaller) mount bugs found while testing this one. Support is added for the upcoming 6.11 kernel release, with a patch by Woody Suwalski and a later patch of my own because things broke a second time. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>
Avoid double parsing of mount options
2026-08-18 02:19:59 -07:00 · 2024-09-12 20:20:49 -03:00 · 2024-09-11 16:54:47 -03:00 · 2024-08-30 20:25:44 -03:00 · 2024-08-15 16:01:07 -03:00 · 2024-08-15 16:01:07 -03:00
6 changed files with 337 additions and 165 deletions
@@ -217,6 +217,11 @@ struct apfs_ephemeral_object_info {
 /* Mount option flags for a container */
 #define APFS_CHECK_NODES	1
 #define APFS_READWRITE		2
+/*
+ * Mount options for a container are decided on its first mount. This flag lets
+ * future mounts know that has happened already.
+ */
+#define APFS_FLAGS_SET		4

 /*
 * Container superblock data in memory
@@ -1,5 +1,5 @@
 PACKAGE_NAME="linux-apfs-rw"
-PACKAGE_VERSION="0.3.10"
+PACKAGE_VERSION="0.3.11"

 BUILT_MODULE_NAME[0]="apfs"
 DEST_MODULE_LOCATION[0]="/extra"
@@ -8,7 +8,9 @@
 #include <linux/blk_types.h>
 #include "apfs.h"

+#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 11, 0)
 #define MAX(X, Y)	((X) <= (Y) ? (Y) : (X))
+#endif

 /**
 * apfs_ext_is_hole - Does this extent represent a hole in a sparse file?
@@ -285,10 +285,8 @@ static void apfs_trans_commit_work(struct work_struct *work)
 	 * (TODO).
 	 */
 	down_write(&nxi->nx_big_sem);
-	mutex_lock(&nxs_mutex);
 	if (!sb || sb->s_flags & SB_RDONLY) {
 		/* The commit already took place, or there was an abort */
-		mutex_unlock(&nxs_mutex);
 		up_write(&nxi->nx_big_sem);
 		return;
 	}
@@ -330,11 +328,9 @@ int apfs_transaction_start(struct super_block *sb, struct apfs_max_ops maxops)
 	int err;

 	down_write(&nxi->nx_big_sem);
-	mutex_lock(&nxs_mutex); /* Don't mount during a transaction */

 	if (sb->s_flags & SB_RDONLY) {
 		/* A previous transaction has failed; this should be rare */
-		mutex_unlock(&nxs_mutex);
 		up_write(&nxi->nx_big_sem);
 		return -EROFS;
 	}
@@ -346,7 +342,6 @@ int apfs_transaction_start(struct super_block *sb, struct apfs_max_ops maxops)
 	if (!nxi->nx_eph_list) {
 		err = apfs_read_ephemeral_objects(sb);
 		if (err) {
-			mutex_unlock(&nxs_mutex);
 			up_write(&nxi->nx_big_sem);
 			apfs_err(sb, "failed to read the ephemeral objects");
 			return err;
@@ -440,14 +435,12 @@ int apfs_transaction_flush_all_inodes(struct super_block *sb)
 		list_del_init(&ai->i_list);

 		nx_trans->t_state |= APFS_NX_TRANS_COMMITTING;
-		mutex_unlock(&nxs_mutex);
 		up_write(&nxi->nx_big_sem);

 		/* Unlocked, so it may call evict() and wait for writeback */
 		iput(inode);

 		down_write(&nxi->nx_big_sem);
-		mutex_lock(&nxs_mutex);
 		nx_trans->t_state = 0;

 		/* Transaction aborted during writeback, error code is lost */
@@ -682,7 +675,11 @@ static int apfs_transaction_commit_nx(struct super_block *sb)
 	}
 	list_for_each_entry_safe(bhi, tmp, &nx_trans->t_buffers, list) {
 		struct buffer_head *bh = bhi->bh;
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 11, 0)
+		struct folio *folio = NULL;
+#else
 		struct page *page = NULL;
+#endif
 		bool is_metadata;

 		ASSERT(buffer_trans(bh));
@@ -702,27 +699,44 @@ static int apfs_transaction_commit_nx(struct super_block *sb)
 		kfree(bhi);
 		bhi = NULL;

+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 11, 0)
+		folio = page_folio(bh->b_page);
+		folio_get(folio);
+#else
 		page = bh->b_page;
 		get_page(page);
+#endif

 		is_metadata = buffer_csum(bh);
 		clear_buffer_csum(bh);
 		put_bh(bh);
 		bh = NULL;

+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 11, 0)
+		folio_lock(folio);
+		folio_mkclean(folio);
+#else
 		/* Future writes to mmapped areas should fault for CoW */
 		lock_page(page);
 		page_mkclean(page);
+#endif
 		/* XXX: otherwise, the page cache fills up and crashes the machine */
 		if (!is_metadata) {
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 19, 0)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 11, 0)
+			try_to_free_buffers(folio);
+#elif LINUX_VERSION_CODE >= KERNEL_VERSION(5, 19, 0)
 			try_to_free_buffers(page_folio(page));
 #else
 			try_to_free_buffers(page);
 #endif
 		}
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 11, 0)
+		folio_unlock(folio);
+		folio_put(folio);
+#else
 		unlock_page(page);
 		put_page(page);
+#endif
 	}
 	err = apfs_checkpoint_end(sb);
 	if (err) {
@@ -834,7 +848,6 @@ int apfs_transaction_commit(struct super_block *sb)
 		mod_delayed_work(system_wq, &trans->t_work, msecs_to_jiffies(100));
 	}

-	mutex_unlock(&nxs_mutex);
 	up_write(&nxi->nx_big_sem);
 	return 0;
 }
@@ -928,7 +941,6 @@ void apfs_transaction_abort(struct super_block *sb)
 		/* Transaction already aborted, do nothing */
 		ASSERT(list_empty(&nx_trans->t_inodes));
 		ASSERT(list_empty(&nx_trans->t_buffers));
-		mutex_unlock(&nxs_mutex);
 		up_write(&nxi->nx_big_sem);
 		return;
 	}
@@ -966,7 +978,6 @@ void apfs_transaction_abort(struct super_block *sb)
 	 */
 	apfs_force_readonly(nxi);

-	mutex_unlock(&nxs_mutex);
 	up_write(&nxi->nx_big_sem);

 	list_for_each_entry_safe(ai, ai_tmp, &nx_trans->t_inodes, i_list) {
@@ -12,9 +12,12 @@
 #include <linux/types.h>
 #include <linux/nls.h>
 #include <linux/ctype.h>
+#include <linux/version.h>
 #include "unicode.h"

+#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 11, 0)
 #define MIN(X, Y)	((X) <= (Y) ? (X) : (Y))
+#endif

 /* The arrays of unicode data are defined at the bottom of the file */
 /* TODO: would a single trie with all the data be more efficient? */
Author	SHA1	Message	Date
Ernesto A. Fernández	0667dbff17	0.3.11 This release prevents deadlock if a single volume gets mounted and unmounted at the same time. It also fixes two other (smaller) mount bugs found while testing this one. Support is added for the upcoming 6.11 kernel release, with a patch by Woody Suwalski and a later patch of my own because things broke a second time. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-09-12 20:20:49 -03:00
Ernesto A. Fernández	af2ee526c0	Avoid double parsing of mount options Originally all mount options were parsed inside apfs_fill_super(), because I was following the example of other filesystems (such as ext2) that have no subvolumes or snapshots. When I implemented those features, I realized that some mount options would be needed early on mount, so that sget() could use them to decide if the superblock is already mounted. My lazy solution at the time was to leave parse_options() as it was, but also add some redundant code early on mount to read the needed options. Just looking at the amount of TODOs in those functions makes it clear that this was always very silly. And now that the locking during mount has been simplified, this is causing a race, and I want to take that stuff seriously. The bug is that while parse_options() rereads the mount options needed by sget(), there is nothing preventing another concurrent mount from testing the superblock. Say the first mount is for volume 1: the volume number gets briefly reset to the default value of zero, and if the other mount actually wants volume 0, sget() will consider this a match. Anyway, split parse_options() into two equally clean functions, one for early mount and another to get called within apfs_fill_super(). I even considered simply parsing all the options early during mount, but that would require a few more changes because the nxi is not attached at that point. Maybe some other time. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-09-11 16:54:47 -03:00
Ernesto A. Fernández	44f4b4f631	Don't redefine MAX() and MIN() for the 6.11 kernel The build has become broken once again for the 6.11 kernel: https://github.com/linux-apfs/linux-apfs-rw/issues/75 The problem is that the second release candidate introduced generic MAX() and MIN() macros for everyone to use, and that crashes with our own definitions. So, only define them for older kernels. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-08-30 20:25:44 -03:00
Ernesto A. Fernández	b44d2a37bf	Don't forget to initialize the omap cache spinlock The omap cache has been in place for years, and it seems that its spinlock was never actually initialized before use. I have no idea what the consequences of this are, but for one it does cause lockdep to turn itself off after every mount. Fix it. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-08-15 16:01:07 -03:00
Ernesto A. Fernández	4881d7c707	Don't return 0 on mount if sget() fails For the previous patch I had to carefully test all the possible errors during mount and unmount. I noticed that injecting an error inside apfs_set_super() triggers npd. The cause is that the error code returned by sget() gets swallowed and the kernel is allowed to think that the mount succeeded. This is not a new issue, it's been around since apfs_mount() was first implemented. I never noticed because apfs_set_super() would only fail if we ran out of anonymous devices. So fix it in a new commit. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-08-15 16:01:07 -03:00
Ernesto A. Fernández	2982c8655f	Don't use the mount mutex so much Repeatedly running generic/604 of xfstests eventually hangs the kernel. This is a test that forces a mount to race with an unmount. The problem is a standard deadlock: ->put_super() gets called while the s_umount lock is held, and then starts a transaction, which takes nxs_mutex; meanwhile ->mount() first takes nxs_mutex and then calls sget(), which takes s_umount. Before this patch, I had never really put much thought as to where exactly nxs_mutex is needed. It's obviously the lock for the list of mounted containers, but it's also used as a generic way to avoid modifications to a container while another of its subvolumes is getting mounted. It's always been obvious that I'm holding it for too long and in too many places, so I want to finally put some effort into figuring this out now, and leave a record here so that I don't forget. The first obvious point is that the mutex can be dropped before calling sget(), and retaken later. There should be no problem if another mount called it at the same time, and it's enough to fix the bug. But putting some thought here exposes an existing issue with this code: sget() may actually test some superblock that is already half-unmounted and had its info freed, so apfs_test_super() is risking npd or use-after-free. Looking at other filesystems I realize that my mistake was freeing the info inside ->put_super(), because at that point the super is still listed in the fs_supers list for apfs. Instead ->kill_sb() should free the info itself, after generic_shutdown_super() is done (which calls ->put_super()), and after kill_super_notify() has taken care of the list. Since kill_super_notify() can't be called from inside a driver, a little hack is needed to call it through kill_anon_super(). The cleanups for apfs_mount() also need to be reconsidered, since ->put_super() is only called by deactivate_locked_super() after the root dentry is set, but the rest of ->kill_sb() runs either way. Going back to the mutex: even if a race for sget() itself is not an issue, dropping it there does have other consequences. The reference count for the container superblock is increased before sget(), and it's later checked on some initializations to see if this is the first mount. That won't do anymore, because a second mount could increase the refcnt before the mutex is retaken, and the first mount would never complete its work. Use other fields for this check: in apfs_read_main_super() use the blocksize (which is zero before the first mount sets it); for apfs_set_nx_flags(), define a new flag to tell if the other flags have been set. This already tells us two functions that definitely need to be protected by the mutex, so that they can complete their initializations before any other mount comes to check them. The situation is similar for apfs_first_read_omap(), which goes through the container's volume list to check for previous reads. After going line by line through the mount code, I'm fairly confident that the mutex only needs to be held for apfs_attach_nxi() and these three functions. For unmount it's the same: apfs_free_main_super() and apfs_put_omap() require the mutex. The second will now unset the omap pointer for the volume atomically, so that it can be checked by apfs_first_read_omap(); it is renamed accordingly. So far this is all about the races between different mounts and umounts; but we also don't want the mount to read inconsistent data from disk because some existing mount happened to write to it. Back when the driver could only mount one volume at a time, apfs_fill_super() didn't need any locking, so when multiple mounts got implemented I decided to just ban transactions during mounts entirely, by taking the nxs_mutex. This double lock looks very awkward and is a factor on the deadlock we are fixing, so let's clean that up now: use the read semaphore itself to protect apfs_fill_super(). Of course this means that two mounts for the same container can now run through this function at the same time, so be careful with changes to the shared nxi: we've already dealt with this issue for apfs_set_nx_flags(), but apfs_set_trans_buffer_limit() might also be unsafe. Just in case, move it to apfs_read_main_super(), it makes more sense that way anyway. By the way, apfs_read_main_super() also reads from disk, but it only does it on the first mount, so there is no risk of racing against a transaction. There is one function that writes without a transaction, namely apfs_make_super_copy(), but that function takes the nxs_mutex to ensure that no mount will begin during the superblock update. I think that covers everything? I'm really not sure if this writeup was clear at all, I have a hard time reasoning about locks. I also put a simpler list of the uses of nxs_mutex in a comment next to its declaration, which should be more practical as a reference. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-08-15 16:01:06 -03:00
Ernesto A. Fernández	707d91b486	Undo code reordering in previous patch I just picked up a patch to add support for the 6.11 kernel. I only did a shallow review, and now I notice that the code was reordered and a null buffer head is getting dereferenced. Revert that. Signed-off-by: Ernesto A. Fernández <ernesto@corellium.com>	2024-07-30 17:55:14 -03:00
Woody Suwalski	3e5c902a9a	Kernel 6.11: use folio for page ops (#74 )	2024-07-30 17:23:17 -03:00