linux-stable.git/net/tipc/link.c, branch linux-4.1.y

tipc: fix problem with parallel link synchronization mechanism

2015-04-29T19:08:59+00:00

Currently, we try to accumulate arrived packets in the links's
'deferred' queue during the parallel link syncronization phase.

This entails two problems:

- With an unlucky combination of arriving packets the algorithm
  may go into a lockstep with the out-of-sequence handling function,
  where the synch mechanism is adding a packet to the deferred queue,
  while the out-of-sequence handling is retrieving it again, thus
  ending up in a loop inside the node_lock scope.

- Even if this is avoided, the link will very often send out
  unnecessary protocol messages, in the worst case leading to
  redundant retransmissions.

We fix this by just dropping arriving packets on the upcoming link
during the synchronization phase, thus relying on the retransmission
protocol to resolve the situation once the two links have arrived to
a synchronized state.

Reviewed-by: Erik Hugne 
Reviewed-by: Ying Xue 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller

tipc: remove wrong use of NLM_F_MULTI

2015-04-29T18:59:17+00:00

NLM_F_MULTI must be used only when a NLMSG_DONE message is sent. In fact,
it is sent only at the end of a dump.

Libraries like libnl will wait forever for NLMSG_DONE.

Fixes: 35b9dd7607f0 ("tipc: add bearer get/dump to new netlink api")
Fixes: 7be57fc69184 ("tipc: add link get/dump to new netlink api")
Fixes: 46f15c6794fb ("tipc: add media get/dump to new netlink api")
CC: Richard Alpe 
CC: Jon Maloy 
CC: Ying Xue 
CC: tipc-discussion@lists.sourceforge.net
Signed-off-by: Nicolas Dichtel 
Signed-off-by: David S. Miller

tipc: fix node refcount issue

2015-04-23T15:50:34+00:00

When link statistics is dumped over netlink, we iterate over
the list of peer nodes and append each links statistics to
the netlink msg. In the case where the dump is resumed after
filling up a nlmsg, the node refcnt is decremented without
having been incremented previously which may cause the node
reference to be freed. When this happens, the following
info/stacktrace will be generated, followed by a crash or
undefined behavior.
We fix this by removing the erroneous call to tipc_node_put
inside the loop that iterates over nodes.

[  384.312303] INFO: trying to register non-static key.
[  384.313110] the code is fine but needs lockdep annotation.
[  384.313290] turning off the locking correctness validator.
[  384.313290] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.0.0+ #13
[  384.313290] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  384.313290]  ffff88003c6d0290 ffff88003cc03ca8 ffffffff8170adf1 0000000000000007
[  384.313290]  ffffffff82728730 ffff88003cc03d38 ffffffff810a6a6d 00000000001d7200
[  384.313290]  ffff88003c6d0ab0 ffff88003cc03ce8 0000000000000285 0000000000000001
[  384.313290] Call Trace:
[  384.313290]    [] dump_stack+0x4c/0x65
[  384.313290]  [] __lock_acquire+0xf3d/0xf50
[  384.313290]  [] lock_acquire+0xd5/0x290
[  384.313290]  [] ? link_timeout+0x1c/0x170 [tipc]
[  384.313290]  [] ? link_state_event+0x4e0/0x4e0 [tipc]
[  384.313290]  [] _raw_spin_lock_bh+0x40/0x80
[  384.313290]  [] ? link_timeout+0x1c/0x170 [tipc]
[  384.313290]  [] link_timeout+0x1c/0x170 [tipc]
[  384.313290]  [] call_timer_fn+0xb8/0x490
[  384.313290]  [] ? process_timeout+0x10/0x10
[  384.313290]  [] run_timer_softirq+0x21c/0x420
[  384.313290]  [] ? link_state_event+0x4e0/0x4e0 [tipc]
[  384.313290]  [] __do_softirq+0xf4/0x630
[  384.313290]  [] irq_exit+0x5d/0x60
[  384.313290]  [] smp_apic_timer_interrupt+0x41/0x50
[  384.313290]  [] apic_timer_interrupt+0x70/0x80
[  384.313290]    [] ? default_idle+0x20/0x210
[  384.313290]  [] ? default_idle+0x1e/0x210
[  384.313290]  [] arch_cpu_idle+0xa/0x10
[  384.313290]  [] cpu_startup_entry+0x2c3/0x530
[  384.313290]  [] ? clockevents_register_device+0x113/0x200
[  384.313290]  [] start_secondary+0x13f/0x170

