Re: [B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Correct rcu refcounting for gw_node

Sven Eckelmann
2011-02-03T17:56:37Z

[-- Attachment #1: Type: Text/Plain, Size: 2306 bytes --]

> For another the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node
> need to be used.

What? Since when is curr_gw::orig_node rcu protected?

> - return curr_gateway_tmp->orig_node;
> + orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
> + if (orig_node) {
> + rcu_read_unlock();
> + return NULL;
> + }
> +
> + rcu_read_unlock();
> + return orig_node;
> }

And you cannot return the (it is not, but when it would be) rcu protected
element without increasing the refcounter.

> + orig_node = rcu_dereference(gw_node->orig_node);
> + if (!orig_node->router)
> continue;

Same here

> - if (bat_priv->curr_gw != curr_gw_tmp) {
> - if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> + if (curr_gw != curr_gw_tmp) {
> + orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> + if ((curr_gw) && (!curr_gw_tmp))

And here

> - if (bat_priv->curr_gw != curr_gw_tmp) {
> - if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> + if (curr_gw != curr_gw_tmp) {
> + orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> + if ((curr_gw) && (!curr_gw_tmp))

Here again

> - if (bat_priv->curr_gw != curr_gw_tmp) {
> - if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> + if (curr_gw != curr_gw_tmp) {
> + orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> + if ((curr_gw) && (!curr_gw_tmp))

Here....

> - gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
> + rcu_read_lock();
> + curr_gw = rcu_dereference(bat_priv->curr_gw);
> + orig_node = rcu_dereference(gw_node->orig_node);
> + gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);

....

> diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
> index 6a9ab61..8816102 100644
> --- a/batman-adv/unicast.c
> +++ b/batman-adv/unicast.c
> @@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct
> bat_priv *bat_priv) if (!orig_node)
> goto trans_search;
>
> - kref_get(&orig_node->refcount);
> goto find_router;
> } else {
> rcu_read_lock();

Why? Maybe this should be part of the patch 3/3?

And please to merge the refcounting stuff with the rcu pointer in one patch...
and maybe it is better to merge patch 2/3 with the rcu pointer patch.

Best regards,
Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]