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

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

On 08/09/2013 11:56 AM, Jon Maloy wrote:
> 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?

First of all, the question is that tipc_block_bearer() currently only
has one user - recv_notification(); secondly, in recv_notification(), we
have to hold net_lock because ib_ptr->bearer/eb_ptr->bearer must be
protected by net_lock too. So net_lock should be removed from
tipc_block_bearer().

In fact, if you carefully take a look at the patch again, you will find
another changes in recv_notification() like:

@@ -254,8 +258,12 @@ static int recv_notification(struct notifier_block
*nb, unsigned long evt,

[...]
	read_lock_bh(&tipc_net_lock);
        case NETDEV_UNREGISTER:
        case NETDEV_CHANGENAME:
+               read_unlock_bh(&tipc_net_lock);
                tipc_disable_bearer(eb_ptr->bearer->name);
+               read_lock_bh(&tipc_net_lock);
                break;
        }
+       read_unlock_bh(&tipc_net_lock);
        return NOTIFY_OK;
 }

Above changes are safe for us because tipc_disable_bearer() will find
b_ptr handler with name. But tipc_block_bearer() now directly access
b_ptr, it's unsafe if we do the same thing like tipc_disable_bearer().

However, holding net_lock read lock will disappear in
recv_notification() if we involve RCU lock.

> 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?
> 

b_ptr->name is never changed, and blocked variable is converted to
atomic_t. Actually not only b_ptr->name is without the scope of bearer
lock protection in tipc_block_bearer(), but also in other places we also
don't hold bearer lock when accessing bearer name.

Regards,
Ying

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