On Thu, Mar 06, 2025 at 02:58:01AM +0000, Tingmao Wang wrote: > On 3/4/25 19:49, Mickaël Salaün wrote: > > > We could indeed have a pointer in the landlock_hierarchy and have a > > dedicated bit in each layer's access_masks to indicate that this layer > > is supervised. This should simplify the whole patch series. > > That seems sensible. I did consider using the landlock_hierarchy, but chose > the current way as it initially seemed more sensible, but on second thought > this means that we have to carefully increment all the refcounts on domain > merge etc. On the other hand storing the supervisor pointer in the > hierarchy, if we have an extra bit in struct access_masks then we can > quickly determine if supervisors are involved without effectively walking a > linked list, which is nice. Right > > Actually, just to check, is the reason why we have the access_masks FAM in > the ruleset purely for performance? Initially I wasn't sure if each layer > correspond 1-to-1 with landlock_hierarchy, since otherwise it seemed to me > you could just put the access mask in the hierarchy too. Yes, we could put the access rights in the hierarchy, but that would involve walking through the hierarchy to know if Landlock should actually handle (i.e. allow or potentially deny) an access request. Landlock is designed in a way that makes legitimate/allowed access as fast as possible (there is still room for improvement though). In the case of the supervisor feature, it should mainly be used to dynamically allow access which are statically denied for one layer. And because it will require a round trip to user space anyway, the performance impact of putting the supervisor pointer in landlock_hierarchy is negligible. Initially the purpose of landlock_hierarchy was to be able to compare domains (for ptrace and later scope restrictions), whereas the landlock_ruleset is to store immutable data (without references) when used as a domain. With the audit feature, the landlock_hierarchy will also contain domain's shared/mutable states and pointers that should only be rarely accessed (i.e. only for denials). So, in a nutshell landlock_ruleset as a domain should stay minimal and improve data locality to speed up allowed access requests. We could decorrelate the current content of landlock_hierarchy from the shared data, but I think it would only be meaningful if this data is significant (see the landlock_details pointer in the audit patch series). > In other words, is it right to assume that, if a domain has 3 layers, for > example, then domain->hierarchy correspond to the third layer, > domain->hierarchy->parent correspond to the second, and > d->h->parent->parent would be the first layer's hierarchy? Yes, that is always the case for a domain. If we create the supervisor FD with landlock_restrict_self(2), then we'll not have to add a new pointer to landlock_ruleset, but only directly to landlock_hierarchy.
