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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230903005334.5356-1-hdanton@sina.com>
Date: Sun,  3 Sep 2023 08:53:34 +0800
From: Hillf Danton <hdanton@...a.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Netdev <netdev@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Naresh Kamboju <naresh.kamboju@...aro.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: selftests: net: pmtu.sh: Unable to handle kernel paging request at virtual address

On Thu, 31 Aug 2023 15:12:30 +0200 Eric Dumazet <edumazet@...gle.com>
> On Thu, Aug 31, 2023 at 2:17=E2=80=AFPM Hillf Danton <hdanton@...a.com>
> > On Wed, 30 Aug 2023 21:44:57 +0900 Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> > >On 2023/08/30 20:26, Hillf Danton wrote:
> > >>> <4>[  399.014716] Call trace:
> > >>> <4>[  399.015702]  percpu_counter_add_batch+0x28/0xd0
> > >>> <4>[  399.016399]  dst_destroy+0x44/0x1e4
> > >>> <4>[  399.016681]  dst_destroy_rcu+0x14/0x20
> > >>> <4>[  399.017009]  rcu_core+0x2d0/0x5e0
> > >>> <4>[  399.017311]  rcu_core_si+0x10/0x1c
> > >>> <4>[  399.017609]  __do_softirq+0xd4/0x23c
> > >>> <4>[  399.017991]  ____do_softirq+0x10/0x1c
> > >>> <4>[  399.018320]  call_on_irq_stack+0x24/0x4c
> > >>> <4>[  399.018723]  do_softirq_own_stack+0x1c/0x28
> > >>> <4>[  399.022639]  __irq_exit_rcu+0x6c/0xcc
> > >>> <4>[  399.023434]  irq_exit_rcu+0x10/0x1c
> > >>> <4>[  399.023962]  el1_interrupt+0x8c/0xc0
> > >>> <4>[  399.024810]  el1h_64_irq_handler+0x18/0x24
> > >>> <4>[  399.025324]  el1h_64_irq+0x64/0x68
> > >>> <4>[  399.025612]  _raw_spin_lock_bh+0x0/0x6c
> > >>> <4>[  399.026102]  cleanup_net+0x280/0x45c
> > >>> <4>[  399.026403]  process_one_work+0x1d4/0x310
> > >>> <4>[  399.027140]  worker_thread+0x248/0x470
> > >>> <4>[  399.027621]  kthread+0xfc/0x184
> > >>> <4>[  399.028068]  ret_from_fork+0x10/0x20
> > >>
> > >> static void cleanup_net(struct work_struct *work)
> > >> {
> > >>      ...
> > >>
> > >>      synchronize_rcu();
> > >>
> > >>      /* Run all of the network namespace exit methods */
> > >>      list_for_each_entry_reverse(ops, &pernet_list, list)
> > >>              ops_exit_list(ops, &net_exit_list);
> > >>      ...
> > >>
> > >> Why did the RCU sync above fail to work in this report, Eric?
> > >
> > > Why do you assume that synchronize_rcu() failed to work?
> >
> > In the ipv6 pernet_operations [1] for instance, dst_entries_destroy() is
> > invoked after RCU sync to ensure that nobody is using the exiting net,
> > but this report shows that protection falls apart.
> 
> Because synchronize_rcu() is not the same than rcu_barrier()
> 
> The dst_entries_add()/ percpu_counter_add_batch() call should not
> happen after an rcu grace period.

	cpu2			cpu3
	====			====
	cleanup_net()		rcu_read_lock();
				it is safe to use either netns or dst
				rcu_read_unlock();
	synchronize_rcu();
				unsafe to access anyone now
> 
> Something like this (untested) patch
> 
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 980e2fd2f013b3e50cc47ed0666ee5f24f50444b..f02fdd1da6066a4d56c2a0aa8038eca76d62f8bd
> 100644
> --- 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 |=3D DST_NOCOUNT;
> +                       dst_entries_add(dst->ops, -1);

Could this add happen after the rcu sync above?

> +               }
>                 call_rcu_hurry(&dst->rcu_head, dst_destroy_rcu);
> +       }
>  }
>  EXPORT_SYMBOL(dst_release);