Re: [tipc-discussion] [net 2/2] tipc: fix failed service subscription deletion
-Original Message-
From: Ying Xue
Sent: Monday, May 11, 2020 8:23 PM
To: Tuong Tong Lien ; jma...@redhat.com;
ma...@donjonn.com; tipc-discussion@lists.sourceforge.net
Cc: tipc-dek
Subject: Re: [net 2/2] tipc: fix failed service subscription deletion
On 5/7/20 7:12 PM, Tuong Lien wrote:
> When a service subscription is expired or canceled by user, it needs to
> be deleted from the subscription list, so that new subscriptions can be
> registered (max = 65535 per net). However, there are two issues in code
> that can cause such an unused subscription to persist:
>
> 1) The 'tipc_conn_delete_sub()' has a loop on the subscription list but
> it makes a break shortly when the 1st subscription differs from the one
> specified, so the subscription will not be deleted.
>
> 2) In case a subscription is canceled, the code to remove the
> 'TIPC_SUB_CANCEL' flag from the subscription filter does not work if it
> is a local subscription (i.e. the little endian isn't involved). So, it
> will be no matches when looking for the subscription to delete later.
>
> The subscription(s) will be removed eventually when the user terminates
> its topology connection but that could be a long time later. Meanwhile,
> the number of available subscriptions may be exhausted.
>
> This commit fixes the two issues above, so as needed a subscription can
> be deleted correctly.
>
> Signed-off-by: Tuong Lien
> ---
> net/tipc/topsrv.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 399a89f6f1bf..44609238849e 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -237,8 +237,6 @@ static void tipc_conn_delete_sub(struct tipc_conn *con,
> struct tipc_subscr *s)
> if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
> tipc_sub_unsubscribe(sub);
> atomic_dec(>subscription_count);
> - } else if (s) {
> - break;
I am quite surprised at this issue. The issue really exists as you
describe above, but it's better to fix it as follows:
@@ -237,8 +237,8 @@ static void tipc_conn_delete_sub(struct tipc_conn
*con, struct tipc_subscr *s)
if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
tipc_sub_unsubscribe(sub);
atomic_dec(>subscription_count);
- } else if (s) {
- break;
+ if (s)
+ break;
}
}
[Tuong]: Yes, this has been included in v3 of the patch along with the change
for the sub write below, please see from there.
BR/Tuong
> }
> }
> spin_unlock_bh(>sub_lock);
> @@ -364,7 +362,10 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
> struct tipc_subscription *sub;
>
> if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
> - s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + if (!(s->filter & TIPC_FILTER_MASK))
> + s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + else
> + s->filter &= ~TIPC_SUB_CANCEL;
> tipc_conn_delete_sub(con, s);
> return 0;
> }
>
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
Re: [tipc-discussion] [net 2/2] tipc: fix failed service subscription deletion
On 5/7/20 7:12 PM, Tuong Lien wrote:
> When a service subscription is expired or canceled by user, it needs to
> be deleted from the subscription list, so that new subscriptions can be
> registered (max = 65535 per net). However, there are two issues in code
> that can cause such an unused subscription to persist:
>
> 1) The 'tipc_conn_delete_sub()' has a loop on the subscription list but
> it makes a break shortly when the 1st subscription differs from the one
> specified, so the subscription will not be deleted.
>
> 2) In case a subscription is canceled, the code to remove the
> 'TIPC_SUB_CANCEL' flag from the subscription filter does not work if it
> is a local subscription (i.e. the little endian isn't involved). So, it
> will be no matches when looking for the subscription to delete later.
>
> The subscription(s) will be removed eventually when the user terminates
> its topology connection but that could be a long time later. Meanwhile,
> the number of available subscriptions may be exhausted.
>
> This commit fixes the two issues above, so as needed a subscription can
> be deleted correctly.
>
> Signed-off-by: Tuong Lien
> ---
> net/tipc/topsrv.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 399a89f6f1bf..44609238849e 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -237,8 +237,6 @@ static void tipc_conn_delete_sub(struct tipc_conn *con,
> struct tipc_subscr *s)
> if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
> tipc_sub_unsubscribe(sub);
> atomic_dec(>subscription_count);
> - } else if (s) {
> - break;
I am quite surprised at this issue. The issue really exists as you
describe above, but it's better to fix it as follows:
@@ -237,8 +237,8 @@ static void tipc_conn_delete_sub(struct tipc_conn
*con, struct tipc_subscr *s)
if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
tipc_sub_unsubscribe(sub);
atomic_dec(>subscription_count);
- } else if (s) {
- break;
+ if (s)
+ break;
}
}
> }
> }
> spin_unlock_bh(>sub_lock);
> @@ -364,7 +362,10 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
> struct tipc_subscription *sub;
>
> if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
> - s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + if (!(s->filter & TIPC_FILTER_MASK))
> + s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
> + else
> + s->filter &= ~TIPC_SUB_CANCEL;
> tipc_conn_delete_sub(con, s);
> return 0;
> }
>
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
Re: [tipc-discussion] [net 2/2] tipc: fix failed service subscription deletion
On 5/7/20 7:12 AM, Tuong Lien wrote:
When a service subscription is expired or canceled by user, it needs to
be deleted from the subscription list, so that new subscriptions can be
registered (max = 65535 per net). However, there are two issues in code
that can cause such an unused subscription to persist:
1) The 'tipc_conn_delete_sub()' has a loop on the subscription list but
it makes a break shortly when the 1st subscription differs from the one
specified, so the subscription will not be deleted.
2) In case a subscription is canceled, the code to remove the
'TIPC_SUB_CANCEL' flag from the subscription filter does not work if it
is a local subscription (i.e. the little endian isn't involved). So, it
will be no matches when looking for the subscription to delete later.
The subscription(s) will be removed eventually when the user terminates
its topology connection but that could be a long time later. Meanwhile,
the number of available subscriptions may be exhausted.
This commit fixes the two issues above, so as needed a subscription can
be deleted correctly.
Signed-off-by: Tuong Lien
---
net/tipc/topsrv.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 399a89f6f1bf..44609238849e 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -237,8 +237,6 @@ static void tipc_conn_delete_sub(struct tipc_conn *con,
struct tipc_subscr *s)
if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
tipc_sub_unsubscribe(sub);
atomic_dec(>subscription_count);
- } else if (s) {
- break;
ok
}
}
spin_unlock_bh(>sub_lock);
@@ -364,7 +362,10 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
struct tipc_subscription *sub;
if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
- s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
+ if (!(s->filter & TIPC_FILTER_MASK))
+ s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
+ else
+ s->filter &= ~TIPC_SUB_CANCEL;
It has a value to keep the little-endian/big-endian issues isolated in
separate code, as I did
with the macros tipc_sub_read() and tipc_evt_write(). In this case we
need another macro,
tipc_sub_write(), despite the fact that it is used only once.
BR
///jon
tipc_conn_delete_sub(con, s);
return 0;
}
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
[tipc-discussion] [net 2/2] tipc: fix failed service subscription deletion
When a service subscription is expired or canceled by user, it needs to
be deleted from the subscription list, so that new subscriptions can be
registered (max = 65535 per net). However, there are two issues in code
that can cause such an unused subscription to persist:
1) The 'tipc_conn_delete_sub()' has a loop on the subscription list but
it makes a break shortly when the 1st subscription differs from the one
specified, so the subscription will not be deleted.
2) In case a subscription is canceled, the code to remove the
'TIPC_SUB_CANCEL' flag from the subscription filter does not work if it
is a local subscription (i.e. the little endian isn't involved). So, it
will be no matches when looking for the subscription to delete later.
The subscription(s) will be removed eventually when the user terminates
its topology connection but that could be a long time later. Meanwhile,
the number of available subscriptions may be exhausted.
This commit fixes the two issues above, so as needed a subscription can
be deleted correctly.
Signed-off-by: Tuong Lien
---
net/tipc/topsrv.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 399a89f6f1bf..44609238849e 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -237,8 +237,6 @@ static void tipc_conn_delete_sub(struct tipc_conn *con,
struct tipc_subscr *s)
if (!s || !memcmp(s, >evt.s, sizeof(*s))) {
tipc_sub_unsubscribe(sub);
atomic_dec(>subscription_count);
- } else if (s) {
- break;
}
}
spin_unlock_bh(>sub_lock);
@@ -364,7 +362,10 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
struct tipc_subscription *sub;
if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
- s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
+ if (!(s->filter & TIPC_FILTER_MASK))
+ s->filter &= __constant_ntohl(~TIPC_SUB_CANCEL);
+ else
+ s->filter &= ~TIPC_SUB_CANCEL;
tipc_conn_delete_sub(con, s);
return 0;
}
--
2.13.7
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
