Re: Potential improvements to mount-flatpak

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3046 bytes --]

On 7/22/26 04:17, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> I looked at the mount-flatpak codebase and noticed some potential
>> improvements for the future:
>>
>> - The code uses std::fs::write to write to files in the target
>>   directory.  This is safe because the VM isn't running, but it might
>>   be better to use libpathrs.
> 
> I think it's fine.

Makes sense.

>> - The code doesn't validate the user-provided app ID, resulting in
>>   worse error messages than otherwise possible.  App IDs are always
>>   D-Bus well-known names, so there is no risk of the rules changing
>>   in future versions of Flatpak.
> 
> Even though Flatpak 2 won't use D-Bus?

I think I forgot that.  Is there a document with the list of Flatpak
2 changes anywhere?

>> - Passing something that isn't a flatpak repository as a parameter
>>   will result in a confusing "no such file or directory" error.
> 
> Could check that if it's cheap, since this is user-facing.  I would not
> spend much time on it though, since ultimately this should not be the
> interface people use.

Makes sense.

>> - Various checks for corrupt repositories could be added, such as:
>>   - Wrong number of slashes in "current" symlink.
>>   - Wrong number of slashes in runtime path.
>>   - Bad "active" symlink.
> 
> I'm not sure I see the value?  Does normal Flatpak do these checks?
> I don't think we need to check everything that could possibly go wrong
> if it wouldn't be a security concern.

I don't see any reason either.  There is a trick one can do to
improve worst-case runtime, and that is to not use RESOLVE_BENEATH
or RESOLVE_IN_ROOT.  Instead, one uses RESOLVE_NO_SYMLINKS |
RESOLVE_NO_MAGICLINKS and checks that the path has no ".." components
and is not absolute.  My understanding is that RESOLVE_BENEATH
and RESOLVE_IN_ROOT require heavyweight kernel-side checks to guard
against race conditions, and these can produce false positives.
The other approach doesn't require these checks and so is more reliable.
libpathrs resolves this with an ugly retry loop.

It's up to you whether this change is worth making.

>> - The code that resolves the "active" symlink is duplicated.
> 
> I think there's already a patch sitting at the bottom of my inbox for
> you from this.
> 
>> - The target mount point is not made "noexec".
> 
> Sounds like a good improvement.

Yup, although all of this is in a separate mount namespace and I think
only virtiofsd runs in that namespace.

>> - mount_setattr() is called on a directory FD.  I suspect this
>>   changes parameters of the source mount.  It might be better to use
>>   open_tree_attr() with OPEN_TREE_CLONE instead.
> 
> Well, does it change the source mount or doesn't it?  If there's a bug,
> fix it; if there isn't, don't.  (I do think you are probably right though.)

There's no actual bug.  It was opened with open_tree() with OPEN_TREE_CLONE.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]