> -----Original Message-----
> From: Ying Xue [mailto:[email protected]]
> Sent: Monday, December 11, 2017 06:57
> 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 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.
I was of course assuming that the queues are flushed in
tipc_conn_kref_release() as I suggested. Then this wouldn't be a problem.
>
> 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
[...]
> > 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.
It is both possible and necessary, as I see it. The key to achieve this is to
move all deletes, i.e., all calls to xxx_put() of any kind, to the receiving
context of either work queue. Never in an interrupt context, and never from the
event trigging side of the send work queue.
If you look at my patch #4(v3) that I sent out two weeks ago I do exactly this,
but for a different reason; because this might happen even at a subscription
timeout event, although with less fatal consequences.
You don't have to eliminate the tipc_subscriber structure to achieve this, as I
did, (although I strongly recommend it; but that can be done later). You only
need to send some hint along with the subscription event that this is the final
event for this subscription, and that it should be deleted, in work queue
context, after the event has been posted on the socket (or dropped).
>
> 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.
Even better. So, we never need to flush any work queues, only the send event
queue.
Regards
///jon
>
> > 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
