Re: [RFC PATCH v3 01/19] landlock: Support socket access-control

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

Happy New Year!

On Tue, Dec 24, 2024 at 07:55:01PM +0300, Mikhail Ivanov wrote:
> The bitmask approach leads to a complete refactoring of socket rule
> storage. This shouldn't be a big issue, since we're gonna need
> multiplexer for insert_rule(), find_rule() with a port range feature
> anyway [1]. But it seems that the best approach of storing rules
> composed of bitmasks is to store them in linked list and perform
> linear scan in landlock_find_rule(). Any other approach is likely to
> be too heavy and complex.
> 
> Do you think such refactoring is reasonable?
> 
> [1] https://github.com/landlock-lsm/linux/issues/16

The way I understood it in your mail from Nov 28th [1], I thought that the
bitmasks would only exist at the UAPI layer so that users could more
conveniently specify multiple "types" at the same time.  In other
words, a rule which is now expressed as

  {
    .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
    .family = AF_INET,
    .types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
    .protocol = 0,
  },

used to be expressed like this (without bitmasks):

  {
    .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
    .family = AF_INET,
    .type = SOCK_STREAM,
    .protocol = 0,
  },
  {
    .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
    .family = AF_INET,
    .type = SOCK_DGRAM,
    .protocol = 0,
  },

I do not understand why this convenience feature in the UAPI layer
requires a change to the data structures that Landlock uses
internally?  As far as I can tell, struct landlock_socket_attr is only
used in syscalls.c and converted to other data structures already.  I
would have imagined that we'd "unroll" the specified bitmasks into the
possible combinations in the add_rule_socket() function and then call
landlock_append_socket_rule() multiple times with each of these?


That being said, I am not a big fan of red-black trees for such simple
integer lookups either, and I also think there should be something
better if we make more use of the properties of the input ranges. The
question is though whether you want to couple that to this socket type
patch set, or rather do it in a follow up?  (So far we have been doing
fine with the red black trees, and we are already contemplating the
possibility of changing these internal structures in [2].  We have
also used RB trees for the "port" rules with a similar reasoning,
IIRC.)

Regarding the port range feature, I am also not sure whether the data
structure for that would even be similar?  Looking for a containment
in a set of integer ranges is a different task than looking for an
exact match in a non-contiguous set of integers.

In any case, I feel that for now, an exact look up in the RB tree
would work fine as a generic solution (especially considering that the
set of added rules is probably usually small).  IMHO, finding a more
appropriate data structure might be a can of worms that could further
delay the patch set and which might better be discussed separately.

WDYT?

–Günther

[1] https://lore.kernel.org/all/eafd855d-2681-8dfd-a2be-9c02fc07050d@huawei-partners.com/
[2] https://github.com/landlock-lsm/linux/issues/1