treewide: Basic Ubuntu Touch & Halium enablement - #13661
Conversation
|
Everyone contributing to this PR have now signed the CLA. Thanks! |
459cc75 to
118a947
Compare
Meulengracht
added
the
Needs security review
There was a problem hiding this comment.
I think I'd split the part specific to Touch into a separate PR. There may be questions about how we want to approach this specific variant that are unrelated to supporting libhybris
There was a problem hiding this comment.
Tbf I've waited pretty long on the previous PR that has been sitting for a while, which used to be the libhybris-only PR. But if more people want to have it split up, then sure.
There was a problem hiding this comment.
I would split it. The s-c bits should be fairly easy to land, but folks may be asking questions about this one and there's no point in holding back the rest of the PR.
bboozzoo
left a comment
bboozzoo
left a comment
There was a problem hiding this comment.
Thank you. Please have a look at the comments.
There was a problem hiding this comment.
I think you can reuse the buffer since snprintf always adds terminating \0 and simply interleave snprintf/fs operation sequence
Also, please use PATH_MAX where appropriate:
| char rfsbuf[512] = { 0 }; | |
| char rfsbuf[PATH_MAX] = { 0 }; |
There was a problem hiding this comment.
looks like a copy of sc_populate_libgl_with_hostfs_symlinks, could we make the former non-static and share the code?
There was a problem hiding this comment.
again, can we share sc_mkdir_and_mount_and_glob_files from nvidia support code?
There was a problem hiding this comment.
This feels misplaced, neither snap-confine nor any part of the snap execution chain delivered by snapd sets any of the environment variables related to mesa/vulkan/egl. Most likely this should be part of to https://github.com/snapcore/snapcraft-desktop-integration
There was a problem hiding this comment.
IMO having snap-confine set up the environment by itself allows for:
- Expected environment for any Snap
- Less ways of the Snap screwing up something in the setup (because it misses -desktop-integration or does things itself)
- Have existing Snaps working as-is.
I cannot expect upstream app developers to care or have the knowledge to setup libhybris appropriately, which is why I put it there.
EDIT: Also I would argue that the libhybris enablement is for more than just GL drivers, it actually allows any appropriately wrapped bionic-library to function inside of a Snap.
There was a problem hiding this comment.
Hm I think I'd rather place it somewhere around here: https://github.com/snapcore/snapd/blob/68fe7eb9f7496ec54f2b3373a5c8d1993b9eb822/cmd/snap-exec/main.go#L211-L213 there's a precedent for it already for Cups
cc @zyga ?
|
Since security is supposed to be involved in the discussion, here is a little hint about something that might come up: For Halium/Android 9-only devices we had to ship a TLS-padding hack in the form of a shared library that needs to be LD_PRELOADed into the process, otherwise bionic libraries and glibc libraries would have a mismatch in TLS sizes. The do-nothing library resides here: https://gitlab.com/ubports/development/core/hybris-support/tls-padding/-/blob/main/tls-padding.cpp?ref_type=heads Ubuntu Touch sets the LD_PRELOAD environment variable to include this hack into every user-app process, so ideally we can keep it to keep having 9 devices chugging along. It explains the necessity to include the tls-padding .so into the AppArmor profile so that it can be mapped into the process image. |
d41963d to
ba1b38d
Compare
bboozzoo
changed the title
|
Thanks for the additional comments, will take care of them. I just noticed I wasn't specific enough on the reasoning regarding commit d779cae where I stated "on a separate filesystem". This means udev isn't able to manage it using devtmpfs and udevadm being unable to query properties of those binderfs endpoints. EDIT: CLA signed with the email address in my commits. |
d779cae to
4004cfb
Compare
|
I will happily review this after 2.62 is released. Apologies for the delay, we're really busy trying to get the release in good shape. |
| /{,var/}run/shm/hybris_shm_data rw, # FIXME: LP: #1226569 (make app-specific) | ||
| # This is a LD_PRELOADed TLS padding library allowing use of both | ||
| # Android 9 and glibc TLS slots at the same time. Only used on Halium 9. | ||
| /usr/lib/@{multiarch}/libtls-padding.so mr, |
There was a problem hiding this comment.
How is the library resolved at runtime, the directory will be from the perspective of a pivot_root base snap which (I suspect?) does not carry it.
There was a problem hiding this comment.
This particular library resides on the host, which then gets picked up by snap-confine AFAICT.
In general thread-locals in a shared library are taken care of after initialization by ld, before the actual executable is run. Think calling constructors of a static variable, it's similar in style to how ld treats thread-locals.
There was a problem hiding this comment.
Just for the record, host files do not appear at that location. What you see here comes from the base snap.
There was a problem hiding this comment.
I am surprised then that snap-confine correctly stops complaining about not being able to map that library.
There was a problem hiding this comment.
If you're setting LD_PRELOAD in your environment, than the denial from s-c should be raised regardless of this interface, as s-c is running under a different profile. Now, setting LD_PRELOAD in the environment will not have an effect of the snap as IIRC LD_PRELOAD isn't carried over to the snap's environment. Now, if OTOH is being set within the snap, then yes, if you allow it explicitly in the file like here, the denial will go away even if the file doesn't exist. Can you paste the denial you saw?
There was a problem hiding this comment.
After giving it some time, I've come to the conclusion that we might still want to pass through the preload hack to the confined environment in case it's running on Touch specifically. Would that be feasible from your perspective?
There was a problem hiding this comment.
ld.so will strip out LD_PRELOAD before it switches execution to snap-confine, i.e. snap-confine will not even see that environment variable, so there's no chance of passing it to the snap. Perhaps you can load it from /etc/environment when executing in the snap, but it would not be automatic.
There was a problem hiding this comment.
I was more thinking of measuring whether it's running on Touch via other means (checking for existance of a Halium-only file) and recreating the environment variable. It might sound hacky at first but there really seems to be no easy other way.
EDIT: I suppose that needs to happen in the desktop-helpers then? In that case I would reduce this MR by just allowing mapping the file from the hostfs and do the rest in the launchers. Or how about a SNAP_LD_PRELOAD which allows snapd to dictate the environment variable?
There was a problem hiding this comment.
I presume you've ran some apps with this change built locally. Were they able to start and function properly?
There was a problem hiding this comment.
No I couldn't as my assumption about Halium 9 vs 10 was wrong. I thought one of my test devices was susceptible to the issue, whereas it was just Halium 10 MediaTek devices specifically (which seem to have TLS slots hardcoded in their bionic-based GL libs).
| /{,android/}apex/com.android.i18n/lib{,64}/**.so m, | ||
| /{,dev/}socket/property_service rw, # attach_disconnected path | ||
| /{,dev/}socket/logdw rw, # attach_disconnected path | ||
| /{,dev/}__properties__/** rw, # attach_disconnected path |
There was a problem hiding this comment.
Curious, where is properties coming from? Is this some Android-specific interface? What's inside?
There was a problem hiding this comment.
The __properties__ part contains SELinux info of process labels allowed to access Android properties through getprop and setprop. It's more of a void on UT though since functionality tied to SELinux is stubbed out to not conflict with AppArmor.
There was a problem hiding this comment.
Does it need rw access or just read?
There was a problem hiding this comment.
Potentially only needs read-access, since that is set up in the Halium container. But I'll have to check.
Ping! I've noticed 2.62 landing, and I don't want to be left behind. :) |
a95e4ba to
d866b01
Compare
|
For the record and completeness sake, here is the list of devices tested with these changes:
|
|
Sooo... anything new? |
|
@alexmurray can you have a look? |
alexmurray
left a comment
alexmurray
left a comment
There was a problem hiding this comment.
LGTM other than the LD_PRELOAD hack which from the existing comments seems unnecessary / unworkable via snap-confine and should be removed.
There was a problem hiding this comment.
nitpick: this does't appear to be a tmpfs so this error message is misleading
There was a problem hiding this comment.
It seems like this should be removed given the prior discussions that it appears unneeded.
There was a problem hiding this comment.
it is not needed for most devices, but rather for a specific series of devices from a specific Android generation: Android 10 Mediatek, which I don't own, so I cannot figure out a solution by myself.
I am fine with the other changes landing but realistically we will need a solution for Mediatek devices with Android 10 too.
There was a problem hiding this comment.
nitpick: would this be easier to read if it was written as something like the following (note untested):
const char *paths[] = {"/dev/binderfs/binder", "/dev/binderfs/hwbinder", "/dev/binder", "/dev/hwbinder"};
for (int i = 0; i < sizeof(paths)/sizeof(paths[0]); i++) {
struct stat sbuf;
if (stat(paths[i], &sbuf) == 0) {
sc_device_cgroup_allow(cgroup, S_IFCHR, major(sbuf.st_rdev),
minor(sbuf.st_rdev));
}
}There was a problem hiding this comment.
Thanks, based it on your template. Handled in d5a85d6
There was a problem hiding this comment.
Again this should be removed since it appears unnecessary
There was a problem hiding this comment.
Please check and address my previous comment above, we still need a solution for Mediatek Android 10 devices and I doubt waiting for much longer is a good idea.
There was a problem hiding this comment.
Alright, short-term I am fine without supporting MTK-10 devices for now, but I would really like to have this handled later.
Handled in d5a85d6
zyga
left a comment
zyga
left a comment
There was a problem hiding this comment.
I had a look from top to bottom. Some comments.
I would like to land the move to nvidia/mount logic separately to make the diff shorter. I can propose that separately.
There was a problem hiding this comment.
Should this be checked and should s-c die if it does not work? If it is legitimately allowed to fail, as indicated by the (void) up front, can you add a comment just above
to explain why.
There was a problem hiding this comment.
Can take a deeper look later after $DAYJOB
There was a problem hiding this comment.
Since this runs everywhere, what happens if someone installs, say, anbox and gets /dev/binder. Now all snaps will allow access to the binder. I don't have an immediate idea on how to make this better without some host-level hint.
There was a problem hiding this comment.
The host level hint can again be a check for existance of /system/build.prop, which under Halium distributions always has to exist.
cff6dec to
c4c706e
Compare
|
Fri Jul 3 01:15:22 UTC 2026 No spread failures reported |
c4c706e to
d5a85d6
Compare
|
I've addressed all mentioned requested changes, but I came up with another one which I'll happily add in case I get a "makes sense" from you: How about only adding those libhybris-related AppArmor changes to the template in case it detects Alternatively, I could also reintegrate this one here in order to have a more narrowed idea of "Am I running on an Ubuntu Touch with libhybris device?" in addition to the property file existing: https://github.com/snapcore/snapd/pull/13674/files#diff-d04e67128a47f4820ffbef091b0bdc701ce59b4de9671b109405f19e91eb629dR193 |
9f1fbbc to
b679274
Compare
|
Status update: I've cleaned up the history and split out relevant changes into separate commits. This should make review easier and reflect the PR's actual size more accurately. Note that I consider this patchset complete for an initial bringup, hence I will next focus on making existing interfaces from the unity8 era work on Ubuntu Touch with Lomiri. That work will hopefully mostly be independent of this PR. |
bd1cba0 to
e35b9b1
Compare
|
I figured it would be inappropriate to go for the most minimum implementation and extended the Halium checks to be more thorough, with comments all over the place. This will also be shipped as part of Ubuntu Touch 24.04-2.0. I have one remaining question: ideally I will be able to revert commit b6fba16 to even more guarantee this being a proper Halium setup, so that this cannot be intentionally triggered on a classic system. I would go for allowing Ping @bboozzoo for assistance. |
d8708ba to
b94c06a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13661 +/- ##
==========================================
- Coverage 79.13% 79.03% -0.10%
==========================================
Files 1388 1400 +12
Lines 193923 194928 +1005
Branches 2466 2499 +33
==========================================
+ Hits 153452 154063 +611
- Misses 31277 31599 +322
- Partials 9194 9266 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b94c06a to
1dd27a6
Compare
The way that graphics support works with Snaps on Ubuntu Touch, we need snap-confine to
mount /android into the Snap's environment and set up compatibility symlinks. Then a
gpu content snap like hybris-2404 plugs the gpu-2404 interface with libhybris shipped
inside of the content snap. libhybris' EGL gets loaded and resolves libraries stored
in /android/{system,vendor,odm,apex}/lib{,64}, which results in a working graphics stack.
Determine this by checking for the existance of some Halium-typical paths and only proceed
if the running system matches a Halium one.
Ubuntu Touch differs from typical classic distributions in that it is mounted read-only by default including most of /etc, like /etc/passwd. For that we need the built-in capability of managing users through the extrausers db, which Ubuntu Touch uses like Ubuntu Core did previously. For Ubuntu Touch, in addition to triggering OnClassic being true, introduce an OnTouch property to easily determine the running system being Ubuntu Touch as determined by reading /etc/os-release and inspecting for VARIANT_ID=="touch".
This allows more sophisticated Snaps, like CUPS, to install and run properly.
Ubuntu Touch like Ubuntu Core 16 might require some additional system-data paths to be accessible, so add this when running OnTouch. Additionally, fix installation of the Docker Snap by avoiding conflicting AppArmor expressions. This results in a Docker environment that cannot start devmode Snaps but this is not a requirement for Ubuntu Touch, hence proceed using proposed future AppArmor rules for this.
For libhybris/Halium distributions to support graphics acceleration we need to allow loading an unknown set of Android libraries which, due to their proprietary nature, we don't know the exact dependency chain of beforehand. Luckily we have the OnTouch property to only allow these permissions to be applied on Ubuntu Touch systems where this might be needed. snap-confine already prepares the environment appropriately for us, we just need to punch holes into the security policy of the OpenGL-consuming Snap for an app to consume them. This allows loading proprietary Android OpenGL ES drivers on Ubuntu Touch.
Instead of just the /system/build.prop file, check for some prominently required mountpoints, files and symlinks on Halium systems, including whether they actually have binder devices available in /dev. This should narrow down /android mount setup and binder device permission setup to just Ubuntu Touch on Halium.
- Remove creation of the empty hostfs EXTRA_LIB directory, we use hybris-2404 now - Correct comment about mounting /android into the Snap environment
We don't have access to /proc/mounts and I am uncertain whether shoveling it available would open another can of worms.
Android-based devices on Ubuntu Touch require Halium. Ensure it and libhybris work and get a usable Snap environment.
…tat()'s" This reverts commit b6fba16.
Required for mount-hybris-support to determine a valid Halium environment.
1dd27a6 to
bd3fa3e
Compare
bd3fa3e to
84346ca
Compare
7 participants
This PR contains:
For more information about Halium: https://halium.org
Diverged greatly from the NVIDIA support code it's based on and provides a working environment for confined Qt5, Qt6, SDL, GTK and Flutter Snaps (as long as a GLES renderer is available) like QML Creator (
sudo snap install qmlcreator) with thehybris-2404GNU/Linux drivers.SNAPDENG-36010