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

[Jon M. <ma...@do...>]

On 08/08/2013 05:07 AM, Ying Xue wrote:
> 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 :)
Ok. Decision made: Let's keep them separate ;). Otherwise I will have to
improve the motivation for the introduction of the atomic_t.

I am even questioning the need for this change.
This means that all future callers of tipc_block_bearer() will have to take
net_lock. So why don't we keep it where it is, in one place?
I also notice that we access b_ptr just two lines before we grab the
bearer lock, in the pr_info() call. We know that this is safe, but there
will be questions. Why don't we move this inside the lock?

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