Hi Tejun,
On Wed, Jan 30, 2019 at 6:09 PM Tejun Heo <tj@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Wed, Jan 30, 2019 at 12:41:50PM +0100, Ondrej Mosnacek wrote:
> > @@ -673,6 +698,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> > goto err_out3;
> > }
> >
> > + if (parent) {
> > + ret = kernfs_node_init_security(parent, kn);
> > + if (ret)
> > + goto err_out3;
> > + }
>
> So, doing this unconditionally isn't a good idea. kernfs doesn't use
> the usual dentry/inode because there are machines with 6, even 7 digit
> number of kernfs nodes and some of them even failed to boot due to
> memory shortage. Please don't blow it up by default.
Hm, I see... basically the only thing that gets allocated in
kernfs_node_init_security() by default (at least under SELinux/ no
LSM) is the kernfs_iattrs structures, so I assume you are pointing at
that. I think this can be easily fixed, if we again use the assumption
that whenever the parent node has only default attributes
(parent->iattrs == NULL), then the child node should also have just
default attributes (and so we don't need to call kernfs_iattrs() on it
nor call the security hook). Something along these lines:
[...]
+static int kernfs_node_init_security(struct kernfs_node *parent,
+ struct kernfs_node *kn)
+{
+ struct kernfs_iattrs *attrs, *pattrs;
+ struct qstr q;
+
+ pattrs = parent->iattrs;
+ if (!pattrs)
+ return 0;
+
+ attrs = kernfs_iattrs(kn);
+ if (!attrs)
+ return -ENOMEM;
+
+ q.name = kn->name;
+ q.hash_len = hashlen_string(parent, kn->name);
[...]
Technically this might make some LSMs unhappy, if they want to set
some non-default context even if parent is all default, but this is
already impossible now and in this case I think we have no better
choice than sacrificing a bit of flexibility for memory efficiency,
which is apparently critical here.
Tejun, Casey, would the above modification be fine with you?
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
