* [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-17 8:35 ` Xin Long
0 siblings, 0 replies; 12+ messages in thread
From: Xin Long @ 2017-02-17 8:35 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, Vlad Yasevich
sctp has changed to use rhlist for transport rhashtable since commit
7fda702f9315 ("sctp: use new rhlist interface on sctp transport
rhashtable").
But rhltable_insert_key doesn't check the duplicate node when inserting
a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
assoc/transport in rhashtable. like:
client (addr A, B) server (addr X, Y)
connect to X INIT (1)
------------>
connect to Y INIT (2)
------------>
INIT_ACK (1)
<------------
INIT_ACK (2)
<------------
After sending INIT (2), one transport will be created and hashed into
rhashtable. But when receiving INIT_ACK (1) and processing the address
params, another transport will be created and hashed into rhashtable
with the same addr Y and EP as the last transport. This will confuse
the assoc/transport's lookup.
This patch is to fix it by returning err if any duplicate node exists
before inserting it.
Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/input.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 458e506..f65245b 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -872,6 +872,8 @@ void sctp_transport_hashtable_destroy(void)
int sctp_hash_transport(struct sctp_transport *t)
{
+ struct sctp_transport *transport;
+ struct rhlist_head *tmp, *list;
struct sctp_hash_cmp_arg arg;
int err;
@@ -882,8 +884,19 @@ int sctp_hash_transport(struct sctp_transport *t)
arg.paddr = &t->ipaddr;
arg.lport = htons(t->asoc->base.bind_addr.port);
+ list = rhltable_lookup(&sctp_transport_hashtable, &arg,
+ sctp_hash_params);
+
+ rhl_for_each_entry_rcu(transport, tmp, list, node)
+ if (transport->asoc->ep == t->asoc->ep) {
+ err = -EEXIST;
+ goto out;
+ }
+
err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
&t->node, sctp_hash_params);
+
+out:
if (err)
pr_err_once("insert transport fail, errno %d\n", err);
--
2.1.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-17 8:35 ` Xin Long
0 siblings, 0 replies; 12+ messages in thread
From: Xin Long @ 2017-02-17 8:35 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, Vlad Yasevich
sctp has changed to use rhlist for transport rhashtable since commit
7fda702f9315 ("sctp: use new rhlist interface on sctp transport
rhashtable").
But rhltable_insert_key doesn't check the duplicate node when inserting
a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
assoc/transport in rhashtable. like:
client (addr A, B) server (addr X, Y)
connect to X INIT (1)
------------>
connect to Y INIT (2)
------------>
INIT_ACK (1)
<------------
INIT_ACK (2)
<------------
After sending INIT (2), one transport will be created and hashed into
rhashtable. But when receiving INIT_ACK (1) and processing the address
params, another transport will be created and hashed into rhashtable
with the same addr Y and EP as the last transport. This will confuse
the assoc/transport's lookup.
This patch is to fix it by returning err if any duplicate node exists
before inserting it.
Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/input.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 458e506..f65245b 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -872,6 +872,8 @@ void sctp_transport_hashtable_destroy(void)
int sctp_hash_transport(struct sctp_transport *t)
{
+ struct sctp_transport *transport;
+ struct rhlist_head *tmp, *list;
struct sctp_hash_cmp_arg arg;
int err;
@@ -882,8 +884,19 @@ int sctp_hash_transport(struct sctp_transport *t)
arg.paddr = &t->ipaddr;
arg.lport = htons(t->asoc->base.bind_addr.port);
+ list = rhltable_lookup(&sctp_transport_hashtable, &arg,
+ sctp_hash_params);
+
+ rhl_for_each_entry_rcu(transport, tmp, list, node)
+ if (transport->asoc->ep = t->asoc->ep) {
+ err = -EEXIST;
+ goto out;
+ }
+
err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
&t->node, sctp_hash_params);
+
+out:
if (err)
pr_err_once("insert transport fail, errno %d\n", err);
--
2.1.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
2017-02-17 8:35 ` Xin Long
@ 2017-02-17 11:19 ` Neil Horman
-1 siblings, 0 replies; 12+ messages in thread
From: Neil Horman @ 2017-02-17 11:19 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, Vlad Yasevich
On Fri, Feb 17, 2017 at 04:35:24PM +0800, Xin Long wrote:
> sctp has changed to use rhlist for transport rhashtable since commit
> 7fda702f9315 ("sctp: use new rhlist interface on sctp transport
> rhashtable").
>
> But rhltable_insert_key doesn't check the duplicate node when inserting
> a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
> assoc/transport in rhashtable. like:
>
> client (addr A, B) server (addr X, Y)
> connect to X INIT (1)
> ------------>
> connect to Y INIT (2)
> ------------>
> INIT_ACK (1)
> <------------
> INIT_ACK (2)
> <------------
>
> After sending INIT (2), one transport will be created and hashed into
> rhashtable. But when receiving INIT_ACK (1) and processing the address
> params, another transport will be created and hashed into rhashtable
> with the same addr Y and EP as the last transport. This will confuse
> the assoc/transport's lookup.
>
> This patch is to fix it by returning err if any duplicate node exists
> before inserting it.
>
> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/input.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 458e506..f65245b 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -872,6 +872,8 @@ void sctp_transport_hashtable_destroy(void)
>
> int sctp_hash_transport(struct sctp_transport *t)
> {
> + struct sctp_transport *transport;
> + struct rhlist_head *tmp, *list;
> struct sctp_hash_cmp_arg arg;
> int err;
>
> @@ -882,8 +884,19 @@ int sctp_hash_transport(struct sctp_transport *t)
> arg.paddr = &t->ipaddr;
> arg.lport = htons(t->asoc->base.bind_addr.port);
>
> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> + sctp_hash_params);
> +
> + rhl_for_each_entry_rcu(transport, tmp, list, node)
> + if (transport->asoc->ep == t->asoc->ep) {
> + err = -EEXIST;
> + goto out;
> + }
> +
Maybe its just a little early, but I'm rather lost as to how this works. When
you insert a key to the rhltable, its a sctp_hash_cmp_arg that gets inserted,
which is a struct that consists of a sctp_addr union, a struct net, and a u16,
but when you traverse the list for lookup, you treat the returned list as a list
of sctp_transport structs that you compare. That seems very wrong.
Neil
> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> &t->node, sctp_hash_params);
> +
> +out:
> if (err)
> pr_err_once("insert transport fail, errno %d\n", err);
>
> --
> 2.1.0
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-17 11:19 ` Neil Horman
0 siblings, 0 replies; 12+ messages in thread
From: Neil Horman @ 2017-02-17 11:19 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, Vlad Yasevich
On Fri, Feb 17, 2017 at 04:35:24PM +0800, Xin Long wrote:
> sctp has changed to use rhlist for transport rhashtable since commit
> 7fda702f9315 ("sctp: use new rhlist interface on sctp transport
> rhashtable").
>
> But rhltable_insert_key doesn't check the duplicate node when inserting
> a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
> assoc/transport in rhashtable. like:
>
> client (addr A, B) server (addr X, Y)
> connect to X INIT (1)
> ------------>
> connect to Y INIT (2)
> ------------>
> INIT_ACK (1)
> <------------
> INIT_ACK (2)
> <------------
>
> After sending INIT (2), one transport will be created and hashed into
> rhashtable. But when receiving INIT_ACK (1) and processing the address
> params, another transport will be created and hashed into rhashtable
> with the same addr Y and EP as the last transport. This will confuse
> the assoc/transport's lookup.
>
> This patch is to fix it by returning err if any duplicate node exists
> before inserting it.
>
> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/input.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 458e506..f65245b 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -872,6 +872,8 @@ void sctp_transport_hashtable_destroy(void)
>
> int sctp_hash_transport(struct sctp_transport *t)
> {
> + struct sctp_transport *transport;
> + struct rhlist_head *tmp, *list;
> struct sctp_hash_cmp_arg arg;
> int err;
>
> @@ -882,8 +884,19 @@ int sctp_hash_transport(struct sctp_transport *t)
> arg.paddr = &t->ipaddr;
> arg.lport = htons(t->asoc->base.bind_addr.port);
>
> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> + sctp_hash_params);
> +
> + rhl_for_each_entry_rcu(transport, tmp, list, node)
> + if (transport->asoc->ep = t->asoc->ep) {
> + err = -EEXIST;
> + goto out;
> + }
> +
Maybe its just a little early, but I'm rather lost as to how this works. When
you insert a key to the rhltable, its a sctp_hash_cmp_arg that gets inserted,
which is a struct that consists of a sctp_addr union, a struct net, and a u16,
but when you traverse the list for lookup, you treat the returned list as a list
of sctp_transport structs that you compare. That seems very wrong.
Neil
> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> &t->node, sctp_hash_params);
> +
> +out:
> if (err)
> pr_err_once("insert transport fail, errno %d\n", err);
>
> --
> 2.1.0
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
2017-02-17 11:19 ` Neil Horman
@ 2017-02-17 14:32 ` Xin Long
-1 siblings, 0 replies; 12+ messages in thread
From: Xin Long @ 2017-02-17 14:32 UTC (permalink / raw)
To: Neil Horman
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, Vlad Yasevich
On Fri, Feb 17, 2017 at 7:19 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Fri, Feb 17, 2017 at 04:35:24PM +0800, Xin Long wrote:
>> sctp has changed to use rhlist for transport rhashtable since commit
>> 7fda702f9315 ("sctp: use new rhlist interface on sctp transport
>> rhashtable").
>>
>> But rhltable_insert_key doesn't check the duplicate node when inserting
>> a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
>> assoc/transport in rhashtable. like:
>>
>> client (addr A, B) server (addr X, Y)
>> connect to X INIT (1)
>> ------------>
>> connect to Y INIT (2)
>> ------------>
>> INIT_ACK (1)
>> <------------
>> INIT_ACK (2)
>> <------------
>>
>> After sending INIT (2), one transport will be created and hashed into
>> rhashtable. But when receiving INIT_ACK (1) and processing the address
>> params, another transport will be created and hashed into rhashtable
>> with the same addr Y and EP as the last transport. This will confuse
>> the assoc/transport's lookup.
>>
>> This patch is to fix it by returning err if any duplicate node exists
>> before inserting it.
>>
>> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
>> Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/sctp/input.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/net/sctp/input.c b/net/sctp/input.c
>> index 458e506..f65245b 100644
>> --- a/net/sctp/input.c
>> +++ b/net/sctp/input.c
>> @@ -872,6 +872,8 @@ void sctp_transport_hashtable_destroy(void)
>>
>> int sctp_hash_transport(struct sctp_transport *t)
>> {
>> + struct sctp_transport *transport;
>> + struct rhlist_head *tmp, *list;
>> struct sctp_hash_cmp_arg arg;
>> int err;
>>
>> @@ -882,8 +884,19 @@ int sctp_hash_transport(struct sctp_transport *t)
>> arg.paddr = &t->ipaddr;
>> arg.lport = htons(t->asoc->base.bind_addr.port);
>>
>> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
>> + sctp_hash_params);
>> +
>> + rhl_for_each_entry_rcu(transport, tmp, list, node)
>> + if (transport->asoc->ep == t->asoc->ep) {
>> + err = -EEXIST;
>> + goto out;
>> + }
>> +
> Maybe its just a little early, but I'm rather lost as to how this works. When
> you insert a key to the rhltable, its a sctp_hash_cmp_arg that gets inserted,
> which is a struct that consists of a sctp_addr union, a struct net, and a u16,
> but when you traverse the list for lookup, you treat the returned list as a list
> of sctp_transport structs that you compare. That seems very wrong.
The return list is "struct rhlist_head" list, "struct rhlist node" is
a member and
nested in struct sctp_transport, in fact this list consist of sctp_transport.
you can double check the inserting function "rhltable_insert_key()" below.
the param &t->node would be a node linked into this list.
rhltable_insert_key(struct rhlist_head *list[&t->node])
__rhashtable_insert_fast(struct rhash_head *obj [list->rhead])
...
list = container_of(obj, struct rhlist_head, rhead);
^---get back to rhash_list, then insert.
plist = container_of(head, struct rhlist_head, rhead);
RCU_INIT_POINTER(list->next, plist);
head = rht_dereference_bucket(head->next, tbl, hash);
RCU_INIT_POINTER(list->rhead.next, head);
...
the param *arg is NOT really the node used for inserting, but the param
*obj is.
>
> Neil
>
>> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
>> &t->node, sctp_hash_params);
>> +
>> +out:
>> if (err)
>> pr_err_once("insert transport fail, errno %d\n", err);
>>
>> --
>> 2.1.0
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-17 14:32 ` Xin Long
0 siblings, 0 replies; 12+ messages in thread
From: Xin Long @ 2017-02-17 14:32 UTC (permalink / raw)
To: Neil Horman
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, Vlad Yasevich
On Fri, Feb 17, 2017 at 7:19 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Fri, Feb 17, 2017 at 04:35:24PM +0800, Xin Long wrote:
>> sctp has changed to use rhlist for transport rhashtable since commit
>> 7fda702f9315 ("sctp: use new rhlist interface on sctp transport
>> rhashtable").
>>
>> But rhltable_insert_key doesn't check the duplicate node when inserting
>> a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
>> assoc/transport in rhashtable. like:
>>
>> client (addr A, B) server (addr X, Y)
>> connect to X INIT (1)
>> ------------>
>> connect to Y INIT (2)
>> ------------>
>> INIT_ACK (1)
>> <------------
>> INIT_ACK (2)
>> <------------
>>
>> After sending INIT (2), one transport will be created and hashed into
>> rhashtable. But when receiving INIT_ACK (1) and processing the address
>> params, another transport will be created and hashed into rhashtable
>> with the same addr Y and EP as the last transport. This will confuse
>> the assoc/transport's lookup.
>>
>> This patch is to fix it by returning err if any duplicate node exists
>> before inserting it.
>>
>> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
>> Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/sctp/input.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/net/sctp/input.c b/net/sctp/input.c
>> index 458e506..f65245b 100644
>> --- a/net/sctp/input.c
>> +++ b/net/sctp/input.c
>> @@ -872,6 +872,8 @@ void sctp_transport_hashtable_destroy(void)
>>
>> int sctp_hash_transport(struct sctp_transport *t)
>> {
>> + struct sctp_transport *transport;
>> + struct rhlist_head *tmp, *list;
>> struct sctp_hash_cmp_arg arg;
>> int err;
>>
>> @@ -882,8 +884,19 @@ int sctp_hash_transport(struct sctp_transport *t)
>> arg.paddr = &t->ipaddr;
>> arg.lport = htons(t->asoc->base.bind_addr.port);
>>
>> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
>> + sctp_hash_params);
>> +
>> + rhl_for_each_entry_rcu(transport, tmp, list, node)
>> + if (transport->asoc->ep = t->asoc->ep) {
>> + err = -EEXIST;
>> + goto out;
>> + }
>> +
> Maybe its just a little early, but I'm rather lost as to how this works. When
> you insert a key to the rhltable, its a sctp_hash_cmp_arg that gets inserted,
> which is a struct that consists of a sctp_addr union, a struct net, and a u16,
> but when you traverse the list for lookup, you treat the returned list as a list
> of sctp_transport structs that you compare. That seems very wrong.
The return list is "struct rhlist_head" list, "struct rhlist node" is
a member and
nested in struct sctp_transport, in fact this list consist of sctp_transport.
you can double check the inserting function "rhltable_insert_key()" below.
the param &t->node would be a node linked into this list.
rhltable_insert_key(struct rhlist_head *list[&t->node])
__rhashtable_insert_fast(struct rhash_head *obj [list->rhead])
...
list = container_of(obj, struct rhlist_head, rhead);
^---get back to rhash_list, then insert.
plist = container_of(head, struct rhlist_head, rhead);
RCU_INIT_POINTER(list->next, plist);
head = rht_dereference_bucket(head->next, tbl, hash);
RCU_INIT_POINTER(list->rhead.next, head);
...
the param *arg is NOT really the node used for inserting, but the param
*obj is.
>
> Neil
>
>> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
>> &t->node, sctp_hash_params);
>> +
>> +out:
>> if (err)
>> pr_err_once("insert transport fail, errno %d\n", err);
>>
>> --
>> 2.1.0
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
2017-02-17 8:35 ` Xin Long
@ 2017-02-17 20:19 ` David Miller
-1 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-02-17 20:19 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, vyasevich
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 17 Feb 2017 16:35:24 +0800
> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> + sctp_hash_params);
> +
> + rhl_for_each_entry_rcu(transport, tmp, list, node)
> + if (transport->asoc->ep == t->asoc->ep) {
> + err = -EEXIST;
> + goto out;
> + }
> +
> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> &t->node, sctp_hash_params);
> +
> +out:
Well, what if another thread of control inserts a matching transport
after you've checked the list but before rhltable_insert_key() does
it's work?
What write side lock is being held to protect the table from
modifications here?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-17 20:19 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-02-17 20:19 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, vyasevich
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 17 Feb 2017 16:35:24 +0800
> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> + sctp_hash_params);
> +
> + rhl_for_each_entry_rcu(transport, tmp, list, node)
> + if (transport->asoc->ep = t->asoc->ep) {
> + err = -EEXIST;
> + goto out;
> + }
> +
> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> &t->node, sctp_hash_params);
> +
> +out:
Well, what if another thread of control inserts a matching transport
after you've checked the list but before rhltable_insert_key() does
it's work?
What write side lock is being held to protect the table from
modifications here?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
2017-02-17 20:19 ` David Miller
@ 2017-02-18 15:47 ` Xin Long
-1 siblings, 0 replies; 12+ messages in thread
From: Xin Long @ 2017-02-18 15:47 UTC (permalink / raw)
To: David Miller
Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman,
Vlad Yasevich
On Sat, Feb 18, 2017 at 4:19 AM, David Miller <davem@davemloft.net> wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Fri, 17 Feb 2017 16:35:24 +0800
>
>
>> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
>> + sctp_hash_params);
>> +
>> + rhl_for_each_entry_rcu(transport, tmp, list, node)
>> + if (transport->asoc->ep == t->asoc->ep) {
>> + err = -EEXIST;
>> + goto out;
>> + }
>> +
>> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
>> &t->node, sctp_hash_params);
>> +
>> +out:
>
> Well, what if another thread of control inserts a matching transport
> after you've checked the list but before rhltable_insert_key() does
> it's work?
>
> What write side lock is being held to protect the table from
> modifications here?
sock lock.
...
sctp_assoc_add_peer()
sctp_hash_transport()
rhltable_insert_key()
all the places where it call sctp_assoc_add_peer() are proctected by
lock_sock(). it's a big lock, no need to worry about race issues here.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-18 15:47 ` Xin Long
0 siblings, 0 replies; 12+ messages in thread
From: Xin Long @ 2017-02-18 15:47 UTC (permalink / raw)
To: David Miller
Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman,
Vlad Yasevich
On Sat, Feb 18, 2017 at 4:19 AM, David Miller <davem@davemloft.net> wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Fri, 17 Feb 2017 16:35:24 +0800
>
>
>> + list = rhltable_lookup(&sctp_transport_hashtable, &arg,
>> + sctp_hash_params);
>> +
>> + rhl_for_each_entry_rcu(transport, tmp, list, node)
>> + if (transport->asoc->ep = t->asoc->ep) {
>> + err = -EEXIST;
>> + goto out;
>> + }
>> +
>> err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
>> &t->node, sctp_hash_params);
>> +
>> +out:
>
> Well, what if another thread of control inserts a matching transport
> after you've checked the list but before rhltable_insert_key() does
> it's work?
>
> What write side lock is being held to protect the table from
> modifications here?
sock lock.
...
sctp_assoc_add_peer()
sctp_hash_transport()
rhltable_insert_key()
all the places where it call sctp_assoc_add_peer() are proctected by
lock_sock(). it's a big lock, no need to worry about race issues here.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
2017-02-17 8:35 ` Xin Long
@ 2017-02-19 23:19 ` David Miller
-1 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-02-19 23:19 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, vyasevich
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 17 Feb 2017 16:35:24 +0800
> sctp has changed to use rhlist for transport rhashtable since commit
> 7fda702f9315 ("sctp: use new rhlist interface on sctp transport
> rhashtable").
>
> But rhltable_insert_key doesn't check the duplicate node when inserting
> a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
> assoc/transport in rhashtable. like:
>
> client (addr A, B) server (addr X, Y)
> connect to X INIT (1)
> ------------>
> connect to Y INIT (2)
> ------------>
> INIT_ACK (1)
> <------------
> INIT_ACK (2)
> <------------
>
> After sending INIT (2), one transport will be created and hashed into
> rhashtable. But when receiving INIT_ACK (1) and processing the address
> params, another transport will be created and hashed into rhashtable
> with the same addr Y and EP as the last transport. This will confuse
> the assoc/transport's lookup.
>
> This patch is to fix it by returning err if any duplicate node exists
> before inserting it.
>
> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] sctp: check duplicate node before inserting a new transport
@ 2017-02-19 23:19 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-02-19 23:19 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, vyasevich
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 17 Feb 2017 16:35:24 +0800
> sctp has changed to use rhlist for transport rhashtable since commit
> 7fda702f9315 ("sctp: use new rhlist interface on sctp transport
> rhashtable").
>
> But rhltable_insert_key doesn't check the duplicate node when inserting
> a node, unlike rhashtable_lookup_insert_key. It may cause duplicate
> assoc/transport in rhashtable. like:
>
> client (addr A, B) server (addr X, Y)
> connect to X INIT (1)
> ------------>
> connect to Y INIT (2)
> ------------>
> INIT_ACK (1)
> <------------
> INIT_ACK (2)
> <------------
>
> After sending INIT (2), one transport will be created and hashed into
> rhashtable. But when receiving INIT_ACK (1) and processing the address
> params, another transport will be created and hashed into rhashtable
> with the same addr Y and EP as the last transport. This will confuse
> the assoc/transport's lookup.
>
> This patch is to fix it by returning err if any duplicate node exists
> before inserting it.
>
> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> Reported-by: Fabio M. Di Nitto <fdinitto@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-02-19 23:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-17 8:35 [PATCH net] sctp: check duplicate node before inserting a new transport Xin Long
2017-02-17 8:35 ` Xin Long
2017-02-17 11:19 ` Neil Horman
2017-02-17 11:19 ` Neil Horman
2017-02-17 14:32 ` Xin Long
2017-02-17 14:32 ` Xin Long
2017-02-17 20:19 ` David Miller
2017-02-17 20:19 ` David Miller
2017-02-18 15:47 ` Xin Long
2017-02-18 15:47 ` Xin Long
2017-02-19 23:19 ` David Miller
2017-02-19 23:19 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.