Fixes: 8a0f6ebe8494 ("tipc: involve reference counter for node structure")
Signed-off-by: Erik Hugne 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller

tipc: simplify link mtu negotiation

2015-04-02T20:27:12+00:00

When a link is being established, the two endpoints advertise their
respective interface MTU in the transmitted RESET and ACTIVATE messages.
If there is any difference, the lower of the two MTUs will be selected
for use by both endpoints.

However, as a remnant of earlier attempts to introduce TIPC level
routing. there also exists an MTU discovery mechanism. If an intermediate
node has a lower MTU than the two endpoints, they will discover this
through a bisectional approach, and finally adopt this MTU for common use.

Since there is no TIPC level routing, and probably never will be,
this mechanism doesn't make any sense, and only serves to make the
link level protocol unecessarily complex.

In this commit, we eliminate the MTU discovery algorithm,and fall back
to the simple MTU advertising approach. This change is fully backwards
compatible.

Reviewed-by: Ying Xue 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller

tipc: eliminate delayed link deletion at link failover

2015-04-02T20:27:12+00:00

When a bearer is disabled manually, all its links have to be reset
and deleted. However, if there is a remaining, parallel link ready
to take over a deleted link's traffic, we currently delay the delete
of the removed link until the failover procedure is finished. This
is because the remaining link needs to access state from the reset
link, such as the last received packet number, and any partially
reassembled buffer, in order to perform a successful failover.

In this commit, we do instead move the state data over to the new
link, so that it can fulfill the procedure autonomously, without
accessing any data on the old link. This means that we can now
proceed and delete all pertaining links immediately when a bearer
is disabled. This saves us from some unnecessary complexity in such
situations.

We also choose to change the confusing definitions CHANGEOVER_PROTOCOL,
ORIGINAL_MSG and DUPLICATE_MSG to the more descriptive TUNNEL_PROTOCOL,
FAILOVER_MSG and SYNCH_MSG respectively.

Reviewed-by: Ying Xue 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller

tipc: drop tunneled packet duplicates at reception

2015-04-02T20:27:12+00:00

In commit 8b4ed8634f8b3f9aacfc42b4a872d30c36b9e255
("tipc: eliminate race condition at dual link establishment")
we introduced a parallel link synchronization mechanism that
guarentees sequential delivery even for users switching from
an old to a newly established link. The new mechanism makes it
unnecessary to deliver the tunneled duplicate packets back to
the old link, as we are currently doing. It is now sufficient
to use the last tunneled packet's inner sequence number as
synchronization point between the two parallel links, whereafter
it can be dropped.

In this commit, we drop the duplicate packets arriving on the new
link, after updating the synchronization point at each new arrival.

Although it would now have been sufficient for the other endpoint
to only tunnel the last packet in its send queue, and not the
entire queue, we must still do this to maintain compatibility
with older nodes.

This commit makes it possible to get rid if some complex
interaction between the two parallel links.

Reviewed-by: Ying Xue 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller

tipc: involve reference counter for node structure

2015-03-29T19:40:28+00:00

TIPC node hash node table is protected with rcu lock on read side.
tipc_node_find() is used to look for a node object with node address
through iterating the hash node table. As the entire process of what
tipc_node_find() traverses the table is guarded with rcu read lock,
it's safe for us. However, when callers use the node object returned
by tipc_node_find(), there is no rcu read lock applied. Therefore,
this is absolutely unsafe for callers of tipc_node_find().

Now we introduce a reference counter for node structure. Before
tipc_node_find() returns node object to its caller, it first increases
the reference counter. Accordingly, after its caller used it up,
it decreases the counter again. This can prevent a node being used by
one thread from being freed by another thread.

