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

["Günther Noack" <gnoack3000@gmail.com>]

Hello!

On Sat, Mar 21, 2026 at 10:09:35AM +0100, Mickaël Salaün wrote:
> 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.

I feel that this is still not fully addressed.  I know it is a
triviality to go in circles over this BUILD_BUG, but the current form:

  BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));

checks that the size of client_layer is smaller(!) or equal than the
size of client->num_layers, so it seems that the check is accidentally
done the wrong way around.  It does not fire because the two integers
happen to have the same size, but it feels a bit misleading to leave
it like that?

To keep things moving, and because it feels like a minor (any maybe
optional) issue, I will send V7 now and optimistically do the
following:

* Copy the BUILD_BUG_ON and matching comment from domain_is_checked()
  as you initially requested.
* Make a constructive proposal that rewrites the check in both
  functions, to check that all numbers from -1 to
  LANDLOCK_MAX_NUM_LAYERS are representable (and the loop therefore
  terminates).  In my understanding that was the intention here.

–Günther