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

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

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)));

(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.)

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.

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

–Günther