[v2,0/4] selinux: fix race when removing selinuxfs entries

Message

Ondrej Mosnacek Aug. 1, 2019, 2:02 p.m. UTC

After hours and hours of getting familiar with dcache and debugging,
I think I finally found a solution that works and hopefully stands a
chance of being committed.

The series still doesn't address the lack of atomicity of the policy
reload transition, but this is part of a wider problem and can be
resolved later. Let's fix at least the userspace-triggered lockup
first.

Changes since v1:
 - switch to hopefully proper and actually working solution instead
   of the horrible mess I produced last time...
v1: https://lore.kernel.org/selinux/[e-mail]/T/

Ondrej Mosnacek (4):
  d_walk: optionally lock also parent inode
  d_walk: add leave callback
  dcache: introduce d_genocide_safe()
  selinux: use d_genocide_safe() in selinuxfs

 fs/dcache.c                  | 87 +++++++++++++++++++++++++++++++-----
 include/linux/dcache.h       |  1 +
 security/selinux/selinuxfs.c |  2 +-
 3 files changed, 77 insertions(+), 13 deletions(-)

Comments

Al Viro Aug. 1, 2019, 4:09 p.m. UTC | #1

On Thu, Aug 01, 2019 at 04:02:39PM +0200, Ondrej Mosnacek wrote:
> After hours and hours of getting familiar with dcache and debugging,
> I think I finally found a solution that works and hopefully stands a
> chance of being committed.
> 
> The series still doesn't address the lack of atomicity of the policy
> reload transition, but this is part of a wider problem and can be
> resolved later. Let's fix at least the userspace-triggered lockup
> first.

I don't think this is the right approach.  Consider the related problem:
what happens if somebody has mounted something upon a selinuxfs file?
That is the hard part here, and AFAICS your variant doesn't help it
at all...

Ondrej Mosnacek Aug. 8, 2019, 7:59 a.m. UTC | #2

On Thu, Aug 1, 2019 at 6:09 PM Al Viro <[e-mail]> wrote:
> On Thu, Aug 01, 2019 at 04:02:39PM +0200, Ondrej Mosnacek wrote:
> > After hours and hours of getting familiar with dcache and debugging,
> > I think I finally found a solution that works and hopefully stands a
> > chance of being committed.
> >
> > The series still doesn't address the lack of atomicity of the policy
> > reload transition, but this is part of a wider problem and can be
> > resolved later. Let's fix at least the userspace-triggered lockup
> > first.
>
> I don't think this is the right approach.  Consider the related problem:
> what happens if somebody has mounted something upon a selinuxfs file?
> That is the hard part here, and AFAICS your variant doesn't help it
> at all...

But that's another independent problem and it's not even fixed in
debugfs, which for now I'm treating as the baseline as I don't know of
any other filesystem that needs to remove its own directory trees in a
similar way.

I get that you don't want me to add a new function to the dcache API
that isn't bulletproof (and what I wrote here is apparently still far
from it), but you also previously said that I shouldn't open-code this
stuff in selinuxfs.c... I don't think I have the wits to write a
common function that handles all the possible issues, but I still want
to fix at least this one scenario (dcache_readdir() vs.
sel_remove_entries()).

Is there some way I could do this without getting a NACK from you? For
example, I thought of taking what is now debugfs_remove[_recursive]()
out of debugfs into, say, fs/libfs.c (providing some optional callback
to allow debugfs to do its __debugfs_file_removed() business) and use
this function(s) from both debugfs and selinuxfs. This way we could
later fix the leftover mount issue in one place and both filesystems
would (hopefully) immediately benefit from it. Would that be a
feasible way forward?

Thanks,

Ondrej Mosnacek Sept. 3, 2019, 10:56 a.m. UTC | #3

On Thu, Aug 8, 2019 at 9:59 AM Ondrej Mosnacek <[e-mail]> wrote:
> On Thu, Aug 1, 2019 at 6:09 PM Al Viro <[e-mail]> wrote:
> > On Thu, Aug 01, 2019 at 04:02:39PM +0200, Ondrej Mosnacek wrote:
> > > After hours and hours of getting familiar with dcache and debugging,
> > > I think I finally found a solution that works and hopefully stands a
> > > chance of being committed.
> > >
> > > The series still doesn't address the lack of atomicity of the policy
> > > reload transition, but this is part of a wider problem and can be
> > > resolved later. Let's fix at least the userspace-triggered lockup
> > > first.
> >
> > I don't think this is the right approach.  Consider the related problem:
> > what happens if somebody has mounted something upon a selinuxfs file?
> > That is the hard part here, and AFAICS your variant doesn't help it
> > at all...
>
> But that's another independent problem and it's not even fixed in
> debugfs, which for now I'm treating as the baseline as I don't know of
> any other filesystem that needs to remove its own directory trees in a
> similar way.
>
> I get that you don't want me to add a new function to the dcache API
> that isn't bulletproof (and what I wrote here is apparently still far
> from it), but you also previously said that I shouldn't open-code this
> stuff in selinuxfs.c... I don't think I have the wits to write a
> common function that handles all the possible issues, but I still want
> to fix at least this one scenario (dcache_readdir() vs.
> sel_remove_entries()).
>
> Is there some way I could do this without getting a NACK from you? For
> example, I thought of taking what is now debugfs_remove[_recursive]()
> out of debugfs into, say, fs/libfs.c (providing some optional callback
> to allow debugfs to do its __debugfs_file_removed() business) and use
> this function(s) from both debugfs and selinuxfs. This way we could
> later fix the leftover mount issue in one place and both filesystems
> would (hopefully) immediately benefit from it. Would that be a
> feasible way forward?

Ping?