Re: [PATCH] eventpoll: pin files while checking reverse paths

[Mateusz Guzik]

On Fri, Jul 24, 2026 at 5:58 AM Guidong Han <2045gemini@xxxxxxxxx> wrote:
>
> On Fri, Jul 24, 2026 at 3:51 AM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
> >
> > On Sat, Jul 18, 2026 at 06:44:06PM +0800, Guidong Han wrote:
> > > Commit 319c15174757 ("epoll: take epitem list out of struct file")
> > > intentionally removed temporary file references from the reverse path
> > > check list. At the time, both epitems and their files were freed after
> > > an RCU grace period, so unlist_file() could obtain file->f_lock through
> > > an epitem while clear_tfile_check_list() held rcu_read_lock().
> > >
> > > Commit 0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU") made
> > > struct file SLAB_TYPESAFE_BY_RCU and removed its RCU-delayed freeing.
> > > RCU still protects the epitem, but no longer keeps the referenced file
> > > from being freed and reused. A concurrent close can therefore make
> > > unlist_file() lock or unlock f_lock in a recycled file object.
> > >
> > > This violates the documented SLAB_TYPESAFE_BY_RCU rule requiring a
> > > reference before acquiring an object's lock. The race was reproduced,
> > > causing a wild unlock of f_lock in a recycled file and breaking its
> > > mutual exclusion.
> > >
> > > Add ->file to epitems_head to remember the pinned file independently of
> > > ->epitems. A concurrent EPOLL_CTL_DEL can empty ->epitems before the head
> > > is unlisted, leaving no epi->ffd.file from which to drop the reference.
> > >
> > > In list_file(), acquire the reference before adding the head to the
> > > check list. The caller either owns a reference or holds the ep->mtx for
> > > the epitem leading to the file. In the latter case, file_ref_get() can
> > > fail after the last reference is dropped, but eventpoll_release_file()
> > > must acquire the same mutex before the file can be freed. The dying leaf
> > > can be skipped because removing links cannot increase the reverse path
> > > count.
> > >
> > > In unlist_file(), epnested_mutex excludes another list_file() or
> > > unlist_file(), while head->next prevents a concurrent EPOLL_CTL_DEL from
> > > freeing the head. Save head->file locally, clear it with head->next
> > > under f_lock, and drop the reference after the RCU-protected operation.
> > >
> > > Reported-by: Qi Tang <tpluszz77@xxxxxxxxx>
> > > Reported-by: Junxi Qian <qjx1298677004@xxxxxxxxx>
> > > Fixes: 0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Guidong Han <2045gemini@xxxxxxxxx>
> > > ---
> > > SLAB_TYPESAFE_BY_RCU allows a slab slot to be reused while an RCU reader
> > > still holds its old address. Once that address contains a new live
> > > struct file, KASAN sees valid, unpoisoned memory and cannot distinguish
> > > the stale object identity. CONFIG_DEBUG_SPINLOCK exposes the failure
> > > instead.
> > >
> > > The failing interleaving is:
> > >
> > > CPU0: nested EPOLL_CTL_ADD             CPU1: close/open churn
> > > ------------------------------------   ---------------------------------
> > > p = hlist_first_rcu(&head->epitems)
> > > epi = container_of(p, ...)
> > >                                        close(victim)
> > >                                          __fput()
> > >                                            eventpoll_release_file()
> > >                                            file_free(victim)
> > >                                        // the slot is free; f_lock remains
> > > spin_lock(&epi->ffd.file->f_lock)
> > >                                        open() reuses the slot as new_file
> > >                                          spin_lock_init(&new_file->f_lock)
> > > spin_unlock(&epi->ffd.file->f_lock)     // wild unlock of new_file's lock
> > >
> > > CONFIG_DEBUG_SPINLOCK reports:
> > >
> > > BUG: spinlock already unlocked on CPU#0, poc_unlist/150
> >
> >
> > Well I'm no expert on epoll, but on cursory reading I think this is
> > fixable in a simpler manner and without the extra overhead.
> >
> > If you were to add a constructor to the files slab, you could move the
> > spinlock and mutex initialization into it. Then unlock can safely
> > proceed and maybe that's good enough (as in there are no other issues
> > stemming from the typesafe thing)?
>
> Thanks for the suggestion.
>
> I also looked at other struct file users while investigating this. A
> constructor would make this particular stale lock/unlock safe, but it is
> not a general solution to stale file identity.
>
> For example, commit a6dc643c6931 ("eventpoll: fix ep_remove struct
> eventpoll / struct file UAF") used file->f_ep and is_file_epoll(file).
> Immediate reuse could select the wrong free path and cause an invalid
> free. This case happens to use the stale file pointer only for f_lock.
>
> I therefore preferred pinning the file, which keeps its identity stable,
> limits the change to eventpoll, and is straightforward to backport.

The difference compared to other typesafe by rcu usage is that
destruction is synchronized with a spin lock.

In principle, the code with the racing unlock should be able to grab
the lock (like it does now) and verify whether the file got reused, in
which case it should merely unlock and bail as it has nothing to do.
Keeping the spin lock safe to acquire at all times is definitely a
missing ingredient, there might be more.

Can you share the reproducer?