Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is RO-by-default still necessary? #81

Open
kurtbahartr opened this issue Oct 7, 2024 · 13 comments
Open

Is RO-by-default still necessary? #81

kurtbahartr opened this issue Oct 7, 2024 · 13 comments

Comments

@kurtbahartr
Copy link

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;

  • Enable the module and mount the APFS drive as usual (can also use the desktop file manager like Nautilus and Dolphin)
  • Determine the mountpoint of the drive
  • sudo mount -o rw,remount /path/to/mntpnt

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
Copy link
Member

eafer commented Oct 7, 2024

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
Copy link
Author

kurtbahartr commented Oct 7, 2024

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
Copy link
Author

kurtbahartr commented Oct 23, 2024

@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
Copy link
Member

eafer commented Oct 23, 2024

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
Copy link
Author

kurtbahartr commented Oct 23, 2024

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
Copy link
Member

eafer commented Oct 23, 2024

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
Copy link
Author

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 added a commit that referenced this issue Nov 8, 2024
A distro packager has requested writable mounts by default, and is
willing to take responsibility for any potential harm to their users:

  #81

Add a build flag to enable this, but leave it undocumented to avoid
confusing anyone.

Signed-off-by: Ernesto A. Fernández <[email protected]>
@eafer
Copy link
Member

eafer commented Nov 8, 2024

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

@kurtbahartr
Copy link
Author

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
Copy link
Author

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
Copy link
Author

kurtbahartr commented Nov 11, 2024

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;

  • The filesystem label of his Mac's internal drive doesn't seem to have been parsed by the module even though a USB stick he has with the same filesystem did.

  • The USB stick in question returned mount failure as a whole for the time being (I doubt this is unrelated to always R/W but I may check with that disabled if you want). Digging through DMESG tells me that mounting journaled filesystems is not yet supported.

@eafer
Copy link
Member

eafer commented Nov 12, 2024

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
Copy link
Author

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants