linux-apfs / linux-apfs-rw

APFS module for linux, with experimental write support
GNU General Public License v2.0
547 stars 36 forks source link

Is RO-by-default still necessary? #81

Open kurtbahartr opened 1 month ago

kurtbahartr commented 1 month ago

So I've had one of the leaders at Pisi Linux request a kernel module to mount and recover data from his APFS drive inside the distro. I quickly packaged this kernel module and sent it his way, also noting that RW support is still experimental and noted to corrupt the filesystem in question. He then returned to me with the confirmation that the RW operations he did so far using this module worked just fine and that he could still keep using his macOS installation in it. The testing method we went through is as follows;

Now, here's the question: How experimental is the RW support? How necessary is it to keep it RO-by-default and what would you suggest me to do for offering an easy way to enable RW as a packager?

eafer commented 1 month ago

For filesystems with very simple features (case-sensitive, no snapshots, etc), the driver has been tested to death, and I don't expect normal users to suffer corruption. The problem is that you have a lot of possible combinations of feature and mount flags, plus a lot of only partially documented or reverse-engineered record flags (and xfields I think) that keep piling up. I like to think that I've been careful, but I have no way to cover everything in my tests, and some of it may be broken in some unexpected way. The farther your filesystem is from the ones I usually test on, the bigger the risk.

99.99% of users would be fine I think, but the remaining 0.01% would want to murder me for eating their data, hence the mount restrictions (I can't expect everyone to check the readme ahead of time). I should probably tone down the warnings though, it's a bit of an exaggeration at this point. As a packager, if you are willing to explain this to your users and take responsibility, then you can just patch the driver to always set the READWRITE flag unconditionally, putting a line like nx_flags |= APFS_READWRITE; early on parse_options(), for example. I guess I could add some build option for this if you prefer.

kurtbahartr commented 1 month ago

Thanks for the lengthy explanation!

Yep, I'm willing to take full responsibility and tell the users about this when needed. It was me who initially asked for this anyway. :D

As for the flag, it would be much appreciated actually. I hate having to patch the source downstream especially in things as critical as this. I also go to extreme lengths just to avoid having to do so when possible (This very portion of the code was in PiSi itself and wasn't documented if memory serves).

Again, thanks for the explanation! I'll try your suggestion out tomorrow and come back with the results.

kurtbahartr commented 1 month ago

@eafer, it has been almost 2 weeks since I said I'd try but I hadn't forgotten about this.

I tried your method locally yesterday, had someone to test it and he reported that he could copy new files but never write over existing ones. Further investigation showed that the permissions were preserved as is so I guess it's as usable and sturdy as BTRFS as far as the average user is concerned.

However, a build flag would still be useful - I could also try to chip in with the little C knowledge I have if you would fancy that way instead.

eafer commented 1 month ago

I plan to add the build flag before the next release, but I'm worried that you couldn't write over files in general? Was there any output on dmesg? Maybe they were compressed files, in which case you have to make a copy before.

kurtbahartr commented 1 month ago

I couldn't think about asking for a dmesg back there but I remember crystal clear that the owners and permissions of the said files and directories were the exact same as that of macOS's (macOS UIDs and GIDs, with directory permissions set to 600 in the said home directory). The only folder that a regular user could write to was the "Public" directory under the macOS user's home directory, which makes sense considering the name and permissions (666 if memory serves).

The failure to write files part is also rather ambiguous now that I think about it - I have no idea how he tried to write there after all, in which case only a shell with superuser privileges should be able to write into there, which is something I have huge doubts on.

eafer commented 1 month ago

Ah ok, I get it now, so it was an ownership issue. There is a mount option to override the ownership, but I haven't tested it in a while.

kurtbahartr commented 1 month ago

Ah ok, I get it now, so it was an ownership issue. There is a mount option to override the ownership, but I haven't tested it in a while.

I'll have this tested as soon as I can and get back to you.

eafer commented 2 weeks ago

I just pushed a patch with the build flag if you want to try it out.

kurtbahartr commented 2 weeks ago

I just pushed a patch with the build flag if you want to try it out.

Perfecto, I'll test it out within today and come back with the results!

kurtbahartr commented 2 weeks ago

Quick follow-up, I haven't gotten a response from the tester over the weekend. I'll come back here once I get a response from him.

I can't test this myself yet due to upcoming midterms, but I'll do the tests myself next week if everything goes according to the plan.

kurtbahartr commented 2 weeks ago

Alright, now a definite answer - I got positive response.

The tester confirmed he could mount his Mac's internal drive using Pisi and the package I used your commit in. However, he did report 2 bugs (one seemingly being an actual bug and the other being by design I assume) unrelated to RO vs R/W matter;

eafer commented 2 weeks ago

Are you sure the usb is formatted with apfs? This filesystem doesn't use journaling. Maybe it's hfs or something? What's the message exactly?

EDIT: also, the driver doesn't currently parse labels. I don't know what issue you are seeing, but it's probably userland stuff. It's tricky with filesystems that have subvolumes, because you have more than one label for each "device".

kurtbahartr commented 2 weeks ago

Are you sure the usb is formatted with apfs? This filesystem doesn't use journaling. Maybe it's hfs or something? What's the message exactly?

You're right, I just checked the logs my tester provided me with again (output of sudo dmesg | grep mount) and here's the message came out of it;

[   15.724903] new mount options do not match the existing superblock, will be ignored
[   62.571262] hfsplus: write access to a journaled filesystem is not supported, use the force option at your own risk, mounting read-only.
[   64.773896] hfsplus: write access to a journaled filesystem is not supported, use the force option at your own risk, mounting read-only.
[   66.963696] hfsplus: write access to a journaled filesystem is not supported, use the force option at your own risk, mounting read-only.

Yes, I know, this only tells me it's being remounted RO and I phrased the error incorrectly back there. It was middle of the night for me, so I was half-asleep when I sent that message. ^^;

EDIT: also, the driver doesn't currently parse labels. I don't know what issue you are seeing, but it's probably userland stuff. It's tricky with filesystems that have subvolumes, because you have more than one label for each "device".

I see. What I see exactly is that, in Dolphin, the subvolume's name appears as "446.9 GB internal storage (sda2)." The tester told me the drive's label was supposed to be "MR".