usage of rcu_dereference_raw

usage of rcu_dereference_raw

[Pranith Kumar]

Hi Paul,

I was trying to see the various uses of rcu_dereference_ functions to
understand how they were being used. A doubt cropped up while doing this:

* rcu_dereference_raw(): the documentation explicitly mentions that this
  should be minimally used as this does no checking of read critical sections
  and does not implement barriers. But looking at the code, there are various
  places where this is being used and it is being used I think in a buggy
  way. For example, in drivers/net/wireless/iwlwifi/dvm/main.c, there is this:

        kfree(rcu_dereference_raw(priv->noa_data));

  I can't imagine a scenario in which this is valid. So my question is this:
  do most of the uses of rcu_dereference_raw() need to be changed to use other
  dereference functions or are there cases where its usage is valid?

Regards,
-- 
Pranith

Re: usage of rcu_dereference_raw

[Paul E. McKenney]

On Mon, Apr 21, 2014 at 05:37:01PM -0400, Pranith Kumar wrote:
> Hi Paul,
> 
> I was trying to see the various uses of rcu_dereference_ functions to
> understand how they were being used. A doubt cropped up while doing this:
> 
> * rcu_dereference_raw(): the documentation explicitly mentions that this
>   should be minimally used as this does no checking of read critical sections
>   and does not implement barriers. But looking at the code, there are various
>   places where this is being used and it is being used I think in a buggy
>   way. For example, in drivers/net/wireless/iwlwifi/dvm/main.c, there is this:
> 
>         kfree(rcu_dereference_raw(priv->noa_data));
> 
>   I can't imagine a scenario in which this is valid. So my question is this:
>   do most of the uses of rcu_dereference_raw() need to be changed to use other
>   dereference functions or are there cases where its usage is valid?

Well, the first call to it from iwl_op_mode_dvm_start() is valid because
that field has never been exposed to readers, so that no other task or
CPU has access to this field.

The second call from iwl_op_mode_dvm_stop() -might- be valid.  For it
to be valid, there must be a grace period between the time that the
field was made inaccessible to readers and the time that iwl_uninit_drv()
was called.  Usually something like synchronize_rcu() waits for the
needed grace period.

Please let me know what you find!

							Thanx, Paul

Re: usage of rcu_dereference_raw

[Pranith Kumar]

On Mon, Apr 21, 2014 at 6:03 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>>   do most of the uses of rcu_dereference_raw() need to be changed to use other
>>   dereference functions or are there cases where its usage is valid?
>
> The second call from iwl_op_mode_dvm_stop() -might- be valid.  For it
> to be valid, there must be a grace period between the time that the
> field was made inaccessible to readers and the time that iwl_uninit_drv()
> was called.  Usually something like synchronize_rcu() waits for the
> needed grace period.

So there are valid use cases of the rcu_dereference_raw() in scenarios where
we can verify that a grace period has passed.

Thank you for the info. Mind adding it as a comment as in the patch below?

add comment for rcu_dereference_raw

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/linux/rcupdate.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 00a7fd6..af40a86 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -662,7 +662,13 @@ static inline void rcu_preempt_sleep_check(void)
     __rcu_dereference_check((p), rcu_read_lock_sched_held() || (c), \
                 __rcu)
 
-#define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
+/* rcu_dereference_raw() - rcu_dereference with no checking
+ * @p: The pointer to read, prior to dereferencing
+ *
+ * Use this to dereference a rcu pointer if you are sure that there exists a
+ * grace period between the time this pointer was made inaccessible to readers
+ */
+#define rcu_dereference_raw(p) rcu_dereference_check(p, 1)
 
 /*
  * The tracing infrastructure traces RCU (we want that), but unfortunately
-- 
1.7.9.5


-- 
Pranith

Re: usage of rcu_dereference_raw

[Paul E. McKenney]

On Mon, Apr 21, 2014 at 06:47:34PM -0400, Pranith Kumar wrote:
> 
> On Mon, Apr 21, 2014 at 6:03 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> >>   do most of the uses of rcu_dereference_raw() need to be changed to use other
> >>   dereference functions or are there cases where its usage is valid?
> >
> > The second call from iwl_op_mode_dvm_stop() -might- be valid.  For it
> > to be valid, there must be a grace period between the time that the
> > field was made inaccessible to readers and the time that iwl_uninit_drv()
> > was called.  Usually something like synchronize_rcu() waits for the
> > needed grace period.
> 
> So there are valid use cases of the rcu_dereference_raw() in scenarios where
> we can verify that a grace period has passed.
> 
> Thank you for the info. Mind adding it as a comment as in the patch below?
> 
> add comment for rcu_dereference_raw
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>

If we are going to do that, we should probably have a complete list:

1.	The pointer has not yet been made available to readers,
	for example, the access happens during initialization.

2.	A grace period has elapsed since the last time that this
	pointer was available to readers, for example, this access
	happens during clean-up processing within an RCU callback
	function.

3.	Code that is shared among multiple flavors of RCU cannot
	say which flavor of RCU to specify.  Given SRCU, there might
	be an unbounded number to choose from.  This is the reason
	for use of rcu_dereference_raw() in list_for_each_entry_rcu()
	and similar RCU list-traversal macros.

4.	Code that uses a combination of RCU and per-data-structure
	locking, but where a given lock guards not just the data
	structure containing it, but a number of subordinate linked
	structures as well.  Code operating on one of the subordinate
	structures might not know which lock is protecting it.

There probably are others, but that is what comes to mind at the
moment.  Ah, and we should ask that uses of rcu_dereference_raw()
be commented.

							Thanx, Paul

> ---
>  include/linux/rcupdate.h |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 00a7fd6..af40a86 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -662,7 +662,13 @@ static inline void rcu_preempt_sleep_check(void)
>      __rcu_dereference_check((p), rcu_read_lock_sched_held() || (c), \
>                  __rcu)
> 
> -#define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
> +/* rcu_dereference_raw() - rcu_dereference with no checking
> + * @p: The pointer to read, prior to dereferencing
> + *
> + * Use this to dereference a rcu pointer if you are sure that there exists a
> + * grace period between the time this pointer was made inaccessible to readers
> + */
> +#define rcu_dereference_raw(p) rcu_dereference_check(p, 1)
> 
>  /*
>   * The tracing infrastructure traces RCU (we want that), but unfortunately
> -- 
> 1.7.9.5
> 
> 
> -- 
> Pranith
>