[PATCH net-next 0/4] sctp: add some missing events from rfc5061

[PATCH net-next 0/4] sctp: add some missing events from rfc5061

[Xin Long]

There are 4 events defined in rfc5061 missed in linux sctp:
SCTP_ADDR_ADDED, SCTP_ADDR_REMOVED, SCTP_ADDR_MADE_PRIM and
SCTP_SEND_FAILED_EVENT.

This patchset is to add them up.

Xin Long (4):
  sctp: add SCTP_ADDR_ADDED event
  sctp: add SCTP_ADDR_REMOVED event
  sctp: add SCTP_ADDR_MADE_PRIM event
  sctp: add SCTP_SEND_FAILED_EVENT event

 include/net/sctp/ulpevent.h | 16 +++++++------
 include/uapi/linux/sctp.h   | 16 ++++++++++++-
 net/sctp/associola.c        | 22 +++++++----------
 net/sctp/chunk.c            | 40 +++++++++++++++----------------
 net/sctp/ulpevent.c         | 57 ++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 108 insertions(+), 43 deletions(-)

-- 
2.1.0

[PATCH net-next 1/4] sctp: add SCTP_ADDR_ADDED event

[Xin Long]

A helper sctp_ulpevent_nofity_peer_addr_change() will be extracted
to make peer_addr_change event and enqueue it, and the helper will
be called in sctp_assoc_add_peer() to send SCTP_ADDR_ADDED event.

This event is described in rfc6458#section-6.1.2:

  SCTP_ADDR_ADDED:  The address is now part of the association.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/ulpevent.h |  9 ++-------
 net/sctp/associola.c        | 19 ++++++-------------
 net/sctp/ulpevent.c         | 18 +++++++++++++++++-
 3 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/include/net/sctp/ulpevent.h b/include/net/sctp/ulpevent.h
index e1a92c4..e6ead1e 100644
--- a/include/net/sctp/ulpevent.h
+++ b/include/net/sctp/ulpevent.h
@@ -80,13 +80,8 @@ struct sctp_ulpevent *sctp_ulpevent_make_assoc_change(
 	struct sctp_chunk *chunk,
 	gfp_t gfp);
 
