6/10/2024 11:03 AM, Günther Noack wrote:
On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:6/4/2024 11:22 PM, Günther Noack wrote: I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in really strange way (see landlock/limits.h): #define LANDLOCK_SHIFT_ACCESS_SOCKET LANDLOCK_NUM_ACCESS_SOCKET With this definition, socket access mask overlaps the fs access mask in ruleset->access_masks[layer_level]. That's why landlock_get_fs_access_mask() returns non-zero mask in hook_file_open(). So, the macro must be defined in this way: #define LANDLOCK_SHIFT_ACCESS_SOCKET (LANDLOCK_NUM_ACCESS_NET + LANDLOCK_NUM_ACCESS_FS) With this fix, open() doesn't fail in your example. I'm really sorry that I somehow made such a stupid typo. I will try my best to make sure this doesn't happen again.I found that we had the exact same bug with a wrongly defined "SHIFT" value in [1]. Maybe we should define access_masks_t as a bit-field rather than doing the bit-shifts by hand. Then the compiler would keep track of the bit-offsets automatically. Bit-fields have a bad reputation, but in my understanding, this is largely because they make it hard to control the exact bit-by-bit layout. In our case, we do not need such an exact control though, and it would be fine. To quote Linus Torvalds on [2], Bitfields are fine if you don't actually care about the underlying format, and want gcc to just randomly assign bits, and want things to be convenient in that situation. Let me send you a proposal patch which replaces access_masks_t with a bit-field and removes the need for the "SHIFT" definition, which we already got wrong in two patch sets now. It has the additional benefit of making the code a bit shorter and also removing a few static_assert()s which are now guaranteed by the compiler. —Günther [1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@xxxxxxxxxx/ [2] https://yarchive.net/comp/linux/bitfields.html
Thank you, Günther! It really looks more clear. This patch should be applied to Landlock separately, right?
