Re: [tipc-discussion] [PATCH net-next v2 02/10] tipc: make bearer variable in media structure synchronous

[Ying X. <yin...@wi...>]

On 08/08/2013 04:52 PM, Erik Hugne wrote:
> On Thu, Aug 08, 2013 at 04:26:16PM +0800, Ying Xue wrote:
>> When TIPC media is enabled by bearer, the "bearer" variable in the
>> media instance is first initialized with the bearer pointer under
>> tipc_net_lock protection; likewise, when media is disabled,
>> tipc_net_lock is also first held and its "bearer" variable is then
>> set to NULL. But, when the "bearer" variable is accessed in
>> recv_msg() and recv_notification(), tipc_net_lock is not taken. As
>> the context under which recv_msg() and recv_notification() run is
>> different with media enable/disable functions, a stale "bearer"
>> might be got by recv_msg() and recv_notification() occasionally.
>> Therefore, before the bearer variable is read, tipc_net_lock should
>> be held to get rid of the synchronous issue.
>>
>> Signed-off-by: Ying Xue <yin...@wi...>
>> ---
>>  net/tipc/bearer.c    |    2 --
>>  net/tipc/eth_media.c |   13 ++++++++++++-
>>  net/tipc/ib_media.c  |   13 ++++++++++++-
>>  net/tipc/link.c      |    2 --
>>  4 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
>> index 25045ba..c71da45 100644
>> --- a/net/tipc/bearer.c
>> +++ b/net/tipc/bearer.c
>> @@ -420,7 +420,6 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
>>  	struct tipc_link *l_ptr;
>>  	struct tipc_link *temp_l_ptr;
>>  
>> -	read_lock_bh(&tipc_net_lock);
>>  	pr_info("Blocking bearer <%s>\n", b_ptr->name);
>>  	atomic_set(&b_ptr->blocked, 1);
>>  	spin_lock_bh(&b_ptr->lock);
>> @@ -432,7 +431,6 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
>>  		spin_unlock_bh(&n_ptr->lock);
>>  	}
>>  	spin_unlock_bh(&b_ptr->lock);
>> -	read_unlock_bh(&tipc_net_lock);
>>  	return 0;
>>  }
> 
> I'm missing a motivation in the commit message why it's safe do bearer blocking 
> outside of tipc_net_lock protection.

Please notice we now place tipc_net_lock(read) lock into
recv_notification(), and recv_notification() is the only user of
tipc_block_bearer(). So in tipc_block_bearer(), tipc_net_lock is not
useful, but it's still protected by tipc_net_lock.

> By the way, shouldn't this have been done in the last patch from the 
> "non controversial" patchset?

This decision should be made by Jon right now :)

Regards,
Ying

> tipc: convert 'blocked' flag in struct tipc_bearer to atomic_t
> 
> //E
> 
>