-struct sctp_ulpevent *sctp_ulpevent_make_peer_addr_change(
-	const struct sctp_association *asoc,
-	const struct sockaddr_storage *aaddr,
-	int flags,
-	int state,
-	int error,
-	gfp_t gfp);
+void sctp_ulpevent_nofity_peer_addr_change(struct sctp_transport *transport,
+					   int state, int error);
 
 struct sctp_ulpevent *sctp_ulpevent_make_remote_error(
 	const struct sctp_association *asoc,
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index d2ffc9a..55aad70 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -707,6 +707,8 @@ 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 we do not yet have a primary path, set one.  */
 	if (!asoc->peer.primary_path) {
 		sctp_assoc_set_primary(asoc, peer);
@@ -781,10 +783,8 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 				  enum sctp_transport_cmd command,
 				  sctp_sn_error_t error)
 {
-	struct sctp_ulpevent *event;
-	struct sockaddr_storage addr;
-	int spc_state = 0;
 	bool ulp_notify = true;
+	int spc_state = 0;
 
 	/* Record the transition on the transport.  */
 	switch (command) {
@@ -836,16 +836,9 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 	/* Generate and send a SCTP_PEER_ADDR_CHANGE notification
 	 * to the user.
 	 */
-	if (ulp_notify) {
-		memset(&addr, 0, sizeof(struct sockaddr_storage));
-		memcpy(&addr, &transport->ipaddr,
-		       transport->af_specific->sockaddr_len);
-
-		event = sctp_ulpevent_make_peer_addr_change(asoc, &addr,
-					0, spc_state, error, GFP_ATOMIC);
-		if (event)
-			asoc->stream.si->enqueue_event(&asoc->ulpq, event);
-	}
+	if (ulp_notify)
+		sctp_ulpevent_nofity_peer_addr_change(transport,
+						      spc_state, error);
 
 	/* Select new active and retran paths. */
 	sctp_select_active_and_retran_path(asoc);
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index e0cc1ed..f07b986 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -238,7 +238,7 @@ struct sctp_ulpevent  *sctp_ulpevent_make_assoc_change(
  * When a destination address on a multi-homed peer encounters a change
  * an interface details event is sent.
  */
-struct sctp_ulpevent *sctp_ulpevent_make_peer_addr_change(
+static struct sctp_ulpevent *sctp_ulpevent_make_peer_addr_change(
 	const struct sctp_association *asoc,
 	const struct sockaddr_storage *aaddr,
 	int flags, int state, int error, gfp_t gfp)
@@ -336,6 +336,22 @@ struct sctp_ulpevent *sctp_ulpevent_make_peer_addr_change(
 	return NULL;
 }
 
+void sctp_ulpevent_nofity_peer_addr_change(struct sctp_transport *transport,
+					   int state, int error)
+{
+	struct sctp_association *asoc = transport->asoc;
+	struct sockaddr_storage addr;
+	struct sctp_ulpevent *event;
+
+	memset(&addr, 0, sizeof(struct sockaddr_storage));
+	memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
+
+	event = sctp_ulpevent_make_peer_addr_change(asoc, &addr, 0, state,
+						    error, GFP_ATOMIC);
+	if (event)
+		asoc->stream.si->enqueue_event(&asoc->ulpq, event);
+}
+
 /* Create and initialize an SCTP_REMOTE_ERROR notification.
  *
  * Note: This assumes that the chunk->skb->data already points to the
-- 
2.1.0

[PATCH net-next 2/4] sctp: add SCTP_ADDR_REMOVED event

[Xin Long]

sctp_ulpevent_nofity_peer_addr_change() is called in
sctp_assoc_rm_peer() to send SCTP_ADDR_REMOVED event
when this transport is removed from the asoc.

This event is described in rfc6458#section-6.1.2:

  SCTP_ADDR_REMOVED:  The address is no longer part of the
     association.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/associola.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 55aad70..0d3d7ce 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -569,6 +569,7 @@ void sctp_assoc_rm_peer(struct sctp_association *asoc,
 
 	asoc->peer.transport_count--;
 
+	sctp_ulpevent_nofity_peer_addr_change(peer, SCTP_ADDR_REMOVED, 0);
 	sctp_transport_free(peer);
 }
 
-- 
2.1.0

[PATCH net-next 3/4] sctp: add SCTP_ADDR_MADE_PRIM event

[Xin Long]

sctp_ulpevent_nofity_peer_addr_change() would be called in
sctp_assoc_set_primary() to send SCTP_ADDR_MADE_PRIM event
when this transport is set to the primary path of the asoc.

This event is described in rfc6458#section-6.1.2:

  SCTP_ADDR_MADE_PRIM:  This address has now been made the primary
     destination address.  This notification is provided whenever an
     address is made primary.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/associola.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 0d3d7ce..1ba893b 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -429,6 +429,8 @@ 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);
 
 	/* Set a default msg_name for events. */
 	memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
-- 
2.1.0

[PATCH net-next 4/4] sctp: add SCTP_SEND_FAILED_EVENT event

[Xin Long]

This patch is to add a new event SCTP_SEND_FAILED_EVENT described in
rfc6458#section-6.1.11. It's a update of SCTP_SEND_FAILED event:

  struct sctp_sndrcvinfo ssf_info is replaced with
  struct sctp_sndinfo ssfe_info in struct sctp_send_failed_event.

SCTP_SEND_FAILED is being deprecated, but we don't remove it in this
patch. Both are being processed in sctp_datamsg_destroy() when the
corresp event flag is set.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/ulpevent.h |  7 +++++++
 include/uapi/linux/sctp.h   | 16 +++++++++++++++-
 net/sctp/chunk.c            | 40 +++++++++++++++++++---------------------
 net/sctp/ulpevent.c         | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 22 deletions(-)

diff --git a/include/net/sctp/ulpevent.h b/include/net/sctp/ulpevent.h
index e6ead1e..0b032b9 100644
--- a/include/net/sctp/ulpevent.h
+++ b/include/net/sctp/ulpevent.h
@@ -95,6 +95,13 @@ struct sctp_ulpevent *sctp_ulpevent_make_send_failed(
 	__u32 error,
 	gfp_t gfp);
 
+struct sctp_ulpevent *sctp_ulpevent_make_send_failed_event(
+	const struct sctp_association *asoc,
+	struct sctp_chunk *chunk,
+	__u16 flags,
+	__u32 error,
+	gfp_t gfp);
+
 struct sctp_ulpevent *sctp_ulpevent_make_shutdown_event(
 	const struct sctp_association *asoc,
 	__u16 flags,
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index 6d5b164..6bce7f9 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -449,6 +449,16 @@ struct sctp_send_failed {
 	__u8 ssf_data[0];
 };
 
+struct sctp_send_failed_event {
+	__u16 ssf_type;
+	__u16 ssf_flags;
+	__u32 ssf_length;
+	__u32 ssf_error;
+	struct sctp_sndinfo ssfe_info;
+	sctp_assoc_t ssf_assoc_id;
+	__u8 ssf_data[0];
+};
+
 /*
  *   ssf_flags: 16 bits (unsigned integer)
  *
@@ -605,6 +615,7 @@ struct sctp_event_subscribe {
 	__u8 sctp_stream_reset_event;
 	__u8 sctp_assoc_reset_event;
 	__u8 sctp_stream_change_event;
+	__u8 sctp_send_failure_event_event;
 };
 
 /*
@@ -632,6 +643,7 @@ union sctp_notification {
 	struct sctp_stream_reset_event sn_strreset_event;
 	struct sctp_assoc_reset_event sn_assocreset_event;
 	struct sctp_stream_change_event sn_strchange_event;
+	struct sctp_send_failed_event sn_send_failed_event;
 };
 
 /* Section 5.3.1
@@ -667,7 +679,9 @@ enum sctp_sn_type {
 #define SCTP_ASSOC_RESET_EVENT		SCTP_ASSOC_RESET_EVENT
 	SCTP_STREAM_CHANGE_EVENT,
 #define SCTP_STREAM_CHANGE_EVENT	SCTP_STREAM_CHANGE_EVENT
-	SCTP_SN_TYPE_MAX	= SCTP_STREAM_CHANGE_EVENT,
+	SCTP_SEND_FAILED_EVENT,
+#define SCTP_SEND_FAILED_EVENT		SCTP_SEND_FAILED_EVENT
+	SCTP_SN_TYPE_MAX	= SCTP_SEND_FAILED_EVENT,
 #define SCTP_SN_TYPE_MAX		SCTP_SN_TYPE_MAX
 };
 
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index cc0405c..cc3ce5d 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -75,41 +75,39 @@ static void sctp_datamsg_destroy(struct sctp_datamsg *msg)
 	struct list_head *pos, *temp;
 	struct sctp_chunk *chunk;
 	struct sctp_ulpevent *ev;
-	int error = 0, notify;
-
-	/* If we failed, we may need to notify. */
-	notify = msg->send_failed ? -1 : 0;
+	int error, sent;
 
 	/* Release all references. */
 	list_for_each_safe(pos, temp, &msg->chunks) {
 		list_del_init(pos);
 		chunk = list_entry(pos, struct sctp_chunk, frag_list);
-		/* Check whether we _really_ need to notify. */
-		if (notify < 0) {
-			asoc = chunk->asoc;
-			if (msg->send_error)
-				error = msg->send_error;
-			else
-				error = asoc->outqueue.error;
-
-			notify = sctp_ulpevent_type_enabled(asoc->subscribe,
-							    SCTP_SEND_FAILED);
+
+		if (!msg->send_failed) {
+			sctp_chunk_put(chunk);
+			continue;
 		}
 
-		/* Generate a SEND FAILED event only if enabled. */
-		if (notify > 0) {
-			int sent;
-			if (chunk->has_tsn)
-				sent = SCTP_DATA_SENT;
-			else
-				sent = SCTP_DATA_UNSENT;
+		asoc = chunk->asoc;
+		error = msg->send_error ?: asoc->outqueue.error;
+		sent = chunk->has_tsn ? SCTP_DATA_SENT : SCTP_DATA_UNSENT;
 
+		if (sctp_ulpevent_type_enabled(asoc->subscribe,
+					       SCTP_SEND_FAILED)) {
 			ev = sctp_ulpevent_make_send_failed(asoc, chunk, sent,
 							    error, GFP_ATOMIC);
 			if (ev)
 				asoc->stream.si->enqueue_event(&asoc->ulpq, ev);
 		}
 
+		if (sctp_ulpevent_type_enabled(asoc->subscribe,
+					       SCTP_SEND_FAILED_EVENT)) {
+			ev = sctp_ulpevent_make_send_failed_event(asoc, chunk,
+								  sent, error,
+								  GFP_ATOMIC);
+			if (ev)
+				asoc->stream.si->enqueue_event(&asoc->ulpq, ev);
+		}
+
 		sctp_chunk_put(chunk);
 	}
 
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index f07b986..c82dbdc 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -527,6 +527,45 @@ struct sctp_ulpevent *sctp_ulpevent_make_send_failed(
 	return NULL;
 }
 
+struct sctp_ulpevent *sctp_ulpevent_make_send_failed_event(
+	const struct sctp_association *asoc, struct sctp_chunk *chunk,
+	__u16 flags, __u32 error, gfp_t gfp)
+{
+	struct sctp_send_failed_event *ssf;
+	struct sctp_ulpevent *event;
+	struct sk_buff *skb;
+	int len;
+
+	skb = skb_copy_expand(chunk->skb, sizeof(*ssf), 0, gfp);
+	if (!skb)
+		return NULL;
+
+	len = ntohs(chunk->chunk_hdr->length);
+	len -= sctp_datachk_len(&asoc->stream);
+
+	skb_pull(skb, sctp_datachk_len(&asoc->stream));
+	event = sctp_skb2event(skb);
+	sctp_ulpevent_init(event, MSG_NOTIFICATION, skb->truesize);
+
+	ssf = skb_push(skb, sizeof(*ssf));
+	ssf->ssf_type = SCTP_SEND_FAILED_EVENT;
+	ssf->ssf_flags = flags;
+	ssf->ssf_length = sizeof(*ssf) + len;
+	skb_trim(skb, ssf->ssf_length);
+	ssf->ssf_error = error;
+
+	ssf->ssfe_info.snd_sid = chunk->sinfo.sinfo_stream;
+	ssf->ssfe_info.snd_ppid = chunk->sinfo.sinfo_ppid;
+	ssf->ssfe_info.snd_context = chunk->sinfo.sinfo_context;
+	ssf->ssfe_info.snd_assoc_id = chunk->sinfo.sinfo_assoc_id;
+	ssf->ssfe_info.snd_flags = chunk->chunk_hdr->flags;
+
+	sctp_ulpevent_set_owner(event, asoc);
+	ssf->ssf_assoc_id = sctp_assoc2id(asoc);
+
+	return event;
+}
+
 /* Create and initialize a SCTP_SHUTDOWN_EVENT notification.
  *
  * Socket Extensions for SCTP - draft-01
-- 
2.1.0

Re: [PATCH net-next 0/4] sctp: add some missing events from rfc5061

[Neil Horman]

On Tue, Oct 08, 2019 at 07:27:32PM +0800, Xin Long wrote:
> There are 4 events defined in rfc5061 missed in linux sctp:
> SCTP_ADDR_ADDED, SCTP_ADDR_REMOVED, SCTP_ADDR_MADE_PRIM and
> SCTP_SEND_FAILED_EVENT.
> 
> This patchset is to add them up.
> 
> Xin Long (4):
>   sctp: add SCTP_ADDR_ADDED event
>   sctp: add SCTP_ADDR_REMOVED event
>   sctp: add SCTP_ADDR_MADE_PRIM event
>   sctp: add SCTP_SEND_FAILED_EVENT event
> 
>  include/net/sctp/ulpevent.h | 16 +++++++------
>  include/uapi/linux/sctp.h   | 16 ++++++++++++-
>  net/sctp/associola.c        | 22 +++++++----------
>  net/sctp/chunk.c            | 40 +++++++++++++++----------------
>  net/sctp/ulpevent.c         | 57 ++++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 108 insertions(+), 43 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 0/4] sctp: add some missing events from rfc5061

[Jakub Kicinski]

On Tue,  8 Oct 2019 19:27:32 +0800, Xin Long wrote:
> There are 4 events defined in rfc5061 missed in linux sctp:
> SCTP_ADDR_ADDED, SCTP_ADDR_REMOVED, SCTP_ADDR_MADE_PRIM and
> SCTP_SEND_FAILED_EVENT.
> 
> This patchset is to add them up.

Applied, thanks.

ABI breakage in sctp_event_subscribe (was [PATCH net-next 0/4] sctp: add some missing events from rfc5061)

[Harald Welte]

Dear Linux SCTP developers,

this patchset (merged back in Q4/2019) has broken ABI compatibility, more
or less exactly as it was discussed/predicted in Message-Id
<20190206201430.18830-1-julien@arista.com>
"[PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length"
on this very list in February 2019.

The process to reproduce this is quite simple:
* upgrade your kernel / uapi headers to a later version (happens
  automatically on most distributions as linux-libc-dev is upgraded)
* rebuild any application using SCTP_EVENTS which was working perfectly
  fine before
* fail to execute on any older kernels

This can be a severe issue in production systems where you may not
upgrade the kernel until/unless a severe security issue actually makes
you do so.

Those steps above can very well happen on different machines, i.e. your
build server having a more recent linux-libc-dev package (and hence
linux/sctp.h) than some of the users in the field are running kernels.

I think this is a severe problem that affects portability of binaries
between differnt Linux versions and hence the kind of ABI breakage that
the kernel exactly doesn't want to have.

The point here is that there is no check if any of those newly-added
events at the end are actually used.  I can accept that programs using
those new options will not run on older kernels - obviously.  But old
programs that have no interest in new events being added should run just
fine, even if rebuilt against modern headers.

In the kernel setsockopt handling coee: Why not simply check if any of
the newly-added events are actually set to non-zero?  If those are all
zero, we can assume that the code doesn't use them.

Yes, for all the existing kernels out there it's too late as they simply
only have the size based check.  But I'm worried history will repeat
itself...

Thanks for your consideration.

-- 
- Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Re: ABI breakage in sctp_event_subscribe (was [PATCH net-next 0/4] sctp: add some missing events from rfc5061)

[Harald Welte]

Dear Linux SCTP developers,

On Sun, Apr 19, 2020 at 12:25:36PM +0200, Harald Welte wrote:
> this patchset (merged back in Q4/2019) has broken ABI compatibility, more
> or less exactly as it was discussed/predicted in Message-Id
> <20190206201430.18830-1-julien@arista.com>
> "[PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length"
> on this very list in February 2019.

does the lack of any follow-up so far seems to indicate nobody considers
this a problem?  Even without any feedback from the Linux kernel
developers, I would be curious to hear What do other SCTP users say.

So far I have a somewhat difficult time understanding that I would be
the only one worried about ABI breakage?  If that's the case, I guess
it would be best to get the word out that people using Linux SCTP should
better make sure to not use binary packages but always build on the
system they run it on, to ensure kernel headers are identical.

I don't mean this in any cynical way.  The point is either the ABI is
stable and people can move binaries between different OS/kernel
versions, or they cannot.  So far the general assumption on Linux is you
can, but with SCTP you can not, so this needs to be clarified.

Regards,
	Harald
-- 
- Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Re: ABI breakage in sctp_event_subscribe (was [PATCH net-next 0/4] sctp: add some missing events from rfc5061)

[Marcelo Ricardo Leitner]

On Fri, May 01, 2020 at 03:16:07PM +0200, Harald Welte wrote:
> Dear Linux SCTP developers,
> 
> On Sun, Apr 19, 2020 at 12:25:36PM +0200, Harald Welte wrote:
> > this patchset (merged back in Q4/2019) has broken ABI compatibility, more
> > or less exactly as it was discussed/predicted in Message-Id
> > <20190206201430.18830-1-julien@arista.com>
> > "[PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length"
> > on this very list in February 2019.
> 
> does the lack of any follow-up so far seems to indicate nobody considers
> this a problem?  Even without any feedback from the Linux kernel
> developers, I would be curious to hear What do other SCTP users say.

No. Speaking for myself only, I just didn't have the time to check
your report yet. I'm a developer but it's not on my main priorities.

> 
> So far I have a somewhat difficult time understanding that I would be
> the only one worried about ABI breakage?  If that's the case, I guess
> it would be best to get the word out that people using Linux SCTP should
> better make sure to not use binary packages but always build on the
> system they run it on, to ensure kernel headers are identical.
> 
> I don't mean this in any cynical way.  The point is either the ABI is
> stable and people can move binaries between different OS/kernel
> versions, or they cannot.  So far the general assumption on Linux is you
> can, but with SCTP you can not, so this needs to be clarified.

That's what we want as well. Some breakage happened, yes, by mistake,
and fixing that properly now, without breaking anything else, may be
just impossible, unfortunatelly. But you can be sure that we are
engaged on not doing it again.

Thanks,
Marcelo

Re: ABI breakage in sctp_event_subscribe (was [PATCH net-next 0/4] sctp: add some missing events from rfc5061)

[Harald Welte]

Dear SCTP developers,

I have to get back to this bug.  It is slowly turning into a nightmare.

Not only affected it forwards/backwards compatibility of application binaries
during upgrades of a distribution, but it also affects the ability to run
containerized workloads with SCTP.  It's sort-of obvious but I didn't
realize it until now.

We are observing this problem now when we operate CentOS 8 based containers
on a Debian 9 based (docker) host.  Apparently the CentOS userland has a different
definition of the event structure (larger) than the Debian kernel has (smaller) -> boom.

From my point of view, this bug is making it virtually impossible to run
containerized telecom workloads.  I guess most users are very
conservative and still running rather ancient kernels and/or
distributions, but as soon as they start upgrading their kernel to
anything that includes that patch to the SCTP events structure, the
nightmare starts.

To my knowledge, there is no infrastructure at all for a situation like this - neither
in the Docker universe nor in k8s..  You cannot build separate container
images depending on what the host OS/kernel is going to be.

And particularly, if you are not self-hosting your container runtimes
but running your containers on some kind of cloud infrastructure
provider, you have no control over what exact kernel version might be in
use there - and it also may change at any time at the discretion of the
cloud service provider.

On Fri, May 01, 2020 at 11:20:08AM -0300, Marcelo Ricardo Leitner wrote:
> That's what we want as well. Some breakage happened, yes, by mistake,
> and fixing that properly now, without breaking anything else, may be
> just impossible, unfortunatelly. But you can be sure that we are
> engaged on not doing it again.

I would actually seriously consider to roll that change back - not only
in the next kernel release but also in all stable kernel releases.  At least
the breakage then is constrained to a limited set of kernel versions.

Alternatively, I suggest to at least apply a patch to all supported
stable kernel series (picked up hopefully distributions) that makes those
older kernels accept a larger-length sctp_event_subscribe structure from
userspace, *if* any of the additional members are 0 (memcmp the
difference between old and new).

Regards,
	Harald
-- 
- Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Re: ABI breakage in sctp_event_subscribe (was [PATCH net-next 0/4] sctp: add some missing events from rfc5061)

[Neil Horman]

On Sun, Apr 19, 2020 at 12:25:36PM +0200, Harald Welte wrote:
> Dear Linux SCTP developers,
> 
> this patchset (merged back in Q4/2019) has broken ABI compatibility, more
> or less exactly as it was discussed/predicted in Message-Id
> <20190206201430.18830-1-julien@arista.com>
> "[PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length"
> on this very list in February 2019.
> 
> The process to reproduce this is quite simple:
> * upgrade your kernel / uapi headers to a later version (happens
>   automatically on most distributions as linux-libc-dev is upgraded)
> * rebuild any application using SCTP_EVENTS which was working perfectly
>   fine before
> * fail to execute on any older kernels
> 
> This can be a severe issue in production systems where you may not
> upgrade the kernel until/unless a severe security issue actually makes
> you do so.
> 
> Those steps above can very well happen on different machines, i.e. your
> build server having a more recent linux-libc-dev package (and hence
> linux/sctp.h) than some of the users in the field are running kernels.
> 
> I think this is a severe problem that affects portability of binaries
> between differnt Linux versions and hence the kind of ABI breakage that
> the kernel exactly doesn't want to have.
> 
> The point here is that there is no check if any of those newly-added
> events at the end are actually used.  I can accept that programs using
> those new options will not run on older kernels - obviously.  But old
> programs that have no interest in new events being added should run just
> fine, even if rebuilt against modern headers.
> 
> In the kernel setsockopt handling coee: Why not simply check if any of
> the newly-added events are actually set to non-zero?  If those are all
> zero, we can assume that the code doesn't use them.
> 
> Yes, for all the existing kernels out there it's too late as they simply
> only have the size based check.  But I'm worried history will repeat
> itself...
> 
> Thanks for your consideration.
> 
As Marcello noted, I don't think theres anything we can do here.  We screwed
this up, but reverting the change is just going to create a second breakage
point through which users are going to have to deal with this.  The best way
forward is to document the need to run a newer userspace uapi header set on a
correspondingly new kernel, and be sure in the future that any extension to the
uapi structures are codified as separate structures with separate socket options
(or some simmilar approach to allow for fixed size api structures)

Neil

> -- 
> - Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
>                                                   (ETSI EN 300 175-7 Ch. A6)
>