Reviewed-by: Erik Hugne 
Reviewed-by: Jon Maloy 
Signed-off-by: Ying Xue 
Signed-off-by: David S. Miller

tipc: fix potential deadlock when all links are reset

2015-03-29T19:40:27+00:00

[   60.988363] ======================================================
[   60.988754] [ INFO: possible circular locking dependency detected ]
[   60.989152] 3.19.0+ #194 Not tainted
[   60.989377] -------------------------------------------------------
[   60.989781] swapper/3/0 is trying to acquire lock:
[   60.990079]  (&(&n_ptr->lock)->rlock){+.-...}, at: [] tipc_link_retransmit+0x1aa/0x240 [tipc]
[   60.990743]
[   60.990743] but task is already holding lock:
[   60.991106]  (&(&bclink->lock)->rlock){+.-...}, at: [] tipc_bclink_lock+0x8e/0xa0 [tipc]
[   60.991738]
[   60.991738] which lock already depends on the new lock.
[   60.991738]
[   60.992174]
[   60.992174] the existing dependency chain (in reverse order) is:
[   60.992174]
-> #1 (&(&bclink->lock)->rlock){+.-...}:
[   60.992174]        [] lock_acquire+0x9c/0x140
[   60.992174]        [] _raw_spin_lock_bh+0x3f/0x50
[   60.992174]        [] tipc_bclink_lock+0x8e/0xa0 [tipc]
[   60.992174]        [] tipc_bclink_add_node+0x97/0xf0 [tipc]
[   60.992174]        [] tipc_node_link_up+0xf5/0x110 [tipc]
[   60.992174]        [] link_state_event+0x2b3/0x4f0 [tipc]
[   60.992174]        [] tipc_link_proto_rcv+0x24c/0x418 [tipc]
[   60.992174]        [] tipc_rcv+0x827/0xac0 [tipc]
[   60.992174]        [] tipc_l2_rcv_msg+0x73/0xd0 [tipc]
[   60.992174]        [] __netif_receive_skb_core+0x746/0x980
[   60.992174]        [] __netif_receive_skb+0x21/0x70
[   60.992174]        [] netif_receive_skb_internal+0x35/0x130
[   60.992174]        [] napi_gro_receive+0x158/0x1d0
[   60.992174]        [] e1000_clean_rx_irq+0x155/0x490
[   60.992174]        [] e1000_clean+0x267/0x990
[   60.992174]        [] net_rx_action+0x150/0x360
[   60.992174]        [] __do_softirq+0x123/0x360
[   60.992174]        [] irq_exit+0x8e/0xb0
[   60.992174]        [] do_IRQ+0x65/0x110
[   60.992174]        [] ret_from_intr+0x0/0x13
[   60.992174]        [] arch_cpu_idle+0xf/0x20
[   60.992174]        [] cpu_startup_entry+0x2f6/0x3f0
[   60.992174]        [] start_secondary+0x13a/0x150
[   60.992174]
-> #0 (&(&n_ptr->lock)->rlock){+.-...}:
[   60.992174]        [] __lock_acquire+0x163d/0x1ca0
[   60.992174]        [] lock_acquire+0x9c/0x140
[   60.992174]        [] _raw_spin_lock_bh+0x3f/0x50
[   60.992174]        [] tipc_link_retransmit+0x1aa/0x240 [tipc]
[   60.992174]        [] tipc_bclink_rcv+0x611/0x640 [tipc]
[   60.992174]        [] tipc_rcv+0x616/0xac0 [tipc]
[   60.992174]        [] tipc_l2_rcv_msg+0x73/0xd0 [tipc]
[   60.992174]        [] __netif_receive_skb_core+0x746/0x980
[   60.992174]        [] __netif_receive_skb+0x21/0x70
[   60.992174]        [] netif_receive_skb_internal+0x35/0x130
[   60.992174]        [] napi_gro_receive+0x158/0x1d0
[   60.992174]        [] e1000_clean_rx_irq+0x155/0x490
[   60.992174]        [] e1000_clean+0x267/0x990
[   60.992174]        [] net_rx_action+0x150/0x360
[   60.992174]        [] __do_softirq+0x123/0x360
[   60.992174]        [] irq_exit+0x8e/0xb0
[   60.992174]        [] do_IRQ+0x65/0x110
[   60.992174]        [] ret_from_intr+0x0/0x13
[   60.992174]        [] arch_cpu_idle+0xf/0x20
[   60.992174]        [] cpu_startup_entry+0x2f6/0x3f0
[   60.992174]        [] start_secondary+0x13a/0x150
[   60.992174]
[   60.992174] other info that might help us debug this:
[   60.992174]
[   60.992174]  Possible unsafe locking scenario:
[   60.992174]
[   60.992174]        CPU0                    CPU1
[   60.992174]        ----                    ----
[   60.992174]   lock(&(&bclink->lock)->rlock);
[   60.992174]                                lock(&(&n_ptr->lock)->rlock);
[   60.992174]                                lock(&(&bclink->lock)->rlock);
[   60.992174]   lock(&(&n_ptr->lock)->rlock);
[   60.992174]
[   60.992174]  *** DEADLOCK ***
[   60.992174]
[   60.992174] 3 locks held by swapper/3/0:
[   60.992174]  #0:  (rcu_read_lock){......}, at: [] __netif_receive_skb_core+0x71/0x980
[   60.992174]  #1:  (rcu_read_lock){......}, at: [] tipc_l2_rcv_msg+0x5/0xd0 [tipc]
[   60.992174]  #2:  (&(&bclink->lock)->rlock){+.-...}, at: [] tipc_bclink_lock+0x8e/0xa0 [tipc]
[   60.992174]

