Re: [PATCH 1/2] kernfs: add kernfs_ops.free operation to free resources tied to the file

[Christian Brauner]

On Thu, Jun 29, 2023 at 05:59:07PM -0700, Suren Baghdasaryan wrote:
> On Wed, Jun 28, 2023 at 2:50 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> >
> > On Wed, Jun 28, 2023 at 1:34 PM Tejun Heo <tj@xxxxxxxxxx> wrote:
> > >
> > > Hello, Suren.
> > >
> > > On Wed, Jun 28, 2023 at 01:12:23PM -0700, Suren Baghdasaryan wrote:
> > > > AFAIU all other files that handle polling rely on f_op->release()
> > > > being called after all the users are gone, therefore they can safely
> > > > free their resources. However kernfs can call ->release() while there
> > > > are still active users of the file. I can't use that operation for
> > > > resource cleanup therefore I was suggesting to add a new operation
> > > > which would be called only after the last fput() and would guarantee
> > > > no users. Again, I'm not an expert in this, so there might be a better
> > > > way to handle it. Please advise.
> > >
> > > So, w/ kernfs, the right thing to do is making sure that whatever is exposed
> > > to the kernfs user is terminated on removal - ie. after kernfs_ops->release
> > > is called, the ops table should be considered dead and there shouldn't be
> > > anything left to clean up from the kernfs user side. You can add abstraction
> > > kernfs so that kernfs can terminate the calls coming down from the higher
> > > layers on its own. That's how every other operation is handled and what
> > > should happen with the psi polling too.
> >
> > I'm not sure I understand. The waitqueue head we are freeing in
> > ->release() can be accessed asynchronously and does not require any
> > kernfs_op call. Here is a recap of that race:
> >
> >                                                 do_select
> >                                                       vfs_poll
> > cgroup_pressure_release
> >     psi_trigger_destroy
> >         wake_up_pollfree(&t->event_wait) -> unblocks vfs_poll
> >         synchronize_rcu()
> >         kfree(t) -> frees waitqueue head
> >                                                      poll_freewait() -> UAF
> >
> > Note that poll_freewait() is not part of any kernel_op, so I'm not
> > sure how adding an abstraction kernfs would help, but again, this is
> > new territory for me and I might be missing something.
> >
> > On a different note, I think there might be an easy way to fix this.
> > What if psi triggers reuse kernfs_open_node->poll waitqueue head?
> > Since we are overriding the ->poll() method, that waitqueue head is
> > unused AFAIKT. And best of all, its lifecycle is tied to the file's
> > lifecycle, so it does not have the issue that trigger waitqueue head
> > has. In the trigger I could simply store a pointer to that waitqueue
> > and use it. Then in ->release() freeing trigger would not affect the
> > waitqueue at all. Does that sound sane?
> 
> I think this approach is much cleaner and I'm guessing that's in line
> with what Tejun was describing (maybe it's exactly what he was telling
> me but it took time for me to get it). Posted the patch implementing
> this approach here:
> https://lore.kernel.org/all/20230630005612.1014540-1-surenb@xxxxxxxxxx/

I'm sure that how things work today in kernfs are there for a reason.A

What I'm mostly reacting to is that there's a kernfs_ops->release()
method which mirrors f_op->release() but can be called when there are
still users which is counterintuitive for release semantics. And that
ultimately caused this UAF issue which was rather subtle given how long
it took to track down the root cause.

A rmdir() isn't triggering a f_op->release() if there are still file
references but it's apparently triggering a kernfs_ops->release(). It
feels like this should at least be documented in struct kernfs_ops...