| 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
| ||
|
Message-ID: <PH0PR11MB588088F6212E9291D3E69244DA8E9@PH0PR11MB5880.namprd11.prod.outlook.com>
Date: Thu, 30 Mar 2023 15:20:15 +0000
From: "Zhang, Qiang1" <qiang1.zhang@...el.com>
To: "Zhang, Qiang1" <qiang1.zhang@...el.com>,
"paulmck@...nel.org" <paulmck@...nel.org>
CC: "rcu@...r.kernel.org" <rcu@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel-team@...a.com" <kernel-team@...a.com>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"hch@....de" <hch@....de>,
"qiang.zhang1211@...il.com" <qiang.zhang1211@...il.com>
Subject: RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically
allocated in-module srcu_struct
> >Further shrinking the srcu_struct structure is eased by requiring
> >that in-module srcu_struct structures rely more heavily on static
> >initialization. In particular, this preserves the property that a
> >module-load-time srcu_struct initialization can fail only due to
> >memory-allocation failure of the per-CPU srcu_data structures.
> >It might also slightly improve robustness by keeping the number of
> >memory allocations that must succeed down percpu_alloc() call.
> >
> >This is in preparation for splitting an srcu_usage structure out of
> >the srcu_struct structure.
> >
> >[ paulmck: Fold in qiang1.zhang@...el.com feedback. ]
> >
> >Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
> >Cc: Christoph Hellwig <hch@....de>
> >---
> > include/linux/srcutree.h | 19 ++++++++++++++-----
> > kernel/rcu/srcutree.c | 19 +++++++++++++------
> > 2 files changed, 27 insertions(+), 11 deletions(-)
> >
> >diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> >index ac8af12f93b3..428480152375 100644
> >--- a/include/linux/srcutree.h
> >+++ b/include/linux/srcutree.h
> >@@ -121,15 +121,24 @@ struct srcu_struct {
> > #define SRCU_STATE_SCAN1 1
> > #define SRCU_STATE_SCAN2 2
> >
> >-#define __SRCU_STRUCT_INIT(name, pcpu_name) \
> >-{ \
> >- .sda = &pcpu_name, \
> >+#define __SRCU_STRUCT_INIT_COMMON(name) \
> > .lock = __SPIN_LOCK_UNLOCKED(name.lock), \
> > .srcu_gp_seq_needed = -1UL, \
> > .work = __DELAYED_WORK_INITIALIZER(name.work, NULL, 0), \
> >- __SRCU_DEP_MAP_INIT(name) \
> >+ __SRCU_DEP_MAP_INIT(name)
> >+
> >+#define __SRCU_STRUCT_INIT_MODULE(name) \
> >+{ \
> >+ __SRCU_STRUCT_INIT_COMMON(name) \
> > }
> >
> >+#define __SRCU_STRUCT_INIT(name, pcpu_name) \
> >+{ \
> >+ .sda = &pcpu_name, \
> >+ __SRCU_STRUCT_INIT_COMMON(name) \
> >+}
> >+
> >+
> > /*
> > * Define and initialize a srcu struct at build time.
> > * Do -not- call init_srcu_struct() nor cleanup_srcu_struct() on it.
> >@@ -151,7 +160,7 @@ struct srcu_struct {
> > */
> > #ifdef MODULE
> > # define __DEFINE_SRCU(name, is_static) \
> >- is_static struct srcu_struct name; \
> >+ is_static struct srcu_struct name = __SRCU_STRUCT_INIT_MODULE(name); \
> > extern struct srcu_struct * const __srcu_struct_##name; \
> > struct srcu_struct * const __srcu_struct_##name \
> > __section("___srcu_struct_ptrs") = &name diff --git
> >a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index
> >cd46fe063e50..7a6d9452a5d0 100644
> >--- a/kernel/rcu/srcutree.c
> >+++ b/kernel/rcu/srcutree.c
> >@@ -1895,13 +1895,14 @@ void __init srcu_init(void) static int
> >srcu_module_coming(struct module *mod) {
> > int i;
> >+ struct srcu_struct *ssp;
> > struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> >- int ret;
> >
> > for (i = 0; i < mod->num_srcu_structs; i++) {
> >- ret = init_srcu_struct(*(sspp++));
> >- if (WARN_ON_ONCE(ret))
> >- return ret;
> >+ ssp = *(sspp++);
> >+ ssp->sda = alloc_percpu(struct srcu_data);
> >+ if (WARN_ON_ONCE(!ssp->sda))
> >+ return -ENOMEM;
> > }
> > return 0;
> > }
> >@@ -1910,10 +1911,16 @@ static int srcu_module_coming(struct module
> >*mod) static void srcu_module_going(struct module *mod) {
> > int i;
> >+ struct srcu_struct *ssp;
> > struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> >
> >- for (i = 0; i < mod->num_srcu_structs; i++)
> >- cleanup_srcu_struct(*(sspp++));
> >+ for (i = 0; i < mod->num_srcu_structs; i++) {
> >+ ssp = *(sspp++);
> >+ if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) &&
> >+ !WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
> >+ cleanup_srcu_struct(ssp);
> >+ free_percpu(ssp->sda);
>
>
> Hi Paul
>
> About 037b80b8865fb ("srcu: Check for readers at module-exit time ")
>
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1911,7 +1911,8 @@ static void srcu_module_going(struct module *mod)
> if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) &&
> !WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
> cleanup_srcu_struct(ssp);
The srcu_sup->sda_is_static is true, in cleanup_srcu_struct(), the ssp->sda can not be freed.
> - free_percpu(ssp->sda);
> + else if (!WARN_ON(srcu_readers_active(ssp)))
> + free_percpu(ssp->sda);
>
> Should the else statement be removed? like this:
>
> if (!WARN_ON(srcu_readers_active(ssp)))
> free_percpu(ssp->sda);
>
>Mightn't that cause us to double-free ssp->sda? Once in free_percpu(),
>and before that in cleanup_srcu_struct()?
how about this? any thought?
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1937,7 +1937,7 @@ static void srcu_module_going(struct module *mod)
if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) &&
!WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
cleanup_srcu_struct(ssp);
- else if (!WARN_ON(srcu_readers_active(ssp)))
+ if (!WARN_ON(srcu_readers_active(ssp)))
free_percpu(ssp->sda);
}
}
Thanks
Zqiang
>
> Thanx, Paul
>
> Thanks
> Zqiang
>
>
> >+ }
> > }
> >
> > /* Handle one module, either coming or going. */
> >--
> >2.40.0.rc2
> >
Powered by blists - more mailing lists