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

[Suren Baghdasaryan <surenb@google.com>]

On Wed, Jun 28, 2023 at 2:50 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jun 28, 2023 at 1:34 PM Tejun Heo <tj@kernel.org> 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@google.com/

>
>
> >
> > Thanks.
> >
> > --
> > tejun