| 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: <20200915013404.GD2579423@google.com>
Date: Mon, 14 Sep 2020 21:34:04 -0400
From: Joel Fernandes <joel@...lfernandes.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: nikolay@...ulusnetworks.com, davem@...emloft.net,
netdev@...r.kernel.org, paulmck@...nel.org, josh@...htriplett.org,
peterz@...radead.org, christian.brauner@...ntu.com,
rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
sfr@...b.auug.org.au, roopa@...dia.com
Subject: Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing
the condition
On Mon, Sep 14, 2020 at 03:47:38PM -0700, Jakub Kicinski wrote:
> On Mon, 14 Sep 2020 16:21:22 -0400 Joel Fernandes wrote:
> > On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
> > > On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@...ulusnetworks.com wrote:
> > > > Ah, you want to solve it for all. :)
> > > > Looks and sounds good to me,
> > > > Reviewed-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> > >
> > > Actually, I give up, lockdep_is_held() is not defined without
> > > CONFIG_LOCKDEP, let's just go with your patch..
> >
> > Care to send a patch just for the RCU macro then? Not sure what Dave is
> > applying but if the net-next tree is not taking the RCU macro change, then
> > send another one with my tag:
>
> Seems like quite a few places depend on the macro disappearing its
> argument. I was concerned that it's going to be had to pick out whether
> !LOCKDEP builds should return true or false from LOCKDEP helpers, but
> perhaps relying on the linker errors even more is not such poor taste?
>
> Does the patch below look acceptable to you?
>
> --->8------------
>
> rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition
>
> We run into a unused variable warning in bridge code when
> variable is only used inside the condition of
> rcu_dereference_protected().
>
> #define mlock_dereference(X, br) \
> rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
>
> Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
> compiles to nothing the compiler doesn't see the variable use.
>
> Prevent the warning by adding the condition as dead code.
> We need to un-hide the declaration of lockdep_tasklist_lock_is_held(),
> lockdep_sock_is_held(), RCU lock maps and remove some declarations
> in net/sched header, because they have a wrong type.
>
> Add forward declarations of lockdep_is_held(), lock_is_held() which
> will cause a linker errors if actually used with !LOCKDEP.
> At least RCU expects some locks _not_ to be held so it's hard to
> pick true/false for a dummy implementation.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> include/linux/lockdep.h | 6 ++++++
> include/linux/rcupdate.h | 11 ++++++-----
> include/linux/rcupdate_trace.h | 4 ++--
> include/linux/sched/task.h | 2 --
> include/net/sch_generic.h | 12 ------------
> include/net/sock.h | 2 --
Would it make sense to split it into individual patches?
So 1 for rcu, 1 for lockdep and then 1 for networking. The lockdep ones may
need PeterZ's ack.
thanks,
- Joel
> 6 files changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 6a584b3e5c74..c4b6225ee320 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -371,6 +371,12 @@ static inline void lockdep_unregister_key(struct lock_class_key *key)
>
> #define lockdep_depth(tsk) (0)
>
> +/*
> + * Dummy forward declarations, allow users to write less ifdef-y code
> + * and depend on dead code elimination.
> + */
> +int lock_is_held(const void *);
> +int lockdep_is_held(const void *);
> #define lockdep_is_held_type(l, r) (1)
>
> #define lockdep_assert_held(l) do { (void)(l); } while (0)
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index d15d46db61f7..50d45781fa99 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -234,6 +234,11 @@ bool rcu_lockdep_current_cpu_online(void);
> static inline bool rcu_lockdep_current_cpu_online(void) { return true; }
> #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
>
> +extern struct lockdep_map rcu_lock_map;
> +extern struct lockdep_map rcu_bh_lock_map;
> +extern struct lockdep_map rcu_sched_lock_map;
> +extern struct lockdep_map rcu_callback_map;
> +
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>
> static inline void rcu_lock_acquire(struct lockdep_map *map)
> @@ -246,10 +251,6 @@ static inline void rcu_lock_release(struct lockdep_map *map)
> lock_release(map, _THIS_IP_);
> }
>
> -extern struct lockdep_map rcu_lock_map;
> -extern struct lockdep_map rcu_bh_lock_map;
> -extern struct lockdep_map rcu_sched_lock_map;
> -extern struct lockdep_map rcu_callback_map;
> int debug_lockdep_rcu_enabled(void);
> int rcu_read_lock_held(void);
> int rcu_read_lock_bh_held(void);
> @@ -320,7 +321,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>
> #else /* #ifdef CONFIG_PROVE_RCU */
>
> -#define RCU_LOCKDEP_WARN(c, s) do { } while (0)
> +#define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c))
> #define rcu_sleep_check() do { } while (0)
>
> #endif /* #else #ifdef CONFIG_PROVE_RCU */
> diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
> index aaaac8ac927c..25cdef506cae 100644
> --- a/include/linux/rcupdate_trace.h
> +++ b/include/linux/rcupdate_trace.h
> @@ -11,10 +11,10 @@
> #include <linux/sched.h>
> #include <linux/rcupdate.h>
>
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> -
> extern struct lockdep_map rcu_trace_lock_map;
>
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +
> static inline int rcu_read_lock_trace_held(void)
> {
> return lock_is_held(&rcu_trace_lock_map);
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> index a98965007eef..9f943c391df9 100644
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -47,9 +47,7 @@ extern spinlock_t mmlist_lock;
> extern union thread_union init_thread_union;
> extern struct task_struct init_task;
>
> -#ifdef CONFIG_PROVE_RCU
> extern int lockdep_tasklist_lock_is_held(void);
> -#endif /* #ifdef CONFIG_PROVE_RCU */
>
> extern asmlinkage void schedule_tail(struct task_struct *prev);
> extern void init_idle(struct task_struct *idle, int cpu);
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index d60e7c39d60c..1aaa9e3d2e9c 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -432,7 +432,6 @@ struct tcf_block {
> struct mutex proto_destroy_lock; /* Lock for proto_destroy hashtable. */
> };
>
> -#ifdef CONFIG_PROVE_LOCKING
> static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
> {
> return lockdep_is_held(&chain->filter_chain_lock);
> @@ -442,17 +441,6 @@ static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
> {
> return lockdep_is_held(&tp->lock);
> }
> -#else
> -static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
> -{
> - return true;
> -}
> -
> -static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
> -{
> - return true;
> -}
> -#endif /* #ifdef CONFIG_PROVE_LOCKING */
>
> #define tcf_chain_dereference(p, chain) \
> rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
> diff --git a/include/net/sock.h b/include/net/sock.h
> index eaa5cac5e836..1c67b1297a72 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1566,13 +1566,11 @@ do { \
> lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0); \
> } while (0)
>
> -#ifdef CONFIG_LOCKDEP
> static inline bool lockdep_sock_is_held(const struct sock *sk)
> {
> return lockdep_is_held(&sk->sk_lock) ||
> lockdep_is_held(&sk->sk_lock.slock);
> }
> -#endif
>
> void lock_sock_nested(struct sock *sk, int subclass);
>
> --
> 2.24.1
>
Powered by blists - more mailing lists