Re: [PATCH v6 3/9] landlock: Control pathname UNIX domain socket resolution by path

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Mar 20, 2026 at 11:25:04PM +0100, Günther Noack wrote:
> On Fri, Mar 20, 2026 at 06:51:13PM +0100, Mickaël Salaün wrote:
> > On Fri, Mar 20, 2026 at 05:15:40PM +0100, Günther Noack wrote:
> > > On Wed, Mar 18, 2026 at 05:52:48PM +0100, Mickaël Salaün wrote:
> > > > On Sun, Mar 15, 2026 at 11:21:44PM +0100, Günther Noack wrote:
> > > > > + * @client: Client domain
> > > > > + * @server: Server domain
> > > > > + * @masks: Layer access masks to unmask
> > > > > + * @access: Access bit that controls scoping
> > > > > + */
> > > > > +static void unmask_scoped_access(const struct landlock_ruleset *const client,
> > > > > +				 const struct landlock_ruleset *const server,
> > > > > +				 struct layer_access_masks *const masks,
> > > > > +				 const access_mask_t access)
> > > > > +{
> > > > > +	int client_layer, server_layer;
> > > > > +	const struct landlock_hierarchy *client_walker, *server_walker;
> > > > > +
> > > > > +	/* This should not happen. */
> > > > > +	if (WARN_ON_ONCE(!client))
> > > > > +		return;
> > > > > +
> > > > > +	/* Server has no Landlock domain; nothing to clear. */
> > > > > +	if (!server)
> > > > > +		return;
> > > > > +
> > > > 
> > > > Please also copy the BUILD_BUG_ON() from domain_is_scoped().
> > > 
> > > I don't understand what this check is good for.  It says:
> > > 
> > > /*
> > >  * client_layer must be a signed integer with greater capacity
> > >  * than client->num_layers to ensure the following loop stops.
> > >  */
> > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > 
> > > For the loop to terminate, in my understanding, client_layer must be
> > > able to store client->num_layers - 1 down to - 1, but that is anyway a
> > > given since num_layers can't exceed 16 and client_layer is signed.  It
> > > seems that the termination of this would anyway be caught in our tests
> > > as well?
> > > 
> > > Could you please clarify what this BUILD_BUG_ON() is trying to assert?
> > 
> > The intent was to make sure client_layer is indeed an int and not an
> > unsigned int for instance.  Hopefully tests catch that but using a
> > build-time assert catch the potential issue and document it.  Also, it
> > would be weird to not have the same checks in both copies of code.
> 
> It isn't clear to me why the BUILD_BUG_ON is checking based on the
> sizeof() of these variables then.  The number of bytes in an integer
> type is independent of its signedness, after all.  Should we rather do
> this maybe?
> 
>   /*
>    * client_layer must be a signed integer to ensure the following
>    * loop stops.
>    */
>   BUILD_BUG_ON(!is_signed_type(typeof(client_layer)));

I didn't know about this macro, looks good.

> 
> (Although that is also a bit of a triviality given that the same
> variable is being defined as a signed integer just a few lines above,
> but at least it is very explicit about it.)

Yeah, I know, but I added this canary check because I was bitten by a
bug.  Even if it might be trivial now, it might help when working on
other parts, and it's just a build-time check that also serves as doc.
If in doubt, let's keep build-time checks that were most likely added
for a good reason.  I prefer to have build-time errors than run-time
ones.

> 
> I'd drop the remark about the capacity, as even a signed 8-bit integer
> is large enough to hold the layer indices and the -1.

Large enough for now...  All these checks are useful to easily spot
issues if/when the current invariant change (e.g. if one day we extend
the number of max layers).  Integer C types can silently cast to other
integer types (with different capacity or sign/unsigned), which might be
the source of bugs.  So yeah, please keep the current capacity check and
add the sign one, it won't do any harm.

> 
> Does that sound reasonable?  I can do it in the other function as well
> if you want to keep things consistent.

Yes, please update the other function as well.