Thread: Re: [tipc-discussion] [PATCH net-next v4 02/10] tipc: make bearer variable in media structure synch (Page 3)

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

On 10/28/2013 05:57 AM, Jon Maloy wrote:
> On 10/25/2013 06:40 AM, 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.
> Ying,
> I may have misunderstood something here, but to me it looks like this
> patch is obsoleted by patch #8 in this series, where you
> aggregate in the bearer struct in eth_media.

No, patch #8 doesn't obsolete the patch.

Although the bearer pointer protected by tipc_net_lock in the patch
appears in patch #8, tipc_net_lock is still useful for us in recv_msg()
because tipc_net_lock is used to protect media_hlist. Please see point
*(1) in below piece of code:

static inline struct hlist_head *media_hashfn(int ifindex)
{
        return &media_hlist[ifindex & (NETDEV_HASHENTRIES - 1)];
}

static struct eth_media *media_get(int ifindex)
{
        struct eth_media *eb_ptr;
        struct hlist_head *head = media_hashfn(ifindex);

        hlist_for_each_entry(eb_ptr, head, hlist)
                if (eb_ptr->dev->ifindex == ifindex)
                        return eb_ptr;
        return NULL;
}

static int recv_msg(struct sk_buff *buf, struct net_device *dev,
                    struct packet_type *pt, struct net_device *orig_dev)
{
        struct eth_media *eb_ptr;

        if (!net_eq(dev_net(dev), &init_net)) {
                kfree_skb(buf);
                return NET_RX_DROP;
        }

        read_lock_bh(&tipc_net_lock);
        eb_ptr = media_get(dev->ifindex);<-------------point *(1)
        if (likely(eb_ptr)) {
                if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
                        buf->next = NULL;
                        tipc_recv_msg(buf, (struct tipc_bearer *)eb_ptr);
                        read_unlock_bh(&tipc_net_lock);
                        return NET_RX_SUCCESS;
                }
        }
        read_unlock_bh(&tipc_net_lock);
        kfree_skb(buf);
        return NET_RX_DROP;
}

After RCU lock is involved, tipc_net_lock read lock will be replaced
with rcu_read_lock() to protect media_hlist on read side.

Threfore, now expanding the scope of protection of tipc_net_lock is an
appropriate time.

Regards,
Ying

 I wouldn't mind
> submitting this anyway, since tipc_net_lock is going away eventually,
> but I suspect  David M and others may make the same observation
> and ask the same question.
> 
> ///jon
> 
>>
>> 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 17f9824..3bbd90b 100644
>> --- a/net/tipc/bearer.c
>> +++ b/net/tipc/bearer.c
>> @@ -404,7 +404,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_add_unless(&b_ptr->blocked, 1, 1);
>>  	spin_lock_bh(&b_ptr->lock);
>> @@ -416,7 +415,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;
>>  }
>>  
>> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
>> index 32d01e8..ffba0c5 100644
>> --- a/net/tipc/eth_media.c
>> +++ b/net/tipc/eth_media.c
>> @@ -36,6 +36,7 @@
>>  
>>  #include "core.h"
>>  #include "bearer.h"
>> +#include "net.h"
>>  
>>  #define MAX_ETH_MEDIA		MAX_BEARERS
>>  
>> @@ -135,13 +136,16 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
>>  		return NET_RX_DROP;
>>  	}
>>  
>> +	read_lock_bh(&tipc_net_lock);
>>  	if (likely(eb_ptr->bearer)) {
>>  		if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
>>  			buf->next = NULL;
>>  			tipc_recv_msg(buf, eb_ptr->bearer);
>> +			read_unlock_bh(&tipc_net_lock);
>>  			return NET_RX_SUCCESS;
>>  		}
>>  	}
>> +	read_unlock_bh(&tipc_net_lock);
>>  	kfree_skb(buf);
>>  	return NET_RX_DROP;
>>  }
>> @@ -254,8 +258,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>>  		if (++eb_ptr == stop)
>>  			return NOTIFY_DONE;	/* couldn't find device */
>>  	}
>> -	if (!eb_ptr->bearer)
>> +
>> +	read_lock_bh(&tipc_net_lock);
>> +	if (!eb_ptr->bearer) {
>> +		read_unlock_bh(&tipc_net_lock);
>>  		return NOTIFY_DONE;		/* bearer had been disabled */
>> +	}
>>  
>>  	eb_ptr->bearer->mtu = dev->mtu;
>>  
>> @@ -279,9 +287,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>>  		break;
>>  	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;
>>  }
>>  
>> diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
>> index 5241a5b..8ed7358 100644
>> --- a/net/tipc/ib_media.c
>> +++ b/net/tipc/ib_media.c
>> @@ -41,6 +41,7 @@
>>  #include <linux/if_infiniband.h>
>>  #include "core.h"
>>  #include "bearer.h"
>> +#include "net.h"
>>  
>>  #define MAX_IB_MEDIA		MAX_BEARERS
>>  
>> @@ -128,13 +129,16 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
>>  		return NET_RX_DROP;
>>  	}
>>  
>> +	read_lock_bh(&tipc_net_lock);
>>  	if (likely(ib_ptr->bearer)) {
>>  		if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
>>  			buf->next = NULL;
>>  			tipc_recv_msg(buf, ib_ptr->bearer);
>> +			read_unlock_bh(&tipc_net_lock);
>>  			return NET_RX_SUCCESS;
>>  		}
>>  	}
>> +	read_unlock_bh(&tipc_net_lock);
>>  	kfree_skb(buf);
>>  	return NET_RX_DROP;
>>  }
>> @@ -247,8 +251,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>>  		if (++ib_ptr == stop)
>>  			return NOTIFY_DONE;	/* couldn't find device */
>>  	}
>> -	if (!ib_ptr->bearer)
>> +
>> +	read_lock_bh(&tipc_net_lock);
>> +	if (!ib_ptr->bearer) {
>> +		read_unlock_bh(&tipc_net_lock);
>>  		return NOTIFY_DONE;		/* bearer had been disabled */
>> +	}
>>  
>>  	ib_ptr->bearer->mtu = dev->mtu;
>>  
>> @@ -272,9 +280,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>>  		break;
>>  	case NETDEV_UNREGISTER:
>>  	case NETDEV_CHANGENAME:
>> +		read_unlock_bh(&tipc_net_lock);
>>  		tipc_disable_bearer(ib_ptr->bearer->name);
>> +		read_lock_bh(&tipc_net_lock);
>>  		break;
>>  	}
>> +	read_unlock_bh(&tipc_net_lock);
>>  	return NOTIFY_OK;
>>  }
>>  
>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>> index 0e7d50a..12c72d2 100644
>> --- a/net/tipc/link.c
>> +++ b/net/tipc/link.c
>> @@ -1491,7 +1491,6 @@ static int link_recv_buf_validate(struct sk_buff *buf)
>>   */
>>  void tipc_recv_msg(struct sk_buff *head, struct tipc_bearer *b_ptr)
>>  {
>> -	read_lock_bh(&tipc_net_lock);
>>  	while (head) {
>>  		struct tipc_node *n_ptr;
>>  		struct tipc_link *l_ptr;
>> @@ -1687,7 +1686,6 @@ deliver:
>>  cont:
>>  		kfree_skb(buf);
>>  	}
>> -	read_unlock_bh(&tipc_net_lock);
>>  }
>>  
>>  /**
> 
> 

Thread: Re: [tipc-discussion] [PATCH net-next v4 02/10] tipc: make bearer variable in media structure synch (Page 3)

Cluster wide IPC providing datagram, connection, and bus messaging

tipc-discussion