Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes

[Casey Schaufler <casey@schaufler-ca.com>]

On 5/27/2020 5:38 AM, KP Singh wrote:
> On 26-May 22:08, Christoph Hellwig wrote:
>> On Tue, May 26, 2020 at 06:33:34PM +0200, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>>>
>>> Similar to bpf_local_storage for sockets, add local storage for inodes.
>>> The life-cycle of storage is managed with the life-cycle of the inode.
>>> i.e. the storage is destroyed along with the owning inode.
>>>
>>> Since, the intention is to use this in LSM programs, the destruction is
>>> done after security_inode_free in __destroy_inode.
>> NAK onbloating the inode structure.  Please find an out of line way
>> to store your information.
> The other alternative is to use lbs_inode (security blobs) and we can
> do this without adding fields to struct inode.

This is the correct approach, and always has been. This isn't the
first ( or second :( ) case where the correct behavior for an LSM
has been pretty darn obvious, but you've taken a different approach
for no apparent reason.

> Here is a rough diff (only illustrative, won't apply cleanly) of the
> changes needed to this patch:
>
>  https://gist.github.com/sinkap/1d213d17fb82a5e8ffdc3f320ec37d79

To do just a little nit-picking, please use bpf_inode() instead of
bpf_inode_storage(). This is in keeping with the convention used by
the other security modules. Sticking with the existing convention
makes it easier for people (and tools) that work with multiple
security modules.

> Once tracing has gets a whitelist based access to inode storage, I
> guess it, too, can use bpf_local_storage for inodes

Only within the BPF module. Your sentence above is slightly garbled,
so I'm not really sure what you're saying, but if you're suggesting
that tracing code outside of the BPF security module can use the
BPF inode data, the answer is a resounding "no".

>  if CONFIG_BPF_LSM
> is enabled. Does this sound reasonable to the BPF folks?
>
> - KP
>
>