linux-kernel - Re: [RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

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]

Date:	Mon, 22 Mar 2010 11:33:41 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
CC:	akpm@...ux-foundation.org, Ingo Molnar <mingo@...e.hu>,
	linux-kernel@...r.kernel.org, paulmck@...ux.vnet.ibm.com,
	dipankar@...ibm.com, josh@...htriplett.org, dvhltc@...ibm.com,
	niv@...ibm.com, tglx@...utronix.de, peterz@...radead.org,
	rostedt@...dmis.org, Valdis.Kletnieks@...edu, dhowells@...hat.com,
	eric.dumazet@...il.com, adobriyan@...il.com
Subject: Re: [RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

Very nice you do not touch struct rcu_head.

Mathieu Desnoyers wrote:

> +
> +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> +# define STATE_RCU_HEAD_READY	0
> +# define STATE_RCU_HEAD_QUEUED	1
> +
> +extern struct debug_obj_descr rcuhead_debug_descr;
> +
> +static inline void debug_rcu_head_queue(struct rcu_head *head)
> +{
> +	debug_object_activate(head, &rcuhead_debug_descr);
> +	debug_object_active_state(head, &rcuhead_debug_descr,
> +				  STATE_RCU_HEAD_READY,
> +				  STATE_RCU_HEAD_QUEUED);
> +}
> +
> +static inline void debug_rcu_head_unqueue(struct rcu_head *head)
> +{
> +	debug_object_active_state(head, &rcuhead_debug_descr,
> +				  STATE_RCU_HEAD_QUEUED,
> +				  STATE_RCU_HEAD_READY);
> +	debug_object_deactivate(head, &rcuhead_debug_descr);
> +}

I'm a little foolish. I don't know why we need
STATE_RCU_HEAD_READY/QUEUED.

1) obj->astate does not be set to STATE_RCU_HEAD_READY in
debug_object_activate/init().

Maybe you need to add a function:
debug_object_activate_with_astate(head, descr, init_astate);
(the same as debug_object_activate(), but init obj->astate also)

Or you need:
@debugobjects.h
# define DEBUG_OBJECT_ASTATE_INIT 0
@rcupdate.c
# define STATE_RCU_HEAD_READY	DEBUG_OBJECT_ASTATE_INIT
# define STATE_RCU_HEAD_QUEUED	(STATE_RCU_HEAD_READY + 1)

2) In debug_rcu_head_queue(), debug_object_active_state()
   is always success when debug_object_activate() is success.

   In debug_rcu_head_unqueue(), debug_object_active_state()
   is always success if RCU subsystem is running correctly.

(Correct me if I'm wrong) So we don't need debug_object_active_state()
here if we just find racy users of call_rcu().

> + */
> +static int rcuhead_fixup_init(void *addr, enum debug_obj_state state)
> +{
> +	struct rcu_head *head = addr;
> +
> +	switch (state) {
> +	case ODEBUG_STATE_ACTIVE:
> +		/*
> +		 * Ensure that queued callbacks are all executed.
> +		 * If we detect that we are nested in a RCU read-side critical
> +		 * section, we should simply fail, otherwise we would deadlock.
> +		 */
> +		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
> +		    irqs_disabled()) {
> +			WARN_ON(1);
> +			return 0;
> +		}

preempt_count() != 0 ??? What happen when !CONFIG_PREEMPT?

> +		rcu_barrier();

We may need call rcu_barrier(), rcu_barrier_sched(), rcu_barrier_bh() together?
rcu_barrier() does not ensure to flush callbacks of RCU_BH, RCU_SCHED.

> +		debug_object_init(head, &rcuhead_debug_descr);
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/