Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
These events are described in rfc6458#section-6.1
SCTP_PEER_ADDR_CHANGE:
This tag indicates that an address that is
part of an existing association has experienced a change of
state (e.g., a failure or return to service of the reachability
of an endpoint via a specific transport address).
Signed-off-by: Jonas Falkevik <[email protected]>
---
net/sctp/associola.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 437079a4883d..0c5dd295f9b8 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -432,8 +432,10 @@ void sctp_assoc_set_primary(struct sctp_association *asoc,
changeover = 1 ;
asoc->peer.primary_path = transport;
- sctp_ulpevent_nofity_peer_addr_change(transport,
- SCTP_ADDR_MADE_PRIM, 0);
+ if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
+ sctp_ulpevent_nofity_peer_addr_change(transport,
+ SCTP_ADDR_MADE_PRIM,
+ 0);
/* Set a default msg_name for events. */
memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
@@ -714,7 +716,10 @@ struct sctp_transport *sctp_assoc_add_peer(struct
sctp_association *asoc,
list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list);
asoc->peer.transport_count++;
- sctp_ulpevent_nofity_peer_addr_change(peer, SCTP_ADDR_ADDED, 0);
+ if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
+ sctp_ulpevent_nofity_peer_addr_change(peer,
+ SCTP_ADDR_ADDED,
+ 0);
/* If we do not yet have a primary path, set one. */
if (!asoc->peer.primary_path) {
--
2.25.3
On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
How did you get them?
I'm thinking you're fixing a side-effect of another issue here. For
example, in sctp_assoc_update(), it first calls sctp_assoc_add_peer()
to only then call sctp_assoc_set_id(), which would generate the event
you might have seen. In this case, it should be allocating IDR before,
so that the event can be sent with the right assoc_id already.
>
> These events are described in rfc6458#section-6.1
> SCTP_PEER_ADDR_CHANGE:
> This tag indicates that an address that is
> part of an existing association has experienced a change of
> state (e.g., a failure or return to service of the reachability
> of an endpoint via a specific transport address).
>
> Signed-off-by: Jonas Falkevik <[email protected]>
> ---
> net/sctp/associola.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 437079a4883d..0c5dd295f9b8 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -432,8 +432,10 @@ void sctp_assoc_set_primary(struct sctp_association *asoc,
> changeover = 1 ;
>
> asoc->peer.primary_path = transport;
> - sctp_ulpevent_nofity_peer_addr_change(transport,
> - SCTP_ADDR_MADE_PRIM, 0);
> + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> + sctp_ulpevent_nofity_peer_addr_change(transport,
> + SCTP_ADDR_MADE_PRIM,
> + 0);
>
> /* Set a default msg_name for events. */
> memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
> @@ -714,7 +716,10 @@ struct sctp_transport *sctp_assoc_add_peer(struct
> sctp_association *asoc,
> list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list);
> asoc->peer.transport_count++;
>
> - sctp_ulpevent_nofity_peer_addr_change(peer, SCTP_ADDR_ADDED, 0);
> + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> + sctp_ulpevent_nofity_peer_addr_change(peer,
> + SCTP_ADDR_ADDED,
> + 0);
>
> /* If we do not yet have a primary path, set one. */
> if (!asoc->peer.primary_path) {
> --
> 2.25.3
On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
<[email protected]> wrote:
>
> On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
>
> How did you get them?
>
I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
Here a closed association is created, sctp_make_temp_assoc().
Which is later used when calling sctp_process_init().
In sctp_process_init() one of the first things are to call
sctp_assoc_add_peer()
on the closed / temp assoc.
sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
for the potentially new association.
$ cat sctp.bpftrace
#!/usr/local/bin/bpftrace
BEGIN
{
printf("Tracing sctp_assoc_add_peer\n");
printf("Hit Ctrl-C to end.\n");
}
kprobe:sctp_assoc_add_peer
{
@[kstack]=count();
}
$ sudo bpftrace sctp.bpftrace
Attaching 2 probes...
Tracing sctp_assoc_add_peer
Hit Ctrl-C to end.
^C
@[
sctp_assoc_add_peer+1
sctp_process_init+77
sctp_sf_do_5_1B_init+615
sctp_do_sm+132
sctp_endpoint_bh_rcv+256
sctp_rcv+2379
ip_protocol_deliver_rcu+393
ip_local_deliver_finish+68
ip_local_deliver+203
ip_rcv+156
__netif_receive_skb_one_core+96
process_backlog+164
net_rx_action+312
__softirqentry_text_start+238
do_softirq_own_stack+42
do_softirq.part.0+65
__local_bh_enable_ip+75
ip_finish_output2+415
ip_output+102
__ip_queue_xmit+364
sctp_packet_transmit+1814
sctp_outq_flush_ctrl.constprop.0+394
sctp_outq_flush+86
sctp_do_sm+3914
sctp_primitive_ASSOCIATE+44
__sctp_connect+707
sctp_inet_connect+98
__sys_connect+156
__x64_sys_connect+22
do_syscall_64+91
entry_SYSCALL_64_after_hwframe+68
]: 1
...
> I'm thinking you're fixing a side-effect of another issue here. For
> example, in sctp_assoc_update(), it first calls sctp_assoc_add_peer()
> to only then call sctp_assoc_set_id(), which would generate the event
> you might have seen. In this case, it should be allocating IDR before,
> so that the event can be sent with the right assoc_id already.
>
> >
> > These events are described in rfc6458#section-6.1
> > SCTP_PEER_ADDR_CHANGE:
> > This tag indicates that an address that is
> > part of an existing association has experienced a change of
> > state (e.g., a failure or return to service of the reachability
> > of an endpoint via a specific transport address).
> >
> > Signed-off-by: Jonas Falkevik <[email protected]>
> > ---
> > net/sctp/associola.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 437079a4883d..0c5dd295f9b8 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -432,8 +432,10 @@ void sctp_assoc_set_primary(struct sctp_association *asoc,
> > changeover = 1 ;
> >
> > asoc->peer.primary_path = transport;
> > - sctp_ulpevent_nofity_peer_addr_change(transport,
> > - SCTP_ADDR_MADE_PRIM, 0);
> > + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> > + sctp_ulpevent_nofity_peer_addr_change(transport,
> > + SCTP_ADDR_MADE_PRIM,
> > + 0);
> >
> > /* Set a default msg_name for events. */
> > memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
> > @@ -714,7 +716,10 @@ struct sctp_transport *sctp_assoc_add_peer(struct
> > sctp_association *asoc,
> > list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list);
> > asoc->peer.transport_count++;
> >
> > - sctp_ulpevent_nofity_peer_addr_change(peer, SCTP_ADDR_ADDED, 0);
> > + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> > + sctp_ulpevent_nofity_peer_addr_change(peer,
> > + SCTP_ADDR_ADDED,
> > + 0);
> >
> > /* If we do not yet have a primary path, set one. */
> > if (!asoc->peer.primary_path) {
> > --
> > 2.25.3
On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> <[email protected]> wrote:
> >
> > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> >
> > How did you get them?
> >
>
> I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> Here a closed association is created, sctp_make_temp_assoc().
> Which is later used when calling sctp_process_init().
> In sctp_process_init() one of the first things are to call
> sctp_assoc_add_peer()
> on the closed / temp assoc.
>
> sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> for the potentially new association.
I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
for setting/getting socket options that will be used for new asocs. In
this case, it is just a coincidence that asoc_id is not set (but
initialized to 0) and SCTP_FUTURE_ASSOC is also 0. Moreso, if I didn't
miss anything, it would block valid events, such as those from
sctp_sf_do_5_1D_ce
sctp_process_init
because sctp_process_init will only call sctp_assoc_set_id() by its
end.
I can't see a good reason for generating any event on temp assocs. So
I'm thinking the checks on this patch should be on whether the asoc is
a temporary one instead. WDYT?
Then, considering the socket is locked, both code points should be
allocating the IDR earlier. It's expensive, yes (point being, it could
be avoided in case of other failures), but it should be generating
events with the right assoc id. Are you interested in pursuing this
fix as well?
>
> $ cat sctp.bpftrace
> #!/usr/local/bin/bpftrace
>
> BEGIN
> {
> printf("Tracing sctp_assoc_add_peer\n");
> printf("Hit Ctrl-C to end.\n");
> }
>
> kprobe:sctp_assoc_add_peer
> {
> @[kstack]=count();
> }
>
> $ sudo bpftrace sctp.bpftrace
> Attaching 2 probes...
> Tracing sctp_assoc_add_peer
> Hit Ctrl-C to end.
> ^C
>
> @[
> sctp_assoc_add_peer+1
> sctp_process_init+77
> sctp_sf_do_5_1B_init+615
> sctp_do_sm+132
> sctp_endpoint_bh_rcv+256
> sctp_rcv+2379
> ip_protocol_deliver_rcu+393
> ip_local_deliver_finish+68
> ip_local_deliver+203
> ip_rcv+156
> __netif_receive_skb_one_core+96
> process_backlog+164
> net_rx_action+312
> __softirqentry_text_start+238
> do_softirq_own_stack+42
> do_softirq.part.0+65
> __local_bh_enable_ip+75
> ip_finish_output2+415
> ip_output+102
> __ip_queue_xmit+364
> sctp_packet_transmit+1814
> sctp_outq_flush_ctrl.constprop.0+394
> sctp_outq_flush+86
> sctp_do_sm+3914
> sctp_primitive_ASSOCIATE+44
> __sctp_connect+707
> sctp_inet_connect+98
> __sys_connect+156
> __x64_sys_connect+22
> do_syscall_64+91
> entry_SYSCALL_64_after_hwframe+68
> ]: 1
> ...
>
> > I'm thinking you're fixing a side-effect of another issue here. For
> > example, in sctp_assoc_update(), it first calls sctp_assoc_add_peer()
> > to only then call sctp_assoc_set_id(), which would generate the event
> > you might have seen. In this case, it should be allocating IDR before,
> > so that the event can be sent with the right assoc_id already.
> >
> > >
> > > These events are described in rfc6458#section-6.1
> > > SCTP_PEER_ADDR_CHANGE:
> > > This tag indicates that an address that is
> > > part of an existing association has experienced a change of
> > > state (e.g., a failure or return to service of the reachability
> > > of an endpoint via a specific transport address).
> > >
> > > Signed-off-by: Jonas Falkevik <[email protected]>
> > > ---
> > > net/sctp/associola.c | 11 ++++++++---
> > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > > index 437079a4883d..0c5dd295f9b8 100644
> > > --- a/net/sctp/associola.c
> > > +++ b/net/sctp/associola.c
> > > @@ -432,8 +432,10 @@ void sctp_assoc_set_primary(struct sctp_association *asoc,
> > > changeover = 1 ;
> > >
> > > asoc->peer.primary_path = transport;
> > > - sctp_ulpevent_nofity_peer_addr_change(transport,
> > > - SCTP_ADDR_MADE_PRIM, 0);
> > > + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> > > + sctp_ulpevent_nofity_peer_addr_change(transport,
> > > + SCTP_ADDR_MADE_PRIM,
> > > + 0);
> > >
> > > /* Set a default msg_name for events. */
> > > memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
> > > @@ -714,7 +716,10 @@ struct sctp_transport *sctp_assoc_add_peer(struct
> > > sctp_association *asoc,
> > > list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list);
> > > asoc->peer.transport_count++;
> > >
> > > - sctp_ulpevent_nofity_peer_addr_change(peer, SCTP_ADDR_ADDED, 0);
> > > + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> > > + sctp_ulpevent_nofity_peer_addr_change(peer,
> > > + SCTP_ADDR_ADDED,
> > > + 0);
> > >
> > > /* If we do not yet have a primary path, set one. */
> > > if (!asoc->peer.primary_path) {
> > > --
> > > 2.25.3
On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
<[email protected]> wrote:
>
> On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > <[email protected]> wrote:
> > >
> > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > >
> > > How did you get them?
> > >
> >
> > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > Here a closed association is created, sctp_make_temp_assoc().
> > Which is later used when calling sctp_process_init().
> > In sctp_process_init() one of the first things are to call
> > sctp_assoc_add_peer()
> > on the closed / temp assoc.
> >
> > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > for the potentially new association.
>
> I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> for setting/getting socket options that will be used for new asocs. In
> this case, it is just a coincidence that asoc_id is not set (but
> initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
yes, you are right, I overlooked that.
> Moreso, if I didn't
> miss anything, it would block valid events, such as those from
> sctp_sf_do_5_1D_ce
> sctp_process_init
> because sctp_process_init will only call sctp_assoc_set_id() by its
> end.
Do we want these events at this stage?
Since the association is a newly established one, have the peer address changed?
Should we enqueue these messages with sm commands instead?
And drop them if we don't have state SCTP_STATE_ESTABLISHED?
>
> I can't see a good reason for generating any event on temp assocs. So
> I'm thinking the checks on this patch should be on whether the asoc is
> a temporary one instead. WDYT?
>
Agree, we shouldn't rely on coincidence.
Either check temp instead or the above mentioned state?
> Then, considering the socket is locked, both code points should be
> allocating the IDR earlier. It's expensive, yes (point being, it could
> be avoided in case of other failures), but it should be generating
> events with the right assoc id. Are you interested in pursuing this
> fix as well?
Sure.
If we check temp status instead, we would need to allocate IDR earlier,
as you mention. So that we send the notification with correct assoc id.
But shouldn't the SCTP_COMM_UP, for a newly established association, be the
first notification event sent?
The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
>
> >
> > $ cat sctp.bpftrace
> > #!/usr/local/bin/bpftrace
> >
> > BEGIN
> > {
> > printf("Tracing sctp_assoc_add_peer\n");
> > printf("Hit Ctrl-C to end.\n");
> > }
> >
> > kprobe:sctp_assoc_add_peer
> > {
> > @[kstack]=count();
> > }
> >
> > $ sudo bpftrace sctp.bpftrace
> > Attaching 2 probes...
> > Tracing sctp_assoc_add_peer
> > Hit Ctrl-C to end.
> > ^C
> >
> > @[
> > sctp_assoc_add_peer+1
> > sctp_process_init+77
> > sctp_sf_do_5_1B_init+615
> > sctp_do_sm+132
> > sctp_endpoint_bh_rcv+256
> > sctp_rcv+2379
> > ip_protocol_deliver_rcu+393
> > ip_local_deliver_finish+68
> > ip_local_deliver+203
> > ip_rcv+156
> > __netif_receive_skb_one_core+96
> > process_backlog+164
> > net_rx_action+312
> > __softirqentry_text_start+238
> > do_softirq_own_stack+42
> > do_softirq.part.0+65
> > __local_bh_enable_ip+75
> > ip_finish_output2+415
> > ip_output+102
> > __ip_queue_xmit+364
> > sctp_packet_transmit+1814
> > sctp_outq_flush_ctrl.constprop.0+394
> > sctp_outq_flush+86
> > sctp_do_sm+3914
> > sctp_primitive_ASSOCIATE+44
> > __sctp_connect+707
> > sctp_inet_connect+98
> > __sys_connect+156
> > __x64_sys_connect+22
> > do_syscall_64+91
> > entry_SYSCALL_64_after_hwframe+68
> > ]: 1
> > ...
> >
> > > I'm thinking you're fixing a side-effect of another issue here. For
> > > example, in sctp_assoc_update(), it first calls sctp_assoc_add_peer()
> > > to only then call sctp_assoc_set_id(), which would generate the event
> > > you might have seen. In this case, it should be allocating IDR before,
> > > so that the event can be sent with the right assoc_id already.
> > >
> > > >
> > > > These events are described in rfc6458#section-6.1
> > > > SCTP_PEER_ADDR_CHANGE:
> > > > This tag indicates that an address that is
> > > > part of an existing association has experienced a change of
> > > > state (e.g., a failure or return to service of the reachability
> > > > of an endpoint via a specific transport address).
> > > >
> > > > Signed-off-by: Jonas Falkevik <[email protected]>
> > > > ---
> > > > net/sctp/associola.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > > > index 437079a4883d..0c5dd295f9b8 100644
> > > > --- a/net/sctp/associola.c
> > > > +++ b/net/sctp/associola.c
> > > > @@ -432,8 +432,10 @@ void sctp_assoc_set_primary(struct sctp_association *asoc,
> > > > changeover = 1 ;
> > > >
> > > > asoc->peer.primary_path = transport;
> > > > - sctp_ulpevent_nofity_peer_addr_change(transport,
> > > > - SCTP_ADDR_MADE_PRIM, 0);
> > > > + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> > > > + sctp_ulpevent_nofity_peer_addr_change(transport,
> > > > + SCTP_ADDR_MADE_PRIM,
> > > > + 0);
> > > >
> > > > /* Set a default msg_name for events. */
> > > > memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
> > > > @@ -714,7 +716,10 @@ struct sctp_transport *sctp_assoc_add_peer(struct
> > > > sctp_association *asoc,
> > > > list_add_tail_rcu(&peer->transports, &asoc->peer.transport_addr_list);
> > > > asoc->peer.transport_count++;
> > > >
> > > > - sctp_ulpevent_nofity_peer_addr_change(peer, SCTP_ADDR_ADDED, 0);
> > > > + if (sctp_assoc2id(asoc) != SCTP_FUTURE_ASSOC)
> > > > + sctp_ulpevent_nofity_peer_addr_change(peer,
> > > > + SCTP_ADDR_ADDED,
> > > > + 0);
> > > >
> > > > /* If we do not yet have a primary path, set one. */
> > > > if (!asoc->peer.primary_path) {
> > > > --
> > > > 2.25.3
On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> <[email protected]> wrote:
> >
> > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > >
> > > > How did you get them?
> > > >
> > >
> > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > Here a closed association is created, sctp_make_temp_assoc().
> > > Which is later used when calling sctp_process_init().
> > > In sctp_process_init() one of the first things are to call
> > > sctp_assoc_add_peer()
> > > on the closed / temp assoc.
> > >
> > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > for the potentially new association.
> >
> > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > for setting/getting socket options that will be used for new asocs. In
> > this case, it is just a coincidence that asoc_id is not set (but
> > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
>
> yes, you are right, I overlooked that.
>
> > Moreso, if I didn't
> > miss anything, it would block valid events, such as those from
> > sctp_sf_do_5_1D_ce
> > sctp_process_init
> > because sctp_process_init will only call sctp_assoc_set_id() by its
> > end.
>
> Do we want these events at this stage?
> Since the association is a newly established one, have the peer address changed?
> Should we enqueue these messages with sm commands instead?
> And drop them if we don't have state SCTP_STATE_ESTABLISHED?
>
> >
> > I can't see a good reason for generating any event on temp assocs. So
> > I'm thinking the checks on this patch should be on whether the asoc is
> > a temporary one instead. WDYT?
> >
>
> Agree, we shouldn't rely on coincidence.
> Either check temp instead or the above mentioned state?
>
> > Then, considering the socket is locked, both code points should be
> > allocating the IDR earlier. It's expensive, yes (point being, it could
> > be avoided in case of other failures), but it should be generating
> > events with the right assoc id. Are you interested in pursuing this
> > fix as well?
>
> Sure.
>
> If we check temp status instead, we would need to allocate IDR earlier,
> as you mention. So that we send the notification with correct assoc id.
>
> But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> first notification event sent?
> The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
The RFC doesn't mention any specific ordering for them, but it would
make sense. Reading the FreeBSD code now (which I consider a reference
implementation), it doesn't raise these notifications from
INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
event is ASCONF ADD command itself. So these are extra in Linux, and
I'm afraid we got to stick with them.
Considering the error handling it already has, looks like the
reordering is feasible and welcomed. I'm thinking the temp check and
reordering is the best way forward here.
Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
breakage.
Thanks,
Marcelo
On Tue, May 19, 2020 at 10:42 PM Marcelo Ricardo Leitner
<[email protected]> wrote:
>
> On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> > On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> > <[email protected]> wrote:
> > >
> > > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > > >
> > > > > How did you get them?
> > > > >
> > > >
> > > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > > Here a closed association is created, sctp_make_temp_assoc().
> > > > Which is later used when calling sctp_process_init().
> > > > In sctp_process_init() one of the first things are to call
> > > > sctp_assoc_add_peer()
> > > > on the closed / temp assoc.
> > > >
> > > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > > for the potentially new association.
> > >
> > > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > > for setting/getting socket options that will be used for new asocs. In
> > > this case, it is just a coincidence that asoc_id is not set (but
> > > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
> >
> > yes, you are right, I overlooked that.
> >
> > > Moreso, if I didn't
> > > miss anything, it would block valid events, such as those from
> > > sctp_sf_do_5_1D_ce
> > > sctp_process_init
> > > because sctp_process_init will only call sctp_assoc_set_id() by its
> > > end.
> >
> > Do we want these events at this stage?
> > Since the association is a newly established one, have the peer address changed?
> > Should we enqueue these messages with sm commands instead?
> > And drop them if we don't have state SCTP_STATE_ESTABLISHED?
> >
> > >
> > > I can't see a good reason for generating any event on temp assocs. So
> > > I'm thinking the checks on this patch should be on whether the asoc is
> > > a temporary one instead. WDYT?
> > >
> >
> > Agree, we shouldn't rely on coincidence.
> > Either check temp instead or the above mentioned state?
> >
> > > Then, considering the socket is locked, both code points should be
> > > allocating the IDR earlier. It's expensive, yes (point being, it could
> > > be avoided in case of other failures), but it should be generating
> > > events with the right assoc id. Are you interested in pursuing this
> > > fix as well?
> >
> > Sure.
> >
> > If we check temp status instead, we would need to allocate IDR earlier,
> > as you mention. So that we send the notification with correct assoc id.
> >
> > But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> > first notification event sent?
> > The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
>
> The RFC doesn't mention any specific ordering for them, but it would
> make sense. Reading the FreeBSD code now (which I consider a reference
> implementation), it doesn't raise these notifications from
> INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
> event is ASCONF ADD command itself. So these are extra in Linux, and
> I'm afraid we got to stick with them.
>
> Considering the error handling it already has, looks like the
> reordering is feasible and welcomed. I'm thinking the temp check and
> reordering is the best way forward here.
>
> Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
> breakage.
Some order is mentioned in RFC 6458 Chapter 6.1.1.
SCTP_COMM_UP: A new association is now ready, and data may be
exchanged with this peer. When an association has been
established successfully, this notification should be the
first one.
I can make a patch with a check on temp and make COMM_UP event first.
Currently the COMM_UP event is enqueued via commands
while the SCTP_ADDR_ADDED event is enqueued directly.
sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
vs.
asoc->stream.si->enqueue_event(&asoc->ulpq, event);
Do you want me to change to use commands instead of enqueing?
Or should we enqueue the COMM_UP event directly?
-Jonas
On Sat, May 23, 2020 at 8:04 PM Jonas Falkevik <[email protected]> wrote:
>
> On Tue, May 19, 2020 at 10:42 PM Marcelo Ricardo Leitner
> <[email protected]> wrote:
> >
> > On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> > > On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > > > >
> > > > > > How did you get them?
> > > > > >
> > > > >
> > > > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > > > Here a closed association is created, sctp_make_temp_assoc().
> > > > > Which is later used when calling sctp_process_init().
> > > > > In sctp_process_init() one of the first things are to call
> > > > > sctp_assoc_add_peer()
> > > > > on the closed / temp assoc.
> > > > >
> > > > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > > > for the potentially new association.
> > > >
> > > > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > > > for setting/getting socket options that will be used for new asocs. In
> > > > this case, it is just a coincidence that asoc_id is not set (but
> > > > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
> > >
> > > yes, you are right, I overlooked that.
> > >
> > > > Moreso, if I didn't
> > > > miss anything, it would block valid events, such as those from
> > > > sctp_sf_do_5_1D_ce
> > > > sctp_process_init
> > > > because sctp_process_init will only call sctp_assoc_set_id() by its
> > > > end.
> > >
> > > Do we want these events at this stage?
> > > Since the association is a newly established one, have the peer address changed?
> > > Should we enqueue these messages with sm commands instead?
> > > And drop them if we don't have state SCTP_STATE_ESTABLISHED?
> > >
> > > >
> > > > I can't see a good reason for generating any event on temp assocs. So
> > > > I'm thinking the checks on this patch should be on whether the asoc is
> > > > a temporary one instead. WDYT?
> > > >
> > >
> > > Agree, we shouldn't rely on coincidence.
> > > Either check temp instead or the above mentioned state?
> > >
> > > > Then, considering the socket is locked, both code points should be
> > > > allocating the IDR earlier. It's expensive, yes (point being, it could
> > > > be avoided in case of other failures), but it should be generating
> > > > events with the right assoc id. Are you interested in pursuing this
> > > > fix as well?
> > >
> > > Sure.
> > >
> > > If we check temp status instead, we would need to allocate IDR earlier,
> > > as you mention. So that we send the notification with correct assoc id.
> > >
> > > But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> > > first notification event sent?
> > > The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
> >
> > The RFC doesn't mention any specific ordering for them, but it would
> > make sense. Reading the FreeBSD code now (which I consider a reference
> > implementation), it doesn't raise these notifications from
> > INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
> > event is ASCONF ADD command itself. So these are extra in Linux, and
> > I'm afraid we got to stick with them.
> >
> > Considering the error handling it already has, looks like the
> > reordering is feasible and welcomed. I'm thinking the temp check and
> > reordering is the best way forward here.
> >
> > Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
> > breakage.
>
> Some order is mentioned in RFC 6458 Chapter 6.1.1.
>
> SCTP_COMM_UP: A new association is now ready, and data may be
> exchanged with this peer. When an association has been
> established successfully, this notification should be the
> first one.
If this is true, as SCTP_COMM_UP event is always followed by state changed
to ESTABLISHED. So I'm thinking to NOT make addr events by checking the
state:
@@ -343,6 +343,9 @@ void sctp_ulpevent_nofity_peer_addr_change(struct
sctp_transport *transport,
struct sockaddr_storage addr;
struct sctp_ulpevent *event;
+ if (asoc->state < SCTP_STATE_ESTABLISHED)
+ return;
+
memset(&addr, 0, sizeof(struct sockaddr_storage));
memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
It's not easy to completely do assoc_id change/event reordering/temp check.
As:
1. sctp_assoc_add_peer() is called in quite a few places where assoc_id is
not set.
2. it's almost impossible to move SCTP_ADDR_ADDED from sctp_assoc_add_peer()
after SCTP_COMM_UP.
>
> I can make a patch with a check on temp and make COMM_UP event first.
> Currently the COMM_UP event is enqueued via commands
> while the SCTP_ADDR_ADDED event is enqueued directly.
>
> sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
> vs.
> asoc->stream.si->enqueue_event(&asoc->ulpq, event);
>
> Do you want me to change to use commands instead of enqueing?
> Or should we enqueue the COMM_UP event directly?
>
> -Jonas
On Mon, May 25, 2020 at 04:42:16PM +0800, Xin Long wrote:
> On Sat, May 23, 2020 at 8:04 PM Jonas Falkevik <[email protected]> wrote:
> >
> > On Tue, May 19, 2020 at 10:42 PM Marcelo Ricardo Leitner
> > <[email protected]> wrote:
> > >
> > > On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> > > > On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > > > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > > > > >
> > > > > > > How did you get them?
> > > > > > >
> > > > > >
> > > > > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > > > > Here a closed association is created, sctp_make_temp_assoc().
> > > > > > Which is later used when calling sctp_process_init().
> > > > > > In sctp_process_init() one of the first things are to call
> > > > > > sctp_assoc_add_peer()
> > > > > > on the closed / temp assoc.
> > > > > >
> > > > > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > > > > for the potentially new association.
> > > > >
> > > > > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > > > > for setting/getting socket options that will be used for new asocs. In
> > > > > this case, it is just a coincidence that asoc_id is not set (but
> > > > > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
> > > >
> > > > yes, you are right, I overlooked that.
> > > >
> > > > > Moreso, if I didn't
> > > > > miss anything, it would block valid events, such as those from
> > > > > sctp_sf_do_5_1D_ce
> > > > > sctp_process_init
> > > > > because sctp_process_init will only call sctp_assoc_set_id() by its
> > > > > end.
> > > >
> > > > Do we want these events at this stage?
> > > > Since the association is a newly established one, have the peer address changed?
> > > > Should we enqueue these messages with sm commands instead?
> > > > And drop them if we don't have state SCTP_STATE_ESTABLISHED?
> > > >
> > > > >
> > > > > I can't see a good reason for generating any event on temp assocs. So
> > > > > I'm thinking the checks on this patch should be on whether the asoc is
> > > > > a temporary one instead. WDYT?
> > > > >
> > > >
> > > > Agree, we shouldn't rely on coincidence.
> > > > Either check temp instead or the above mentioned state?
> > > >
> > > > > Then, considering the socket is locked, both code points should be
> > > > > allocating the IDR earlier. It's expensive, yes (point being, it could
> > > > > be avoided in case of other failures), but it should be generating
> > > > > events with the right assoc id. Are you interested in pursuing this
> > > > > fix as well?
> > > >
> > > > Sure.
> > > >
> > > > If we check temp status instead, we would need to allocate IDR earlier,
> > > > as you mention. So that we send the notification with correct assoc id.
> > > >
> > > > But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> > > > first notification event sent?
> > > > The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
> > >
> > > The RFC doesn't mention any specific ordering for them, but it would
> > > make sense. Reading the FreeBSD code now (which I consider a reference
> > > implementation), it doesn't raise these notifications from
> > > INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
> > > event is ASCONF ADD command itself. So these are extra in Linux, and
> > > I'm afraid we got to stick with them.
> > >
> > > Considering the error handling it already has, looks like the
> > > reordering is feasible and welcomed. I'm thinking the temp check and
> > > reordering is the best way forward here.
> > >
> > > Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
> > > breakage.
> >
> > Some order is mentioned in RFC 6458 Chapter 6.1.1.
> >
> > SCTP_COMM_UP: A new association is now ready, and data may be
> > exchanged with this peer. When an association has been
> > established successfully, this notification should be the
> > first one.
Oh, nice finding.
> If this is true, as SCTP_COMM_UP event is always followed by state changed
> to ESTABLISHED. So I'm thinking to NOT make addr events by checking the
> state:
>
> @@ -343,6 +343,9 @@ void sctp_ulpevent_nofity_peer_addr_change(struct
> sctp_transport *transport,
> struct sockaddr_storage addr;
> struct sctp_ulpevent *event;
>
> + if (asoc->state < SCTP_STATE_ESTABLISHED)
> + return;
> +
> memset(&addr, 0, sizeof(struct sockaddr_storage));
> memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
With the above said, yep. Thanks.
>
> It's not easy to completely do assoc_id change/event reordering/temp check.
> As:
Temp check should be fine, but agree re the others. Anyhow, the above
will be good already. :-)
>
> 1. sctp_assoc_add_peer() is called in quite a few places where assoc_id is
> not set.
> 2. it's almost impossible to move SCTP_ADDR_ADDED from sctp_assoc_add_peer()
> after SCTP_COMM_UP.
>
> >
> > I can make a patch with a check on temp and make COMM_UP event first.
> > Currently the COMM_UP event is enqueued via commands
> > while the SCTP_ADDR_ADDED event is enqueued directly.
> >
> > sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
> > vs.
> > asoc->stream.si->enqueue_event(&asoc->ulpq, event);
> >
> > Do you want me to change to use commands instead of enqueing?
> > Or should we enqueue the COMM_UP event directly?
> >
> > -Jonas
On Mon, May 25, 2020 at 9:10 PM Marcelo Ricardo Leitner
<[email protected]> wrote:
>
> On Mon, May 25, 2020 at 04:42:16PM +0800, Xin Long wrote:
> > On Sat, May 23, 2020 at 8:04 PM Jonas Falkevik <[email protected]> wrote:
> > >
> > > On Tue, May 19, 2020 at 10:42 PM Marcelo Ricardo Leitner
> > > <[email protected]> wrote:
> > > >
> > > > On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> > > > > On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > > > > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > > > > > >
> > > > > > > > How did you get them?
> > > > > > > >
> > > > > > >
> > > > > > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > > > > > Here a closed association is created, sctp_make_temp_assoc().
> > > > > > > Which is later used when calling sctp_process_init().
> > > > > > > In sctp_process_init() one of the first things are to call
> > > > > > > sctp_assoc_add_peer()
> > > > > > > on the closed / temp assoc.
> > > > > > >
> > > > > > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > > > > > for the potentially new association.
> > > > > >
> > > > > > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > > > > > for setting/getting socket options that will be used for new asocs. In
> > > > > > this case, it is just a coincidence that asoc_id is not set (but
> > > > > > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
> > > > >
> > > > > yes, you are right, I overlooked that.
> > > > >
> > > > > > Moreso, if I didn't
> > > > > > miss anything, it would block valid events, such as those from
> > > > > > sctp_sf_do_5_1D_ce
> > > > > > sctp_process_init
> > > > > > because sctp_process_init will only call sctp_assoc_set_id() by its
> > > > > > end.
> > > > >
> > > > > Do we want these events at this stage?
> > > > > Since the association is a newly established one, have the peer address changed?
> > > > > Should we enqueue these messages with sm commands instead?
> > > > > And drop them if we don't have state SCTP_STATE_ESTABLISHED?
> > > > >
> > > > > >
> > > > > > I can't see a good reason for generating any event on temp assocs. So
> > > > > > I'm thinking the checks on this patch should be on whether the asoc is
> > > > > > a temporary one instead. WDYT?
> > > > > >
> > > > >
> > > > > Agree, we shouldn't rely on coincidence.
> > > > > Either check temp instead or the above mentioned state?
> > > > >
> > > > > > Then, considering the socket is locked, both code points should be
> > > > > > allocating the IDR earlier. It's expensive, yes (point being, it could
> > > > > > be avoided in case of other failures), but it should be generating
> > > > > > events with the right assoc id. Are you interested in pursuing this
> > > > > > fix as well?
> > > > >
> > > > > Sure.
> > > > >
> > > > > If we check temp status instead, we would need to allocate IDR earlier,
> > > > > as you mention. So that we send the notification with correct assoc id.
> > > > >
> > > > > But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> > > > > first notification event sent?
> > > > > The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
> > > >
> > > > The RFC doesn't mention any specific ordering for them, but it would
> > > > make sense. Reading the FreeBSD code now (which I consider a reference
> > > > implementation), it doesn't raise these notifications from
> > > > INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
> > > > event is ASCONF ADD command itself. So these are extra in Linux, and
> > > > I'm afraid we got to stick with them.
> > > >
> > > > Considering the error handling it already has, looks like the
> > > > reordering is feasible and welcomed. I'm thinking the temp check and
> > > > reordering is the best way forward here.
> > > >
> > > > Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
> > > > breakage.
> > >
> > > Some order is mentioned in RFC 6458 Chapter 6.1.1.
> > >
> > > SCTP_COMM_UP: A new association is now ready, and data may be
> > > exchanged with this peer. When an association has been
> > > established successfully, this notification should be the
> > > first one.
>
> Oh, nice finding.
>
> > If this is true, as SCTP_COMM_UP event is always followed by state changed
> > to ESTABLISHED. So I'm thinking to NOT make addr events by checking the
> > state:
> >
> > @@ -343,6 +343,9 @@ void sctp_ulpevent_nofity_peer_addr_change(struct
> > sctp_transport *transport,
> > struct sockaddr_storage addr;
> > struct sctp_ulpevent *event;
> >
> > + if (asoc->state < SCTP_STATE_ESTABLISHED)
> > + return;
> > +
> > memset(&addr, 0, sizeof(struct sockaddr_storage));
> > memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
>
> With the above said, yep. Thanks.
>
> >
> > It's not easy to completely do assoc_id change/event reordering/temp check.
> > As:
>
> Temp check should be fine, but agree re the others. Anyhow, the above
> will be good already. :-)
Hi Jonas,
What do you think? If you agree, can you please continue to go with it
after testing?
Thanks.
>
> >
> > 1. sctp_assoc_add_peer() is called in quite a few places where assoc_id is
> > not set.
> > 2. it's almost impossible to move SCTP_ADDR_ADDED from sctp_assoc_add_peer()
> > after SCTP_COMM_UP.
> >
> > >
> > > I can make a patch with a check on temp and make COMM_UP event first.
> > > Currently the COMM_UP event is enqueued via commands
> > > while the SCTP_ADDR_ADDED event is enqueued directly.
> > >
> > > sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
> > > vs.
> > > asoc->stream.si->enqueue_event(&asoc->ulpq, event);
> > >
> > > Do you want me to change to use commands instead of enqueing?
> > > Or should we enqueue the COMM_UP event directly?
> > >
> > > -Jonas
On Mon, May 25, 2020 at 6:10 PM Xin Long <[email protected]> wrote:
>
> On Mon, May 25, 2020 at 9:10 PM Marcelo Ricardo Leitner
> <[email protected]> wrote:
> >
> > On Mon, May 25, 2020 at 04:42:16PM +0800, Xin Long wrote:
> > > On Sat, May 23, 2020 at 8:04 PM Jonas Falkevik <[email protected]> wrote:
> > > >
> > > > On Tue, May 19, 2020 at 10:42 PM Marcelo Ricardo Leitner
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> > > > > > On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > > > > > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > > > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > > > > > > >
> > > > > > > > > How did you get them?
> > > > > > > > >
> > > > > > > >
> > > > > > > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > > > > > > Here a closed association is created, sctp_make_temp_assoc().
> > > > > > > > Which is later used when calling sctp_process_init().
> > > > > > > > In sctp_process_init() one of the first things are to call
> > > > > > > > sctp_assoc_add_peer()
> > > > > > > > on the closed / temp assoc.
> > > > > > > >
> > > > > > > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > > > > > > for the potentially new association.
> > > > > > >
> > > > > > > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > > > > > > for setting/getting socket options that will be used for new asocs. In
> > > > > > > this case, it is just a coincidence that asoc_id is not set (but
> > > > > > > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
> > > > > >
> > > > > > yes, you are right, I overlooked that.
> > > > > >
> > > > > > > Moreso, if I didn't
> > > > > > > miss anything, it would block valid events, such as those from
> > > > > > > sctp_sf_do_5_1D_ce
> > > > > > > sctp_process_init
> > > > > > > because sctp_process_init will only call sctp_assoc_set_id() by its
> > > > > > > end.
> > > > > >
> > > > > > Do we want these events at this stage?
> > > > > > Since the association is a newly established one, have the peer address changed?
> > > > > > Should we enqueue these messages with sm commands instead?
> > > > > > And drop them if we don't have state SCTP_STATE_ESTABLISHED?
> > > > > >
> > > > > > >
> > > > > > > I can't see a good reason for generating any event on temp assocs. So
> > > > > > > I'm thinking the checks on this patch should be on whether the asoc is
> > > > > > > a temporary one instead. WDYT?
> > > > > > >
> > > > > >
> > > > > > Agree, we shouldn't rely on coincidence.
> > > > > > Either check temp instead or the above mentioned state?
> > > > > >
> > > > > > > Then, considering the socket is locked, both code points should be
> > > > > > > allocating the IDR earlier. It's expensive, yes (point being, it could
> > > > > > > be avoided in case of other failures), but it should be generating
> > > > > > > events with the right assoc id. Are you interested in pursuing this
> > > > > > > fix as well?
> > > > > >
> > > > > > Sure.
> > > > > >
> > > > > > If we check temp status instead, we would need to allocate IDR earlier,
> > > > > > as you mention. So that we send the notification with correct assoc id.
> > > > > >
> > > > > > But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> > > > > > first notification event sent?
> > > > > > The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
> > > > >
> > > > > The RFC doesn't mention any specific ordering for them, but it would
> > > > > make sense. Reading the FreeBSD code now (which I consider a reference
> > > > > implementation), it doesn't raise these notifications from
> > > > > INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
> > > > > event is ASCONF ADD command itself. So these are extra in Linux, and
> > > > > I'm afraid we got to stick with them.
> > > > >
> > > > > Considering the error handling it already has, looks like the
> > > > > reordering is feasible and welcomed. I'm thinking the temp check and
> > > > > reordering is the best way forward here.
> > > > >
> > > > > Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
> > > > > breakage.
> > > >
> > > > Some order is mentioned in RFC 6458 Chapter 6.1.1.
> > > >
> > > > SCTP_COMM_UP: A new association is now ready, and data may be
> > > > exchanged with this peer. When an association has been
> > > > established successfully, this notification should be the
> > > > first one.
> >
> > Oh, nice finding.
> >
> > > If this is true, as SCTP_COMM_UP event is always followed by state changed
> > > to ESTABLISHED. So I'm thinking to NOT make addr events by checking the
> > > state:
> > >
> > > @@ -343,6 +343,9 @@ void sctp_ulpevent_nofity_peer_addr_change(struct
> > > sctp_transport *transport,
> > > struct sockaddr_storage addr;
> > > struct sctp_ulpevent *event;
> > >
> > > + if (asoc->state < SCTP_STATE_ESTABLISHED)
> > > + return;
> > > +
> > > memset(&addr, 0, sizeof(struct sockaddr_storage));
> > > memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
> >
> > With the above said, yep. Thanks.
> >
> > >
> > > It's not easy to completely do assoc_id change/event reordering/temp check.
> > > As:
> >
> > Temp check should be fine, but agree re the others. Anyhow, the above
> > will be good already. :-)
> Hi Jonas,
>
> What do you think? If you agree, can you please continue to go with it
> after testing?
>
> Thanks.
>
I agree, it looks good. Looks like it will produce results similar to
the initial change.
Will test and verify as well.
Then should I submit v2 of the patch?
While at it, I have a patch renaming nofity to notify in the function
sctp_ulpevent_nofity_peer_addr_change.
Did I misunderstand the name or is it a typo? Worth submitting as well?
Thanks,
Jonas
On Mon, May 25, 2020 at 10:49:06PM +0200, Jonas Falkevik wrote:
> On Mon, May 25, 2020 at 6:10 PM Xin Long <[email protected]> wrote:
> >
> > On Mon, May 25, 2020 at 9:10 PM Marcelo Ricardo Leitner
> > <[email protected]> wrote:
> > >
> > > On Mon, May 25, 2020 at 04:42:16PM +0800, Xin Long wrote:
> > > > On Sat, May 23, 2020 at 8:04 PM Jonas Falkevik <[email protected]> wrote:
> > > > >
> > > > > On Tue, May 19, 2020 at 10:42 PM Marcelo Ricardo Leitner
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, May 15, 2020 at 10:30:29AM +0200, Jonas Falkevik wrote:
> > > > > > > On Wed, May 13, 2020 at 11:32 PM Marcelo Ricardo Leitner
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Wed, May 13, 2020 at 10:11:05PM +0200, Jonas Falkevik wrote:
> > > > > > > > > On Wed, May 13, 2020 at 6:01 PM Marcelo Ricardo Leitner
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, May 13, 2020 at 04:52:16PM +0200, Jonas Falkevik wrote:
> > > > > > > > > > > Do not generate SCTP_ADDR_{MADE_PRIM,ADDED} events for SCTP_FUTURE_ASSOC assocs.
> > > > > > > > > >
> > > > > > > > > > How did you get them?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I think one case is when receiving INIT chunk in sctp_sf_do_5_1B_init().
> > > > > > > > > Here a closed association is created, sctp_make_temp_assoc().
> > > > > > > > > Which is later used when calling sctp_process_init().
> > > > > > > > > In sctp_process_init() one of the first things are to call
> > > > > > > > > sctp_assoc_add_peer()
> > > > > > > > > on the closed / temp assoc.
> > > > > > > > >
> > > > > > > > > sctp_assoc_add_peer() are generating the SCTP_ADDR_ADDED event on the socket
> > > > > > > > > for the potentially new association.
> > > > > > > >
> > > > > > > > I see, thanks. The SCTP_FUTURE_ASSOC means something different. It is
> > > > > > > > for setting/getting socket options that will be used for new asocs. In
> > > > > > > > this case, it is just a coincidence that asoc_id is not set (but
> > > > > > > > initialized to 0) and SCTP_FUTURE_ASSOC is also 0.
> > > > > > >
> > > > > > > yes, you are right, I overlooked that.
> > > > > > >
> > > > > > > > Moreso, if I didn't
> > > > > > > > miss anything, it would block valid events, such as those from
> > > > > > > > sctp_sf_do_5_1D_ce
> > > > > > > > sctp_process_init
> > > > > > > > because sctp_process_init will only call sctp_assoc_set_id() by its
> > > > > > > > end.
> > > > > > >
> > > > > > > Do we want these events at this stage?
> > > > > > > Since the association is a newly established one, have the peer address changed?
> > > > > > > Should we enqueue these messages with sm commands instead?
> > > > > > > And drop them if we don't have state SCTP_STATE_ESTABLISHED?
> > > > > > >
> > > > > > > >
> > > > > > > > I can't see a good reason for generating any event on temp assocs. So
> > > > > > > > I'm thinking the checks on this patch should be on whether the asoc is
> > > > > > > > a temporary one instead. WDYT?
> > > > > > > >
> > > > > > >
> > > > > > > Agree, we shouldn't rely on coincidence.
> > > > > > > Either check temp instead or the above mentioned state?
> > > > > > >
> > > > > > > > Then, considering the socket is locked, both code points should be
> > > > > > > > allocating the IDR earlier. It's expensive, yes (point being, it could
> > > > > > > > be avoided in case of other failures), but it should be generating
> > > > > > > > events with the right assoc id. Are you interested in pursuing this
> > > > > > > > fix as well?
> > > > > > >
> > > > > > > Sure.
> > > > > > >
> > > > > > > If we check temp status instead, we would need to allocate IDR earlier,
> > > > > > > as you mention. So that we send the notification with correct assoc id.
> > > > > > >
> > > > > > > But shouldn't the SCTP_COMM_UP, for a newly established association, be the
> > > > > > > first notification event sent?
> > > > > > > The SCTP_COMM_UP notification is enqueued later in sctp_sf_do_5_1D_ce().
> > > > > >
> > > > > > The RFC doesn't mention any specific ordering for them, but it would
> > > > > > make sense. Reading the FreeBSD code now (which I consider a reference
> > > > > > implementation), it doesn't raise these notifications from
> > > > > > INIT_ACK/COOKIE_ECHO at all. The only trigger for SCTP_ADDR_ADDED
> > > > > > event is ASCONF ADD command itself. So these are extra in Linux, and
> > > > > > I'm afraid we got to stick with them.
> > > > > >
> > > > > > Considering the error handling it already has, looks like the
> > > > > > reordering is feasible and welcomed. I'm thinking the temp check and
> > > > > > reordering is the best way forward here.
> > > > > >
> > > > > > Thoughts? Neil? Xin? The assoc_id change might be considered an UAPI
> > > > > > breakage.
> > > > >
> > > > > Some order is mentioned in RFC 6458 Chapter 6.1.1.
> > > > >
> > > > > SCTP_COMM_UP: A new association is now ready, and data may be
> > > > > exchanged with this peer. When an association has been
> > > > > established successfully, this notification should be the
> > > > > first one.
> > >
> > > Oh, nice finding.
> > >
> > > > If this is true, as SCTP_COMM_UP event is always followed by state changed
> > > > to ESTABLISHED. So I'm thinking to NOT make addr events by checking the
> > > > state:
> > > >
> > > > @@ -343,6 +343,9 @@ void sctp_ulpevent_nofity_peer_addr_change(struct
> > > > sctp_transport *transport,
> > > > struct sockaddr_storage addr;
> > > > struct sctp_ulpevent *event;
> > > >
> > > > + if (asoc->state < SCTP_STATE_ESTABLISHED)
> > > > + return;
> > > > +
> > > > memset(&addr, 0, sizeof(struct sockaddr_storage));
> > > > memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
> > >
> > > With the above said, yep. Thanks.
> > >
> > > >
> > > > It's not easy to completely do assoc_id change/event reordering/temp check.
> > > > As:
> > >
> > > Temp check should be fine, but agree re the others. Anyhow, the above
> > > will be good already. :-)
> > Hi Jonas,
> >
> > What do you think? If you agree, can you please continue to go with it
> > after testing?
> >
> > Thanks.
> >
> I agree, it looks good. Looks like it will produce results similar to
> the initial change.
> Will test and verify as well.
> Then should I submit v2 of the patch?
Yes,
>
> While at it, I have a patch renaming nofity to notify in the function
> sctp_ulpevent_nofity_peer_addr_change.
> Did I misunderstand the name or is it a typo? Worth submitting as well?
Oops! Yes :-) (as a different patch)
Thanks,
Marcelo