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

SAR and A/B interactions break internal install on Nintendo Switch #3014

Closed
classic-gentleman opened this issue Jul 21, 2020 · 9 comments · Fixed by #3015
Closed

SAR and A/B interactions break internal install on Nintendo Switch #3014

classic-gentleman opened this issue Jul 21, 2020 · 9 comments · Fixed by #3015
Labels
confirmed Issue confirmed to exist and the reason is known

Comments

@classic-gentleman
Copy link
Contributor

This is quite peculiar but I believe is a legitimate case nevertheless.

As you may be aware, some hardware revisions of the Nintendo Switch are amenable to modding and Android ports have been created for it. Usually the port is installed to the SD card and it's straightforward. My particular setup, though, has Switchroot Pie installed to the eMMC, side by side with HorizonOS (and Ubuntu Linux, for the matter).

The HOS partitioning scheme is regular GPT, but it does reserve some partition names, of note:

PRODINFO -> /dev/block/mmcblk0p1
PRODINFOF -> /dev/block/mmcblk0p2
BCPKG2-1-Normal-Main -> /dev/block/mmcblk0p3
BCPKG2-2-Normal-Sub -> /dev/block/mmcblk0p4
BCPKG2-3-SafeMode-Main -> /dev/block/mmcblk0p5
BCPKG2-4-SafeMode-Sub -> /dev/block/mmcblk0p6
BCPKG2-5-Repair-Main -> /dev/block/mmcblk0p7
BCPKG2-6-Repair-Sub -> /dev/block/mmcblk0p8
SAFE -> /dev/block/mmcblk0p9
SYSTEM -> /dev/block/mmcblk0p10
USER -> /dev/block/mmcblk0p11

Herein lies the problem. mmcblk0p10, that is, SYSTEM, is being confused for an A/B system partition because of the case insensitive compare here

if (strcasecmp(dev.partname, blk_info.partname) == 0) {
and the inexistent slot here
sprintf(blk_info.partname, "system%s", cmd->slot);

A solution would be to invert the tested scenario in SARBase::mount_system_root and look for the NVIDIA naming scheme first, then with system[+slot], and abort in case that fails.

Second possibility would be to attempt case sensitive first, then failing all that resort to case-insensitive. It would be more complicated though.

A third possibility is to completely split the code paths between A/B and non-AB SAR.

My vote goes to solution 1, for least impact.

@classic-gentleman
Copy link
Contributor Author

classic-gentleman commented Jul 21, 2020

Proposed patch:
switch_ab.patch.txt

@topjohnwu
Copy link
Owner

Hi, thanks for the detailed report!

Unfortunately, things aren't as simple as you assumed 😉, the comparison is explicitly case insensitive because on devices such as Samsung use full caps SYSTEM as the partition name.
There is literally no way Magisk can differentiate between these cases, so again, unfortunately, your current use case will not be supported.

There is a recommendation to fix this: don't use legacy SAR and switch to use 2SI instead. In that case, Magisk will not do the mounting, but rather defer to the original init to do the early mounting.

But 2SI depends on Android 10+, and I'm not sure about whether Android 10 is supported on switchroot.

@classic-gentleman
Copy link
Contributor Author

I understand your point; however I guess the patch I proposed (the first approach) can't break anything. Unless the Tegra partition name might conflict with something else?

@topjohnwu
Copy link
Owner

Oh, so switchroot is using Tegra naming scheme for Android?

@classic-gentleman
Copy link
Contributor Author

Yes, since the Switch is a Tegra T210 device!

@topjohnwu
Copy link
Owner

topjohnwu commented Jul 21, 2020

Sure, I'll reopen this issue to keep track of this.
Your proposed fix doesn't seem to break things at first glance.

@topjohnwu topjohnwu reopened this Jul 21, 2020
@osm0sis osm0sis added the confirmed Issue confirmed to exist and the reason is known label Jul 21, 2020
@classic-gentleman
Copy link
Contributor Author

I can attest this is working for me™ 😜

Now, if a more careful approach turns out necessary, I think it is possible to grab Switch-specific props or cmdline and adjust accordingly. But the simplest approach is the one in the patch and I'm not foreseeing conflicts with other devices.

@osm0sis
Copy link
Collaborator

osm0sis commented Jul 21, 2020

Submit the patch as a pull request?

@classic-gentleman
Copy link
Contributor Author

Thanks, gentlemen!

kubalav pushed a commit to kubalav/Magisk that referenced this issue Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed Issue confirmed to exist and the reason is known
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants