On 12/06/2017 09:03 PM, Jon Maloy wrote:
>
>
>> -----Original Message-----
>> From: Ying Xue [mailto:[email protected]]
>> Sent: Wednesday, December 06, 2017 03:29
>> To: Jon Maloy <[email protected]>; [email protected]
>> Cc: [email protected]
>> Subject: Re: [net-next PATCH 1/2] tipc: prevent swork item from queuing
>> workqueue after connection is closed
>>
>>
>>
>> On 12/06/2017 03:31 AM, Jon Maloy wrote:
>>>> If you in remove the connection item from the idr list *before* you
>>>> flush the queues and remove the socket, there is no risk that this
>>>> will happen.
>>>> If tipc_conn_sendmsg() obtains a pointer, we can be certain that the
>>>> queue has not been flushed yet, and that this will not happen until
>>>> conn_put() is called.
>>>> And, you won't need to use the callback lock or touch the socket at all
>> here.
>>> It's even better: if you introduce the !kref_get_unless_zero(&con->kref))
>> test, there is no way tipc_conn_sendmsg() will obtain a pointer while
>> tipc_conn_kref_release() is being executed, i.e., after the queue is flushed.
>>> I suggest you make both these changes.
>>
>>
>> Please let me use below call chains to show what the race condition is:
>>
>> CPU 1: CPU 2:
>> ========== ===============
>> T1: tipc_conn_sendmsg
>> T2: con = tipc_conn_lookup()
>> T3: test_bit(CF_CONNECTED, &con->flags)
>> T4: =====> interrupted by another thread;
>> T5: tipc_close_conn()
>> T6: test_and_clear_bit(con->flag)
>> T7: tipc_unregister_callbacks()
>> T8: flush_work(&con->swork)
>> T9: flush_work(&con->rwork)
>> T10: queue_work(s->send_wq, &con->swork)
>>
>>
>> The key point is that the whole process of identifying whether conn is/being
>> closed with test_bit(CF_CONNECTED, &con->flags) or
>> sk->sk_user_data and queuing swork item into workqueue is not atomic.
>
> That is true. But looking at the kref value is, because if that value is zero
> nobody else has a pointer to the connection item.
Yes, but it's unrelated to the case.
And vice versa, if it is non-zero the queues cannot not have been
flushed yet.
In the case, at T4, for example, we has used
kref_get_unless_zero(&con->kref) to confirm that con refcnt is really
not zero, so the checking result shows that we can queue a swork into
workqueue. But at the moment, tipc_conn_sendmsg() is interrupted. At T5
tipc_close_conn() is called on CPU2, and con CF_CONNECTED flag is
cleared, sk_user_data is set to null, and works have been flushed on the
CPU subsequently.
When tipc_conn_sendmsg() comes back at T10, as it is totally unaware
that CF_CONNECTED con->flags has been cleared and works have been
flushed, a swork item will still be queued to a flushed workqueue. As a
result, con may be finally destroyed in the context of calling
ipc_conn_sendmsg() instead of tipc module exit. It means that name table
soft lockup solved by 9dc3abdd1f7e may occurred again.
> I am not even sure that the CF_CONNECTED flag is needed anymore, but I need
> to think more about that.
CF_CONNECTED flag is still useful for us because we use it to identify
whether connection is broken or not in many places.
> What I am suggesting is that we move most of the functionality that is
> currently in close_conn() to keref_release(). Something like this:
>
> static void tipc_close_conn(struct tipc_conn *con, bool flush)
> {
> if (test_and_clear_bit(CF_CONNECTED, &con->flags))
> conn_put(con);
> }
>
> static void tipc_conn_kref_release(struct kref *kref)
> {
> struct tipc_conn *con = container_of(kref, struct tipc_conn, kref);
> struct tipc_server *s = con->server;
> struct sockaddr_tipc *saddr = s->saddr;
> struct socket *sock = con->sock;
> struct sock *sk;
>
> spin_lock_bh(&s->idr_lock);
> idr_remove(&s->conn_idr, con->conid);
> s->idr_in_use--;
> spin_unlock_bh(&s->idr_lock);
>
> if (sock) {
> sk = sock->sk;
> if (test_bit(CF_SERVER, &con->flags)) {
> __module_get(sock->ops->owner);
> __module_get(sk->sk_prot_creator->owner);
> }
> flush_work(&con->swork);
> flush_work(&con->rwork);
Sorry, we cannot move flush works to tipc_conn_kref_release(),
otherwise, that name table soft lockup solved by 9dc3abdd1f7e ("tipc:
fix nametbl_lock soft lockup at module exit") may occur again because
tipc_conn_kref_release may be called in tipc_nametbl_withdraw() rather
than tipc_topsrv_stop() context.
Moreover, before a work item is queued into workqueue, con refcnt needs
to be first held. So when tipc_conn_kref_release() is called, it means
there is no any user for a con. As a consequence, there is no any work
item which are still queued in workqueues, so we don't need to flush
works at all.
> saddr->scope = -TIPC_NODE_SCOPE;
> kernel_bind(sock, (struct sockaddr *)saddr, sizeof(*saddr));
> if (con->conid)
> s->tipc_conn_release(con->conid, con->usr_data);
> sock_release(sock);
> con->sock = NULL;
> }
> tipc_clean_outqueues(con);
> kfree(con);
> }
>
> ///jon
>
>> Even if all checking for connection status are well done and connection is
>> still
>> connected before swork item is queued, if the thread of running
>> tipc_conn_sendmsg() is interrupted at T4 and it's backed at T10, a swork
>> might be queued into workqueue after its queue has been flushed in
>> tipc_close_conn() on CPU 2.
>>
>> That's why we need use sk_callback_lock read lock to guarantee that the
>> entire process of identifying connection status and queuing swork item
>> cannot be interrupted.
>>
>>>
>>> ///jon
>>>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
tipc-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
