Menu
▾
▴
Re: [tipc-discussion] [PATCH net-next v4 02/10] tipc: make bearer variable in media structure synchronous
|
From: Ying X. <yin...@wi...> - 2013-10-29 02:23:11
|
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);
>> }
>>
>> /**
>
>
|
