On 07/08/2016 07:17 AM, Xue, Ying wrote:
>
> -----Original Message-----
> From: Jon Maloy [mailto:jon.ma...@ericsson.com]
> Sent: Friday, July 08, 2016 4:21 AM
> To: tipc-discussion@lists.sourceforge.net;
> parthasarathy.bhuvara...@ericsson.com; Xue, Ying; richard.a...@ericsson.com;
> jon.ma...@ericsson.com
> Cc: ma...@donjonn.com
> Subject: [PATCH net-next v2 1/2] tipc: make bearer packet filtering generic
>
> In commit 5b7066c3dd24 ("tipc: stricter filtering of packets in bearer
> layer") we introduced a method of filtering out messages while a bearer is
> being reset, to avoid that links may be re-created and come back in working
> state while we are still in the process of shutting them down.
>
> This solution works well, but is limited to only work with L2 media, which is
> insufficient with the increasing use of UDP as carrier media.
>
> We now replace this solution with a more generic one, by introducing a new
> boolean field "blocked" in the generic struct tipc_bearer. This field will be
> set and reset at the same locations as with the previous solution, while the
> packet filtering is moved to the generic code for the sending side. On the
> receiving side, the filtering is still done in media specific code, but now
> including the UDP bearer.
>
> Signed-off-by: Jon Maloy <jon.ma...@ericsson.com>
> ---
> net/tipc/bearer.c | 77
> ++++++++++++++++++++++++++--------------------------
> net/tipc/bearer.h | 1 +
> net/tipc/udp_media.c | 2 +-
> 3 files changed, 41 insertions(+), 39 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index e7244d6..6d4c2b3
> 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -56,6 +56,13 @@ static struct tipc_media * const media_info_array[] = {
> NULL
> };
>
> +static struct tipc_bearer *bearer_get(struct net *net, int bearer_id) {
> + struct tipc_net *tn = tipc_net(net);
> +
> + return rcu_dereference_rtnl(tn->bearer_list[bearer_id]);
> +}
> +
> static void bearer_disable(struct net *net, struct tipc_bearer *b);
>
> /**
> @@ -302,6 +309,7 @@ restart:
> b->domain = disc_domain;
> b->net_plane = bearer_id + 'A';
> b->priority = priority;
> + b->up = true;
>
> res = tipc_disc_create(net, b, &b->bcast_addr, &skb);
> if (res) {
> @@ -339,16 +347,25 @@ static int tipc_reset_bearer(struct net *net, struct
> tipc_bearer *b)
> */
> void tipc_bearer_reset_all(struct net *net) {
> - struct tipc_net *tn = tipc_net(net);
> struct tipc_bearer *b;
> int i;
>
> rcu_read_lock();
> for (i = 0; i < MAX_BEARERS; i++) {
> - b = rcu_dereference_rtnl(tn->bearer_list[i]);
> + b = bearer_get(net, i);
> + if (b)
> + b->up = false;
> + }
> + for (i = 0; i < MAX_BEARERS; i++) {
> + b = bearer_get(net, i);
> if (b)
> tipc_reset_bearer(net, b);
> }
> + for (i = 0; i < MAX_BEARERS; i++) {
> + b = bearer_get(net, i);
> + if (b)
> + b->up = true;
> + }
>
> If we are under rcu_read lock, it means that we are staying on read side.
> Regarding RCU locking semantic, we cannot modify RCU variable on read side.
If we stick strictly to the letter, you are right. But what would be the
consequences? As I see it, it would be no different than if we change a
value in the read side of a wr-lock: some contexts will see the wrong
value for a while, and eventually, when the cache is updated, they will
all get the correct value. The consequences of obtaining a stale value
in this particular case are minimal: there will be no crash or
malfunction, only some contexts will let through packets that they
shouldn't, and vice versa, and only for a short period of time. Anyway,
we would be much better off than we are now, when this happens on a
permanent base.
///jon
> As you know, bearer instance is projected by RTNL lock. If we want to change
> it, we must hold it on write side. However, as we must reset links on
> reception path, it means that it's inappropriate to hold RTNL on the context.
> I cannot figure more better approach except that we perform this in another
> process context asynchronously like workqueue. But this is really not what we
> want to do.
>
> Regards,
> Ying
>
> rcu_read_unlock();
> }
>
> @@ -363,8 +380,9 @@ static void bearer_disable(struct net *net, struct
> tipc_bearer *b)
> int bearer_id = b->identity;
>
> pr_info("Disabling bearer <%s>\n", b->name);
> - b->media->disable_media(b);
> + b->up = false;
> tipc_node_delete_links(net, bearer_id);
> + b->media->disable_media(b);
> RCU_INIT_POINTER(b->media_ptr, NULL);
> if (b->link_req)
> tipc_disc_delete(b->link_req);
> @@ -421,22 +439,16 @@ int tipc_l2_send_msg(struct net *net, struct sk_buff
> *skb, {
> struct net_device *dev;
> int delta;
> - void *tipc_ptr;
>
> dev = (struct net_device *)rcu_dereference_rtnl(b->media_ptr);
> if (!dev)
> return 0;
>
> - /* Send RESET message even if bearer is detached from device */
> - tipc_ptr = rcu_dereference_rtnl(dev->tipc_ptr);
> - if (unlikely(!tipc_ptr && !msg_is_reset(buf_msg(skb))))
> - goto drop;
> -
> - delta = dev->hard_header_len - skb_headroom(skb);
> - if ((delta > 0) &&
> - pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC))
> - goto drop;
> -
> + delta = SKB_DATA_ALIGN(dev->hard_header_len - skb_headroom(skb));
> + if ((delta > 0) && pskb_expand_head(skb, delta, 0, GFP_ATOMIC)) {
> + kfree_skb(skb);
> + return 0;
> + }
> skb_reset_network_header(skb);
> skb->dev = dev;
> skb->protocol = htons(ETH_P_TIPC);
> @@ -444,9 +456,6 @@ int tipc_l2_send_msg(struct net *net, struct sk_buff *skb,
> dev->dev_addr, skb->len);
> dev_queue_xmit(skb);
> return 0;
> -drop:
> - kfree_skb(skb);
> - return 0;
> }
>
> int tipc_bearer_mtu(struct net *net, u32 bearer_id) @@ -468,12 +477,12 @@
> void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
> struct sk_buff *skb,
> struct tipc_media_addr *dest)
> {
> - struct tipc_net *tn = tipc_net(net);
> + struct tipc_msg *hdr = buf_msg(skb);
> struct tipc_bearer *b;
>
> rcu_read_lock();
> - b = rcu_dereference_rtnl(tn->bearer_list[bearer_id]);
> - if (likely(b))
> + b = bearer_get(net, bearer_id);
> + if (likely(b && (b->up || msg_is_reset(hdr))))
> b->media->send_msg(net, skb, b, dest);
> else
> kfree_skb(skb);
> @@ -486,7 +495,6 @@ void tipc_bearer_xmit(struct net *net, u32 bearer_id,
> struct sk_buff_head *xmitq,
> struct tipc_media_addr *dst)
> {
> - struct tipc_net *tn = net_generic(net, tipc_net_id);
> struct tipc_bearer *b;
> struct sk_buff *skb, *tmp;
>
> @@ -494,12 +502,15 @@ void tipc_bearer_xmit(struct net *net, u32 bearer_id,
> return;
>
> rcu_read_lock();
> - b = rcu_dereference_rtnl(tn->bearer_list[bearer_id]);
> + b = bearer_get(net, bearer_id);
> if (unlikely(!b))
> __skb_queue_purge(xmitq);
> skb_queue_walk_safe(xmitq, skb, tmp) {
> __skb_dequeue(xmitq);
> - b->media->send_msg(net, skb, b, dst);
> + if (likely(b->up || msg_is_reset(buf_msg(skb))))
> + b->media->send_msg(net, skb, b, dst);
> + else
> + kfree(skb);
> }
> rcu_read_unlock();
> }
> @@ -516,8 +527,8 @@ void tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
> struct tipc_msg *hdr;
>
> rcu_read_lock();
> - b = rcu_dereference_rtnl(tn->bearer_list[bearer_id]);
> - if (unlikely(!b))
> + b = bearer_get(net, bearer_id);
> + if (unlikely(!b || !b->up))
> __skb_queue_purge(xmitq);
> skb_queue_walk_safe(xmitq, skb, tmp) {
> hdr = buf_msg(skb);
> @@ -547,7 +558,7 @@ static int tipc_l2_rcv_msg(struct sk_buff *skb, struct
> net_device *dev,
>
> rcu_read_lock();
> b = rcu_dereference_rtnl(dev->tipc_ptr);
> - if (likely(b && (skb->pkt_type <= PACKET_BROADCAST))) {
> + if (likely(b && b->up && (skb->pkt_type <= PACKET_BROADCAST))) {
> skb->next = NULL;
> tipc_rcv(dev_net(dev), skb, b);
> rcu_read_unlock();
> @@ -572,18 +583,9 @@ static int tipc_l2_device_event(struct notifier_block
> *nb, unsigned long evt, {
> struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> struct net *net = dev_net(dev);
> - struct tipc_net *tn = tipc_net(net);
> struct tipc_bearer *b;
> - int i;
>
> b = rtnl_dereference(dev->tipc_ptr);
> - if (!b) {
> - for (i = 0; i < MAX_BEARERS; b = NULL, i++) {
> - b = rtnl_dereference(tn->bearer_list[i]);
> - if (b && (b->media_ptr == dev))
> - break;
> - }
> - }
> if (!b)
> return NOTIFY_DONE;
>
> @@ -594,11 +596,10 @@ static int tipc_l2_device_event(struct notifier_block
> *nb, unsigned long evt,
> if (netif_carrier_ok(dev))
> break;
> case NETDEV_UP:
> - rcu_assign_pointer(dev->tipc_ptr, b);
> + b->up = true;
> break;
> case NETDEV_GOING_DOWN:
> - RCU_INIT_POINTER(dev->tipc_ptr, NULL);
> - synchronize_net();
> + b->up = false;
> tipc_reset_bearer(net, b);
> break;
> case NETDEV_CHANGEMTU:
> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h index f1e6db5..6f51ed8
> 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -150,6 +150,7 @@ struct tipc_bearer {
> u32 identity;
> struct tipc_link_req *link_req;
> char net_plane;
> + bool up;
> };
>
> struct tipc_bearer_names {
> diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index
> b016c01..c7f52bf 100644
> --- a/net/tipc/udp_media.c
> +++ b/net/tipc/udp_media.c
> @@ -224,7 +224,7 @@ static int tipc_udp_recv(struct sock *sk, struct sk_buff
> *skb)
> rcu_read_lock();
> b = rcu_dereference_rtnl(ub->bearer);
>
> - if (b) {
> + if (b && b->up) {
> tipc_rcv(sock_net(sk), skb, b);
> rcu_read_unlock();
> return 0;
> --
> 1.9.1
>
------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
