Re: selftests: net: pmtu.sh: Unable to handle kernel paging request at virtual address

[Eric Dumazet]

On Sun, Oct 6, 2024 at 8:58 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Sun, Oct 6, 2024 at 8:08 PM Xin Long <lucien.xin@xxxxxxxxx> wrote:
> >
> > Sorry for bringing up this issue, it recently occurred on my aarch64 kernel
> > with blackhole_netdev backported. I tracked it down, and when deleting
> > the netns, the path is:
> >
> > In cleanup_net():
> >
> >   default_device_exit_batch()
> >     unregister_netdevice_many()
> >       addrconf_ifdown() -> call_rcu(rcu, fib6_info_destroy_rcu) <--- [1]
> >     netdev_run_todo()
> >       rcu_barrier() <- [2]
> >   ip6_route_net_exit() -> dst_entries_destroy(net->ip6_dst_ops) <--- [3]
> >
> > In fib6_info_destroy_rcu():
> >
> >   dst_dev_put()
> >   dst_release() -> call_rcu(rcu, dst_destroy_rcu) <--- [5]
> >
> > In dst_destroy_rcu():
> >   dst_destroy() -> dst_entries_add(dst->ops, -1); <--- [6]
> >
> > fib6_info_destroy_rcu() is scheduled at [1], rcu_barrier() will wait
> > for fib6_info_destroy_rcu() to be done at [2]. However, another callback
> > dst_destroy_rcu() is scheduled() in fib6_info_destroy_rcu() at [5], and
> > there's no place calling rcu_barrier() to wait for dst_destroy_rcu() to
> > be done. It means dst_entries_add() at [6] might be run later than
> > dst_entries_destroy() at [3], then this UAF will trigger the panic.
> >
> > On Tue, Oct 17, 2023 at 1:02 PM Naresh Kamboju
> > <naresh.kamboju@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 5 Sept 2023 at 17:55, Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Sep 5, 2023 at 1:52 PM Hillf Danton <hdanton@xxxxxxxx> wrote:
> > > > >
> > > > > On Mon, 4 Sep 2023 13:29:57 +0200 Eric Dumazet <edumazet@xxxxxxxxxx>
> > > > > > On Sun, Sep 3, 2023 at 5:57=E2=80=AFAM Hillf Danton <hdanton@xxxxxxxx>
> > > > > > > On Thu, 31 Aug 2023 15:12:30 +0200 Eric Dumazet <edumazet@xxxxxxxxxx>
> > > > > > > > --- a/net/core/dst.c
> > > > > > > > +++ b/net/core/dst.c
> > > > > > > > @@ -163,8 +163,13 @@ EXPORT_SYMBOL(dst_dev_put);
> > > > > > > >
> > > > > > > >  void dst_release(struct dst_entry *dst)
> > > > > > > >  {
> > > > > > > > -       if (dst && rcuref_put(&dst->__rcuref))
> > > > > > > > +       if (dst && rcuref_put(&dst->__rcuref)) {
> > > > > > > > +               if (!(dst->flags & DST_NOCOUNT)) {
> > > > > > > > +                       dst->flags |= DST_NOCOUNT;
> > > > > > > > +                       dst_entries_add(dst->ops, -1);
> > > > > > >
> > So I think it makes sense to NOT call dst_entries_add() in the path
> > dst_destroy_rcu() -> dst_destroy(), as it does on the patch above,
> > but I don't see it get posted.
> >
> > Hi, Eric, would you like to move forward with your patch above ?
> >
> > Or we can also move the dst_entries_add(dst->ops, -1) from dst_destroy()
> > to dst_release():
> >
> > Note, dst_destroy() is not used outside net/core/dst.c, we may delete
> > EXPORT_SYMBOL(dst_destroy) in the future.
> >
> >
>
> Current kernel has known issue with dst_cache, triggering quite often
> with  selftests: net: pmtu.sh
>
> (Although for some reason it does no longer trigger 'often' any more
> in my vng tests)

Simple hack/patch to 'disable' dst_cache, if you want to confirm the
issue is there.


diff --git a/net/core/dst_cache.c b/net/core/dst_cache.c
index 70c634b9e7b02300188582a1634d5977838db132..53351ff58b35dbee37ff587f7ef8f72580d9e116
100644
--- a/net/core/dst_cache.c
+++ b/net/core/dst_cache.c
@@ -142,12 +142,7 @@ EXPORT_SYMBOL_GPL(dst_cache_get_ip6);

 int dst_cache_init(struct dst_cache *dst_cache, gfp_t gfp)
 {
-       dst_cache->cache = alloc_percpu_gfp(struct dst_cache_pcpu,
-                                           gfp | __GFP_ZERO);
-       if (!dst_cache->cache)
-               return -ENOMEM;
-
-       dst_cache_reset(dst_cache);
+       dst_cache->cache = NULL;
        return 0;
 }
 EXPORT_SYMBOL_GPL(dst_cache_init);