Remove obsolete and long broken addon.d support #6676
Conversation
|
I am extremely disappointed/saddened that included functionality is being intentionally considered for reduction. As I've said previously, the goal should be making users' lives easier, and making them go hunt for a module to have their custom ROM updates remain seamless is definitely not that. I have previously committed to continue to maintain the included addon.d support as I have for the years since its addition, and I stand by that. So whatever internal discussions have gone on without my input, @topjohnwu, please reconsider. |
https://github.com/programminghoch10/Lygisk/blob/ci-management/README.md?plain=1#L9-L11 |
yujincheng08
changed the title
yujincheng08
changed the title
|
Sounds like a feature request? It's not "long broken", that's a dramatic statement and change to make to the title. It simply doesn't work in recovery if the recovery doesn't support decryption. addon.d-v3 might be able to address this even, but @topjohnwu has always stated he wanted the Magisk binaries only in one location, back when I was included in these discussions. |
|
This is more of an indication that the addon.d should be detached from magisk so it can update faster and ignore requests from topjohnwu. |
Sorry, but are you sure?... Isn't it the case that addon.d "fails to reinstall Magisk during OTA if the device does not support FBE decryption in recovery"?... Seems to me therefore that it doesn't support old FDE... 🤔... What am I missing? |
|
FBE works fine via addon.d-v2, @pndwal is correct. I used it as recently as last week on my OnePlus 9 Pro. But see, that's the thing, I'm a real world user of Magisk, but with a bit more ability to triage and fix certain things, and that's what I used to represent and advocate for back when we would actually discuss things like this in Slack. |
|
@osm0sis I'm not merging this change anytime soon, feel free to express your thoughts and provide more information to allow us to make a sound decision. |
|
@osm0sis Is the Recovery you are using supporting decrypted data? |
|
addon.d-v2 is designed for the booted system, and it works fine as currently written (which is why no real changes for the past 2 years). addon.d(-v1) unencrypted works great in recovery, but FDE is a gap in coverage, as @topjohnwu had always been unhappy with the idea of keeping any Magisk binaries in multiple locations which he worried could get out of sync for various reasons. Some further hack could be added to the current script to address this if John has come around on that idea. At one point I was discussing this with the Lineage developers and I seem to recall them talking design for addon.d-v3 as able to natively include support binaries with the addon.d scripts, so that could be something to look into as well, given the same caveat. |
|
(Please just delete if it's not okay to chime in. Just trying to help, as a fellow developer). In my eyes the problem here is deleting the existing approach without providing a known-working alternative. The deprecation of the recovery installation route had the same problem. It was very annoying to run through the recommended installation and then see that Magisk vanished with the next LOS upgrade. It would completely make me drop Magisk if the only remaining alternative were dropped, as this PR seems to suggest - not only because it feels user hostile, but more because there simply would be no way for me to use Magisk anymore. But this shows a way forward: Have both. Extend the existing installation documentation with the steps to make use of the PoC module to keep Magisk alive after upgrades, so user like me can test it. Undeprecate the recovery sideloading installation, and let @osm0sis keep the addon.d support alive as much as possible given the encryption hurdles. You can still lead users to the technically better method, full deprecation and removal is not needed for that. I have a small testsuite of phones here (I work with Android in my day job) and could offer testing support, if it is wanted. I'm sure many others would provide similar support. Just let us know... |
No, it never worked properly, LineageOS needing permissive domains has not been fixed yet, they don't even want to modify their sepolicy and ask Magisk to do it.
You have admitted that non-A/B devices need unencrypted data partition or twrp that can decrypt data, and know that it cannot be improved to support encrypted data partition when it is bundled in Magisk, why still do not agree to let it break away from Magisk?
It looks like the addon.d developers are aware of encrypted data partition, which is good. But there's never been a discussion from them in this issue with Magisk, and I wonder why they haven't fixed the update engine needing a permissive domain. Is it because Magisk actually misuses addon.d, which is not designed to modify boot partition? When the design goal is to modify system partition, it makes sense to put the script in /system/addon.d, but Magisk is systemless, it has always conflicted with Magisk. |
|
Again, for the sake of accuracy in this discussion only:
Unless I'm missing something again, it seems addon.d(-v1) does support encrypted data partition when it is bundled in Magisk, just not in devices using FDE... Am I wrong or did you actually mean "it cannot be improved to support FDE encrypted data partition when it is bundled in Magisk"? 🤔 |
|
A number of good points made... Just a thought: If addon.d were moved to a module, could it still be kept as an official Magisk component?... I'm envisioning it being like the Systemless Hosts module, installable from Magisk settings page perhaps with a legend like: Magisk Survival for Custom ROM updates This way
And let's face it, despite Magisk being very stock-ROM-centric these days (understandably if not well understood...), custom ROMs are the traditional Android modder's staple and it would not be a mistake at all for a custom mod of Magisk's calibre to officially support custom ROM users in particular with a module like this... (Even Google offers special support for custom ROM users!) 😜 |
Similar to Magisk, as far as I remember, had been developed while keeping the community in mind and I hope it continues to do the same. I read somewhere above @osm0sis said, "letting users hunt for modules". From a naive average users' perspective who doesn't do much tinkering their phones but rather install some phone customizing modules, this project already been went too far from reach. Unless the user digs in to see what's happening and search for modules they want on the internet, he/she might have already gave up. I personally took 5+ days to search for |
|
I don't agree with |
I'll address the other replies when I have a bit more time, but addon.d is implemented by the ROM and is in /system because it supports all Android from before any of these other partitions existed. I'll definitely concede that it would work better housed elsewhere, but currently that's up to the ROM developers who wrote it. One pie-in-the-sky idea would be to make a Zygisk module that hooks update_engine to add a new "magisk-addon.d" addon.d replacement for everywhere including stock ROM. 🤯 |
but |
No, but update_engine is. 🙂 |
oh, just noticed now |
Yes, exactly this. (for certain values of "flawless" that include 500mb of accumulated magisk-backed-up recovery ramdisks in /data, but most people aren't flashing as often as I am as the ROM maintainer :D ) Nothing works from /system/addon.d when installing updates in Lineage-based recoveries on A/B devices like mine, but that's a known issue with the recovery environment and not a failure of Magisk. I don't know if it's a philosophical resistance by the Lineage team to enabling custom recoveries to be more powerful than AOSP intended, or a technical hurdle due to FBE; but that's not really important. Users know they need to manually re-flash their GApps zip and Magisk apk if they update "manually" in recovery on an A/B device like this. Please don't break the booted Updater functionality just because of a known issue with the recovery updater that can't be solved anyway without the Lineage team changing their implementation. |
Sorry, but you're mistaken here. LineageOS is actually the main ROM which ships update_engine domain permissive out-of-the-box themselves. Presumably they do this because addon.d is their baby and they know it requires a fair amount of access for GApps/add-on backup, restore and any other operations. This also worked to Magisk's advantage in allowing John and I to get it to do what it needed to do for Magisk addon.d-v2, so we agreed that it made sense to bring that permissive domain to all ROMs: cba0d04 Not sure if any of the other custom ROMs have improved things since then, but it would certainly be interesting to look into it further and discuss things with the major ROM project leads. While I don't believe this to be a major concern or attack vector since only custom ROMs allow addon.d and therefore access to that domain, and a user would need to consciously install any addon.d script just as they do root (an obviously worse potential vector), but one solution could be to only make update_engine permissive when /system/addon.d is present.
This was a known limitation since the very beginning when John and I wrote it, so I don't think my "admission" is the smoking gun you believe it to be for unshipping it. Our concensus at the time was that FDE devices were on the way out (and they are, it's deprecated), and most older FDE devices which people still use tend to be unencrypted, so FBE was the important one and addon.d-v2 handles that well. For those that do run FDE encrypted or are A-only and use a recovery that doesn't support decryption, we went to some lengths to output an error message for the user so that they were aware and could reflash Magisk manually: https://github.com/topjohnwu/Magisk/blob/master/scripts/addon.d.sh#L20-L40 Just to reiterate, if John has come around on it then I've certainly had some ideas over the years to either bundle the Magisk binaries with the existing addon.d-v2 script or update to (at least appear like) addon.d-v3 which I believe may allow for addon.d "support binaries" as I already mentioned...
I was speaking on behalf of Magisk at that time, since we all used to collaborate in the Slack together, when the Slack was still in use, if you recall. Again, Lineage shipped update_engine permissive first, Magisk just used that. To be clear here, Magisk is root. Root can do whatever it wants, including write to system or boot if the user desires that. Your strong stance against system ever being written to is purely your opinion. If the ROM has been built to allow rw by not using erofs or ext4-dedup, with extra space in system, product and system_ext for GApps and other complex mods, maintained through OTAs with addon.d which is (up to this point) intended permissive, then Magisk is there to help with that, and it's my opinion that Magisk should continue to use that existing infrastructure to make users' lives easier unless something better than addon.d comes along. |
Only on FDE devices. And again, a known limitation since it was created, and not a grave concern because FDE itself is deprecated. I would love to see this support for older devices improved, but that's no reason to remove Magisk's addon.d script, since all newer devices are FBE and A/B, and addon.d-v2 support does work for them, as I explain in my previous post.
Updating the addon.d scripts by Direct Install absolutely could be supported by the Magisk app, Magisk is root, you just don't agree that it should, and you're entitled to that opinion, but that doesn't invalidate the request for that support, nor the entire Magisk addon.d feature as it currently exists. For clarity, there are only actually 2 recoveries right now, TWRP-based and Lineage-based, though I agree the different older Android trees which older TWRP devices are built on have always presented some problems. While I agree there are some compromises to installing Magisk in recovery, especially recently for the serules/preinit refactor, it is also the only way to root your device before it ever boots, so should continue to exist in some form or another, even if that form is diminished to basically the same as unrooted boot patching. I fully support that.
Nobody is suggesting Magisk force all system partitions to be writeable. The only topic at hand is custom ROMs which have already clearly, explicitly chosen to be rw by not using erofs or ext4-dedup, and even go so far as to include extra free space in system, product and system_ext for GApps and modifications. They indicate they support addon.d by including /system/addon.d and therefore remounting system rw is acceptable on that ROM. Magisk can be systemless overall but still support the custom ROMs which allow rw and addon.d. You have a difference of opinion on this, clearly, but it's just that, an opinion, and we are here to discuss, so please stop acting as though all rw is against Magisk; Magisk is root and root can do whatever it wants if we want it to.
addon.d is only for custom ROMs. CyanogenMod created it, and Lineage currently maintains it, releasing addon.d-v3 with Lineage 19.0 to support product, vendor and system_ext in addition to the original addon.d (and addon.d-v2) system support. All GApps packages use addon.d scripts to remain installed through OTAs. I agree keeping them elsewhere could be useful, but on a ROM where system is already allowed rw, there's no technical issue with the scripts still being in system. For example if there was some Zygisk-backed replacement for addon.d which could hook update_engine to add something similar to addon.d support to any ROM including OEM, well then it would definitely need to use /metadata or something to support both FDE and FBE, but as you say, OEM ROMs don't allow system rw anymore, so that idea's use case would be pretty limited on OEM ROMs, e.g. to keeping Magisk and/or TWRP installed through an OTA. But that's not what we're discussing right now.
I completely agree /data-free support in Magisk's addon.d would be helpful, and as I've mentioned a few times it could also be added to Magisk's existing script fairly easily, which I'd love to look into and wouldn't require much iteration since FDE is deprecated. It could work as a separate module, sure, but I'm not really seeing any actual reason to remove it from Magisk other than "rw is bad" presented here. Again, all of these limitations were known when John and I wrote it. Let's continue to support the custom ROM community by making it easy for them to stay rooted.
Weird way to approach this... It seems like your argument here is "it was never really officially supported in the first place"? John and I wrote it. It is supported and works in every respect it was intended to work (i.e. excluding the known A-only FDE in-recovery limitation). Then you seem to to be suggesting that my not contributing the few lines in fix_env and/or the Direct Install .kt to mount Magisk's own system mirror rw if the addon.d directory exists to push the script is somehow an indicator the entire feature support is dead? First of all, that feels like some kind of attack on me, just like some of your previous abrupt comments in that issue thread about removing addon.d for "feature parity" do, but I won't rehash that further, I want to continue our debate here in good faith. I have my 13 month old daughter sleeping in the next room to me, as evidence of the time constraints of having a home and family in addition to a full time job, but the real reason for no movement was that I couldn't bring myself to spend what little free time for Android I have on something that you and vvb had already so negatively indicated you weren't in favor of. The issue remained open to see if John might review it and change his mind to support improving the feature parity between booted and recovery by adding more support not less, at which point I'd happily do what I could, but, well, here we now are.
MagiskHide support was an ongoing battle with Google and banking apps, whereas addon.d support has remained very straightforward and unchanging. But I agree about the concerns for opt-in, so we could compromise as I previously suggested: never install the addon.d script by default through any method, but have a button in the Magisk app settings (a la Systemless Hosts) which is only visible on custom ROMs with /system/addon.d and will only install the addon.d script when tapped; an opt in only feature. See my previous post where I discussed the update_engine permissive domain concern; I believe that could be worked around with some built-in logic as well if you still feel it's serious enough to require addressing, though John certainly agreed to it when we discussed it at the time. At the end of the day this whole thing remains a difference of opinion between us about what Magisk as a root solution "should" and "shouldn't" do. You think it should never touch system under any circumstances, and I think it can do whatever it wants if the situation calls for it. We're likely not going to convince eachother, but we'll see what John thinks in reviewing our debate, and that will be that. |
|
We need |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
I don't care how many bells and whistles there are in addon.d.sh, it doesn't bother me. I care that update_engine cannot be permissive. Only a few people use custom ROM, even fewer use addon.d, and they're pretty much all LineageOS, but you're bringing the permissive update_engine domain of LineageOS to all devices. |
|
@osm0sis I'm totally fine with keeping addon.d scripts as it is. However, having If custom ROMs require |
Yep, that part is completely fine with me. I'd be happy to debug that change with the current various ROM implementations of update_engine sepolicy/addon.d-v2+3 to make sure that's all it needs to run su. I'd still like to suggest the addon.d script becomes opt-in only as a button in the app settings, similar to Systemless Hosts opt-in, but only shown if /system/addon.d exists. That way it's equally accessible regardless of install method, and would close my open issue regarding feature parity. Edit: @topjohnwu Any change in your stance regarding keeping Magisk binaries in /system/addon.d so we could try to add FDE recovery support? I'd be happy to work on that too. |
https://github.com/yujincheng08/Magisk-Addon
https://github.com/programminghoch10/Lygisk/blob/ci-management/README.md?plain=1#L9-L11