Menu
▾
▴
Re: [tipc-discussion] [PATCH net-next v4 02/10] tipc: make bearer variable in media structure synchronous
|
From: Jon M. <ma...@do...> - 2013-10-27 21:57:17
|
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. 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);
> }
>
> /**
|
