Re: [tipc-discussion] [net 1/2] tipc: fix memory leak in service subscripting

[Jon M. <jm...@re...>]

On 5/7/20 7:12 AM, 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 <tha...@de...>
> Signed-off-by: Tuong Lien <tuo...@de...>
> ---
>   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 */
s/in meanwhile/in the meantime/
> +		if (likely(connected(con)))
> +			ret = tipc_conn_rcv_sub(srv, con, &s);
>   		read_unlock_bh(&sk->sk_callback_lock);
>   		if (!ret)
>   			return 0;
Acked-by: Jon Maloy <jm...@re...>