Re: [B.A.T.M.A.N.] Use of ref_get_unless_zero() ?

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2644 bytes --]

On Friday 04 March 2016 16:21:32 Andrew Lunn wrote:
> Hi
> 
> I'm sometimes getting a crash after removing a hard interface when the
> batadv_send_outstanding_bat_org_packet() is called in a work queue.
> It calls
> 
> static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
>                                         int packet_len, unsigned long
> send_time, bool direct_link,
>                                         struct batadv_hard_iface
> *if_incoming, struct batadv_hard_iface *if_outgoing, int own_packet)
> {
>         struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
> struct batadv_forw_packet *forw_packet_aggr;
>         unsigned char *skb_buff;
>         unsigned int skb_size;
> 
>         if (!kref_get_unless_zero(&if_incoming->refcount))
>                 return;
> 
>         if (!kref_get_unless_zero(&if_outgoing->refcount))
>                 goto out_free_incoming;
> 
> 
> Given that we have:
> 
> static inline void batadv_hardif_put(struct batadv_hard_iface *hard_iface)
> {
>         kref_put(&hard_iface->refcount, batadv_hardif_release);
> }
> 
> does using kref_get_unless_zero() make sense? If it is zero, hasn't it
> been freed by the kref_put that set it to zero?

At least it makes sense for the outgoing interface because it is only in a 
rcu_read_lock in batadv_iv_ogm_schedule (batadv_iv_ogm_queue_add -> 
batadv_iv_ogm_aggregate_new). The batadv_hardif_list is traversed with 
list_for_each_entry_rcu and it is expected that one entry (maybe) gets dropped 
from the list. The batadv_hardif_release will only queue the actual free of 
the memory (kfree_rcu) and every function which wants to get a reference has 
to increase the counter with kref_get_unless_zero to check that it is not 
actually in the waiting-to-be-freed-phase.

But you have something which needs to be fixed (you see a crash). Question is 
what is causing the crash and what can be done against it. I am currently 
wondering how the if_incoming interface is being protected. It is not fetched 
from a list via a rcu list access primitive and it is not protected via 
rcu_read_lock. I can also not see where the reference for the forw_packet-
>if_incoming is increased. It is just accessed in 
batadv_send_outstanding_bat_ogm_packet (and later send to the mentioned 
function via batadv_schedule_bat_ogm). Also batadv_add_bcast_packet_to_list 
doesn't increase the reference counter for if_incoming before adding to the 
forward packet. So I would just say that the reference counting for 
batadv_hard_iface is broken.


Kind regards,
	Sven

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