On Mon, Aug 26, 2024, Vipin Sharma wrote:
> On 2024-08-26 07:34:35, Sean Christopherson wrote:
> > > } else {
> > > /*
> > > * Try again in future if the page is still in the
> > > * list
> > > */
> > > spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> > > if (!list_empty(&sp->possible_nx_huge_page_link))
> > > list_move_tail(&sp->possible_nx_huge_page_link,
> > > kvm-> &kvm->arch.possible_nx_huge_pages);
> >
> > This is unsafe. The only thing that prevents a use-after-free of "sp" is the fact
> > that this task holds rcu_read_lock(). The sp could already been queued for freeing
> > via call_rcu().
>
> Before call_rcu() happens, that page will be removed from
> kvm->arch.possible_nx_huge_pages list in handle_remove_pt() via
> tdp_mmu_unlink_sp() using kvm->arch.tdp_mmu_pages_lock.
Gah, my bad, I inverted the list_empty() check when reading.
> Here, we are using the same lock and checking if page is in the list or not.
> If it is in the list move to end and if it is not then don't do anything.
>
> Am I missing something else? Otherwise, this logic seems correct to me.
Nope, poor code reading on my part, especially since the _move_ action should have
made it obvious the SP is still live.
> Overall, I will be using your example code, so you won't see this code
> in next version but just want to understand the concern with this else
> part.
