Re: [tipc-discussion] [net 1/2] tipc: fix memory leak in service subscripting
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 connectionsend-work
> |||
> sub-1 |<--//---||
> sub-2 |<--//---||
> ||<---| evt for sub-x
> sub-3 |<--//---||
> :::
> :::
> | /||
> | |* peer closed|
> | |||
> | ||<---X---| evt for sub-y
> | ||<===|
> sub-n |<--/Xshutdown|
> -> 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
> Signed-off-by: Tuong Lien
Acked-by: Ying Xue
> ---
> 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_callback_lock);
> - ret = tipc_conn_rcv_sub(srv, con, );
> + /* RACE: the connection can be closed in meanwhile */
> + if (likely(connected(con)))
> + ret = tipc_conn_rcv_sub(srv, con, );
> read_unlock_bh(>sk_callback_lock);
> if (!ret)
> return 0;
>
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
Re: [tipc-discussion] [net 1/2] tipc: fix memory leak in service subscripting
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 connectionsend-work
|||
sub-1 |<--//---||
sub-2 |<--//---||
||<---| evt for sub-x
sub-3 |<--//---||
:::
:::
| /||
| |* peer closed|
| |||
| ||<---X---| evt for sub-y
| ||<===|
sub-n |<--/Xshutdown|
-> 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
Signed-off-by: Tuong Lien
---
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_callback_lock);
- ret = tipc_conn_rcv_sub(srv, con, );
+ /* 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, );
read_unlock_bh(>sk_callback_lock);
if (!ret)
return 0;
Acked-by: Jon Maloy
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
[tipc-discussion] [net 1/2] tipc: fix memory leak in service subscripting
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 connectionsend-work
|||
sub-1 |<--//---||
sub-2 |<--//---||
||<---| evt for sub-x
sub-3 |<--//---||
:::
:::
| /||
| |* peer closed|
| |||
| ||<---X---| evt for sub-y
| ||<===|
sub-n |<--/Xshutdown|
-> 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
Signed-off-by: Tuong Lien
---
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_callback_lock);
- ret = tipc_conn_rcv_sub(srv, con, );
+ /* RACE: the connection can be closed in meanwhile */
+ if (likely(connected(con)))
+ ret = tipc_conn_rcv_sub(srv, con, );
read_unlock_bh(>sk_callback_lock);
if (!ret)
return 0;
--
2.13.7
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
