Good catch!
On 5/7/20 7:12 PM, Tuong Lien wrote:
> Upon receipt of a service subscription request from user via a topology
> connection, one 'sub' object will be allocated in kernel, so it will be
> able to send an event of the service if any to the user correspondingly
> then. Also, in case of any failure, the connection will be shutdown and
> all the pertaining 'sub' objects will be freed.
>
> However, there is a race condition as follows resulting in memory leak:
>
> receive-work connection send-work
> | | |
> sub-1 |<------//-------| |
> sub-2 |<------//-------| |
> | |<---------------| evt for sub-x
> sub-3 |<------//-------| |
> : : :
> : : :
> | /--------| |
> | | * peer closed |
> | | | |
> | | |<-------X-------| evt for sub-y
> | | |<===============|
> sub-n |<------/ X shutdown |
> -> orphan | |
>
> That is, the 'receive-work' may get the last subscription request while
> the 'send-work' is shutting down the connection due to peer close.
>
> We had a 'lock' on the connection, so the two actions cannot be carried
> out simultaneously. If the last subscription is allocated e.g. 'sub-n',
> before the 'send-work' closes the connection, there will be no issue at
> all, the 'sub' objects will be freed. In contrast the last subscription
> will become orphan since the connection was closed, and we released all
> references.
>
> This commit fixes the issue by simply adding one test if the connection
> remains in 'connected' state soon after we obtain the connection's lock
> then a subscription object can be created as usual, otherwise we ignore
> it.
>
> Reported-by: Thang Ngo <thang.h....@dektech.com.au>
> Signed-off-by: Tuong Lien <tuong.t.l...@dektech.com.au>
Acked-by: Ying Xue <ying....@windriver.com>
> ---
> net/tipc/topsrv.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 73dbed0c4b6b..399a89f6f1bf 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -400,7 +400,9 @@ static int tipc_conn_rcv_from_sock(struct tipc_conn *con)
> return -EWOULDBLOCK;
> if (ret == sizeof(s)) {
> read_lock_bh(&sk->sk_callback_lock);
> - ret = tipc_conn_rcv_sub(srv, con, &s);
> + /* RACE: the connection can be closed in meanwhile */
> + if (likely(connected(con)))
> + ret = tipc_conn_rcv_sub(srv, con, &s);
> read_unlock_bh(&sk->sk_callback_lock);
> if (!ret)
> return 0;
>
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
