The goal of these patches is to avoid calling capable() unconditionally
in simple_xattr_list(), which causes issues under SELinux (see
explanation in the second patch).
The first patch tries to make this change safer by converting
simple_xattrs to use the RCU mechanism, so that capable() is not called
while the xattrs->lock is held. I didn't find evidence that this is an
issue in the current code, but it can't hurt to make that change
either way (and it was quite straightforward).
Ondrej Mosnacek (2):
fs: convert simple_xattrs to RCU list
fs: don't call capable() prematurely in simple_xattr_list()
fs/xattr.c | 39 +++++++++++++++++++++++----------------
include/linux/xattr.h | 1 +
2 files changed, 24 insertions(+), 16 deletions(-)
--
2.37.2
On Thu, Sep 01, 2022 at 05:26:30PM +0200, Ondrej Mosnacek wrote:
> The goal of these patches is to avoid calling capable() unconditionally
> in simple_xattr_list(), which causes issues under SELinux (see
> explanation in the second patch).
>
> The first patch tries to make this change safer by converting
> simple_xattrs to use the RCU mechanism, so that capable() is not called
> while the xattrs->lock is held. I didn't find evidence that this is an
> issue in the current code, but it can't hurt to make that change
> either way (and it was quite straightforward).
Hey Ondrey,
There's another patchset I'd like to see first which switches from a
linked list to an rbtree to get rid of performance issues in this code
that can be used to dos tmpfs in containers:
https://lore.kernel.org/lkml/[email protected]
I don't think Vasily has time to continue with this so I'll just pick it
up hopefully this or the week after LPC.
Christian
On Mon, Sep 5, 2022 at 11:08 AM Christian Brauner <[email protected]> wrote:
> On Thu, Sep 01, 2022 at 05:26:30PM +0200, Ondrej Mosnacek wrote:
> > The goal of these patches is to avoid calling capable() unconditionally
> > in simple_xattr_list(), which causes issues under SELinux (see
> > explanation in the second patch).
> >
> > The first patch tries to make this change safer by converting
> > simple_xattrs to use the RCU mechanism, so that capable() is not called
> > while the xattrs->lock is held. I didn't find evidence that this is an
> > issue in the current code, but it can't hurt to make that change
> > either way (and it was quite straightforward).
>
> Hey Ondrey,
>
> There's another patchset I'd like to see first which switches from a
> linked list to an rbtree to get rid of performance issues in this code
> that can be used to dos tmpfs in containers:
>
> https://lore.kernel.org/lkml/[email protected]
>
> I don't think Vasily has time to continue with this so I'll just pick it
> up hopefully this or the week after LPC.
Hm... does rbtree support lockless traversal? Because if not, that
would make it impossible to fix the issue without calling capable()
inside the critical section (or doing something complicated), AFAICT.
Would rhashtable be a workable alternative to rbtree for this use
case? Skimming <linux/rhashtable.h> it seems to support both lockless
lookup and traversal using RCU. And according to its manpage,
*listxattr(2) doesn't guarantee that the returned names are sorted.
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
On Mon, Sep 05, 2022 at 12:15:01PM +0200, Ondrej Mosnacek wrote:
> On Mon, Sep 5, 2022 at 11:08 AM Christian Brauner <[email protected]> wrote:
> > On Thu, Sep 01, 2022 at 05:26:30PM +0200, Ondrej Mosnacek wrote:
> > > The goal of these patches is to avoid calling capable() unconditionally
> > > in simple_xattr_list(), which causes issues under SELinux (see
> > > explanation in the second patch).
> > >
> > > The first patch tries to make this change safer by converting
> > > simple_xattrs to use the RCU mechanism, so that capable() is not called
> > > while the xattrs->lock is held. I didn't find evidence that this is an
> > > issue in the current code, but it can't hurt to make that change
> > > either way (and it was quite straightforward).
> >
> > Hey Ondrey,
> >
> > There's another patchset I'd like to see first which switches from a
> > linked list to an rbtree to get rid of performance issues in this code
> > that can be used to dos tmpfs in containers:
> >
> > https://lore.kernel.org/lkml/[email protected]
> >
> > I don't think Vasily has time to continue with this so I'll just pick it
> > up hopefully this or the week after LPC.
>
> Hm... does rbtree support lockless traversal? Because if not, that
The rfc that Vasily sent didn't allow for that at least.
> would make it impossible to fix the issue without calling capable()
> inside the critical section (or doing something complicated), AFAICT.
> Would rhashtable be a workable alternative to rbtree for this use
> case? Skimming <linux/rhashtable.h> it seems to support both lockless
> lookup and traversal using RCU. And according to its manpage,
> *listxattr(2) doesn't guarantee that the returned names are sorted.
I've never used the rhashtable infrastructure in any meaningful way. All
I can say from looking at current users that it looks like it could work
well for us here:
struct simple_xattr {
struct rhlist_head rhlist_head;
char *name;
size_t size;
char value[];
};
static const struct rhashtable_params simple_xattr_rhashtable = {
.head_offset = offsetof(struct simple_xattr, rhlist_head),
.key_offset = offsetof(struct simple_xattr, name),
or sm like this.