The correct the sequence of grabbing n_ptr->lock and bclink->lock
should be that the former is first held and the latter is then taken,
which exactly happened on CPU1. But especially when the retransmission
of broadcast link is failed, bclink->lock is first held in
tipc_bclink_rcv(), and n_ptr->lock is taken in link_retransmit_failure()
called by tipc_link_retransmit() subsequently, which is demonstrated on
CPU0. As a result, deadlock occurs.

If the order of holding the two locks happening on CPU0 is reversed, the
deadlock risk will be relieved. Therefore, the node lock taken in
link_retransmit_failure() originally is moved to tipc_bclink_rcv()
so that it's obtained before bclink lock. But the precondition of
the adjustment of node lock is that responding to bclink reset event
must be moved from tipc_bclink_unlock() to tipc_node_unlock().

Reviewed-by: Erik Hugne 
Signed-off-by: Ying Xue 
Signed-off-by: David S. Miller

tipc: eliminate race condition at dual link establishment

2015-03-25T18:05:56+00:00

Despite recent improvements, the establishment of dual parallel
links still has a small glitch where messages can bypass each
other. When the second link in a dual-link configuration is
established, part of the first link's traffic will be steered over
to the new link. Although we do have a mechanism to ensure that
packets sent before and after the establishment of the new link
arrive in sequence to the destination node, this is not enough.
The arriving messages will still be delivered upwards in different
threads, something entailing a risk of message disordering during
the transition phase.

To fix this, we introduce a synchronization mechanism between the
two parallel links, so that traffic arriving on the new link cannot
be added to its input queue until we are guaranteed that all
pre-establishment messages have been delivered on the old, parallel
link.

This problem seems to always have been around, but its occurrence is
so rare that it has not been noticed until recent intensive testing.

Reviewed-by: Ying Xue 
Reviewed-by: Erik Hugne 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller

tipc: clean up handling of link congestion

2015-03-25T18:05:56+00:00

After the recent changes in message importance handling it becomes
possible to simplify handling of messages and sockets when we
encounter link congestion.

We merge the function tipc_link_cong() into link_schedule_user(),
and simplify the code of the latter. The code should now be
easier to follow, especially regarding return codes and handling
of the message that caused the situation.

In case the scheduling function is unable to pre-allocate a wakeup
message buffer, it now returns -ENOBUFS, which is a more correct
code than the previously used -EHOSTUNREACH.

Reviewed-by: Ying Xue 
Reviewed-by: Erik Hugne 
Signed-off-by: Jon Maloy 
Signed-off-by: David S. Miller