And I strongly encourage our downstream in the same way :) I try, I try. However, I am a n00b here (thanks for merging "my" first linux patch). Looking at this code, I was wondering, why isn't SELinux labelling completely orthogonal to the fs type? Is this a measurable memory/performance thing? On Tue, Feb 11, 2020 at 7:17 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Thu, Feb 6, 2020 at 1:12 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > > On 2/6/20 12:41 PM, Steven Moreland wrote: > > > On Thu, Feb 6, 2020 at 9:35 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > > >> > > >> On 2/6/20 12:21 PM, Stephen Smalley wrote: > > >>> On 2/6/20 11:55 AM, Steven Moreland wrote: > > >>>> From: Connor O'Brien <connoro@xxxxxxxxxx> > > >>>> > > >>>> Add support for genfscon per-file labeling of bpffs files. This allows > > >>>> for separate permissions for different pinned bpf objects, which may > > >>>> be completely unrelated to each other. > > >>> > > >>> Do you want bpf fs to also support userspace labeling of files via > > >>> setxattr()? If so, you'll want to also add it to > > >>> selinux_is_genfs_special_handling() as well. > > >>> > > > > > > Android doesn't currently have this use case. > > > > > >>> The only caveat I would note here is that it appears that bpf fs > > >>> supports rename, link, unlink, rmdir etc by userspace, which means that > > >>> name-based labeling via genfscon isn't necessarily safe/stable. See > > >>> https://github.com/SELinuxProject/selinux-kernel/issues/2 > > >>> > > > > > > Android restricts ownership of these files to a single process (bpfloader) and > > > so this isn't a concern in our architecture. Is it a concern in general? > > > > I guess if the inodes are pinned in memory, then only the original name > > under which the file is created will be relevant to determining the > > label and subsequent rename/link operations won't have any effect. So as > > long as the bpfloader creates the files with the same names being > > specified in policy, that should line up and be stable for the lifecycle > > of the inode. > > > > The alternative model is to have bpfloader look up a context from the > > userspace file_contexts configuration via selabel_lookup(3) and friends, > > and set it on the file explicitly. That's what e.g. ueventd does for > > device nodes. However, one difference here is that you could currently > > only do this via setxattr()/setfilecon() after creating the file so that > > the file would temporarily exist in the default label for bpf fs, if > > that matters. ueventd can instead use setfscreatecon(3) before creating > > the file so that it is originally created in the right label but that > > requires the filesystem to call security_inode_init_security() from its > > function that originally creates the inode, which tmpfs/devtmpfs does > > but bpf does not. So you'd have to add that to the bpf filesystem code > > if you wanted to support setfscreatecon(3) on it. > > Considering the relative maturity of bpf, and bpffs, I think it's okay > to take this small step right now, with the understanding that more > work may need to be done, depending on how this is generally adopted > by distros and users (for those of you not following the other thread, > I've merged the v3 draft of this patch). > > However, I've been noticing a trend from the Android folks of tossing > patches over the wall without much thought beyond the Android use > case. I understand the Android devs have a job to do, and products to > focus on, but I would strongly encourage them to think a bit longer > about more general use cases before submitting patches upstream. > > -- > paul moore > www.paul-moore.com
