sctp: support sctp_diag in kernel

PATCH Superseded Xin Long 4 participants 22 messages Apr 9, 2016 – Apr 11, 2016

Subsystems (4)

Networking Network drivers TCP SCTP protocol

lore

Share & tools

lore

Xin Long

author maintainer

2016-04-09 04:53 UTC #0 lore raw

v2 cover sctp: support sctp_diag in kernel

This patchset will add sctp_diag module to implement diag interface on sctp in kernel.

For a listening sctp endpoint, we will just dump it's ep info. For a sctp connection, we will the assoc info and it's ep info.

The ss dump will looks like:

[iproute2]# ./misc/ss --sctp -n -l

State      Recv-Q Send-Q   Local Address:Port       Peer Address:Port
LISTEN     0      128      172.16.254.254:8888      *:*
LISTEN     0      5        127.0.0.1:1234           *:*
LISTEN     0      5        127.0.0.1:1234           *:*
  - ESTAB  0      0        127.0.0.1%lo:1234        127.0.0.1:4321
LISTEN     0      128      172.16.254.254:8888      *:*
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.253.253:8888
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.1.1:8888
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.1.2:8888
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.2.1:8888
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.2.2:8888
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.3.1:8888
  - ESTAB  0      0        172.16.254.254%eth1:8888 172.16.3.2:8888
LISTEN     0      0        127.0.0.1:4321           *:*
  - ESTAB  0      0        127.0.0.1%lo:4321        127.0.0.1:1234

The entries with '- ESTAB' are the assocs, some of them may belong to the same endpoint. So we will dump the parent endpoint first, like the entry with 'LISTEN'. then dump the assocs. ep and assocs entries will be dumped in right order so that ss can show them in tree format easily.

Besides, this patchset also simplifies sctp proc codes, cause it has some similar codes with sctp diag in sctp transport traversal.

v1->v2:
1. inet_diag_get_handler needs to return it as const.
2. merge 5/7 into 2/7 of v1.

Xin Long (6):
sctp: add sctp_info dump api for sctp_diag
sctp: export some apis or variables for sctp_diag and reuse some for
proc
sctp: export some functions for sctp_diag in inet_diag
sctp: add the sctp_diag.c file
sctp: merge the seq_start/next/exits in remaddrs and assocs
sctp: fix some rhashtable functions using in sctp proc/diag

 include/linux/sctp.h           |  65 +++++
 include/net/sctp/sctp.h        |  16 ++
 include/uapi/linux/inet_diag.h |   2 +
 net/ipv4/inet_diag.c           |   9 +-
 net/sctp/Kconfig               |   4 +
 net/sctp/Makefile              |   1 +
 net/sctp/proc.c                | 104 ++------
 net/sctp/sctp_diag.c           | 581 +++++++++++++++++++++++++++++++++++++++++
 net/sctp/socket.c              | 215 +++++++++++++++
 9 files changed, 911 insertions(+), 86 deletions(-)
 create mode 100644 net/sctp/sctp_diag.c

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-04-09 04:53 UTC #1 lore raw

PATCH v2 1/6 sctp: add sctp_info dump api for sctp_diag

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

Signed-off-by Xin Long <[email protected]>

---
 include/linux/sctp.h    | 65 +++++++++++++++++++++++++++++++++++++
 include/net/sctp/sctp.h |  3 ++
 net/sctp/socket.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)

include/linux/sctp.h +65 -0

index a9414fd..a448ebc 100644
@@ -705,4 +705,69 @@ typedef struct sctp_auth_chunk {
 	sctp_authhdr_t auth_hdr;
 } __packed sctp_auth_chunk_t;
 
+struct sctp_info {
+	__u32	sctpi_tag;
+	__u32	sctpi_state;
+	__u32	sctpi_rwnd;
+	__u16	sctpi_unackdata;
+	__u16	sctpi_penddata;
+	__u16	sctpi_instrms;
+	__u16	sctpi_outstrms;
+	__u32	sctpi_fragmentation_point;
+	__u32	sctpi_inqueue;
+	__u32	sctpi_outqueue;
+	__u32	sctpi_overall_error;
+	__u32	sctpi_max_burst;
+	__u32	sctpi_maxseg;
+	__u32	sctpi_peer_rwnd;
+	__u32	sctpi_peer_tag;
+	__u8	sctpi_peer_capable;
+	__u8	sctpi_peer_sack;
+
+	/* assoc status info */
+	__u64	sctpi_isacks;
+	__u64	sctpi_osacks;
+	__u64	sctpi_opackets;
+	__u64	sctpi_ipackets;
+	__u64	sctpi_rtxchunks;
+	__u64	sctpi_outofseqtsns;
+	__u64	sctpi_idupchunks;
+	__u64	sctpi_gapcnt;
+	__u64	sctpi_ouodchunks;
+	__u64	sctpi_iuodchunks;
+	__u64	sctpi_oodchunks;
+	__u64	sctpi_iodchunks;
+	__u64	sctpi_octrlchunks;
+	__u64	sctpi_ictrlchunks;
+
+	/* primary transport info */
+	struct sockaddr_storage	sctpi_p_address;
+	__s32	sctpi_p_state;
+	__u32	sctpi_p_cwnd;
+	__u32	sctpi_p_srtt;
+	__u32	sctpi_p_rto;
+	__u32	sctpi_p_hbinterval;
+	__u32	sctpi_p_pathmaxrxt;
+	__u32	sctpi_p_sackdelay;
+	__u32	sctpi_p_sackfreq;
+	__u32	sctpi_p_ssthresh;
+	__u32	sctpi_p_partial_bytes_acked;
+	__u32	sctpi_p_flight_size;
+	__u16	sctpi_p_error;
+
+	/* sctp sock info */
+	__u32	sctpi_s_autoclose;
+	__u32	sctpi_s_adaptation_ind;
+	__u32	sctpi_s_pd_point;
+	__u8	sctpi_s_nodelay;
+	__u8	sctpi_s_disable_fragments;
+	__u8	sctpi_s_v4mapped;
+	__u8	sctpi_s_frag_interleave;
+};
+
+struct sctp_infox {
+	struct sctp_info *sctpinfo;
+	struct sctp_association *asoc;
+};
+
 #endif /* __LINUX_SCTP_H__ */

include/net/sctp/sctp.h +3 -0

index 65521cf..36e1eae 100644
@@ -116,6 +116,9 @@ extern struct percpu_counter sctp_sockets_allocated;
 int sctp_asconf_mgmt(struct sctp_sock *, struct sctp_sockaddr_entry *);
 struct sk_buff *sctp_skb_recv_datagram(struct sock *, int, int, int *);
 
+int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
+		       struct sctp_info *info);
+
 /*
  * sctp/primitive.c
  */

net/sctp/socket.c +86 -0

92 lines not on this page · raw message

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-04-09 04:53 UTC #2 lore raw

PATCH v2 2/6 sctp: export some apis or variables for sctp_diag and reuse some for proc

For some main variables in sctp.ko, we couldn't export it to other modules, so we have to define some api to access them.

It will include sctp transport and endpoint's traversal.

There are some transport traversal functions for sctp_diag, we can also use it for sctp_proc. cause they have the similar situation to traversal transport.

Signed-off-by Xin Long <[email protected]>

---
 include/net/sctp/sctp.h |  13 +++++
 net/sctp/proc.c         |  80 +++++++------------------------
 net/sctp/socket.c       | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+), 62 deletions(-)

include/net/sctp/sctp.h +13 -0

index 36e1eae..c0c4deb 100644
@@ -116,6 +116,19 @@ extern struct percpu_counter sctp_sockets_allocated;
 int sctp_asconf_mgmt(struct sctp_sock *, struct sctp_sockaddr_entry *);
 struct sk_buff *sctp_skb_recv_datagram(struct sock *, int, int, int *);
 
+int sctp_transport_walk_start(struct rhashtable_iter *iter);
+void sctp_transport_walk_stop(struct rhashtable_iter *iter);
+struct sctp_transport *sctp_transport_get_next(struct net *net,
+			struct rhashtable_iter *iter);
+struct sctp_transport *sctp_transport_get_idx(struct net *net,
+			struct rhashtable_iter *iter, int pos);
+int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),
+				  struct net *net,
+				  const union sctp_addr *laddr,
+				  const union sctp_addr *paddr, void *p);
+int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
+			    struct net *net, int pos, void *p);
+int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *), void *p);
 int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
 		       struct sctp_info *info);

net/sctp/proc.c +18 ~~-62~~

122 lines not on this page · raw message

net/sctp/socket.c +124 -0

130 lines not on this page · raw message

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-04-09 04:53 UTC #3 lore raw

PATCH v2 3/6 sctp: export some functions for sctp_diag in inet_diag

inet_diag_msg_common_fill is used to fill the diag msg common info, we need to use it in sctp_diag as well, so export it.

We also add inet_diag_get_handler() to access inet_diag_table in sctp diag.

Signed-off-by Xin Long <[email protected]>

---
 net/ipv4/inet_diag.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

net/ipv4/inet_diag.c +8 -1

index bd591eb..5a0bfe0 100644
@@ -66,7 +66,13 @@ static void inet_diag_unlock_handler(const struct inet_diag_handler *handler)
 	mutex_unlock(&inet_diag_table_mutex);
 }
 
-static void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
+const struct inet_diag_handler *inet_diag_get_handler(int proto)
+{
+	return inet_diag_table[proto];
+}
+EXPORT_SYMBOL_GPL(inet_diag_get_handler);
+
+void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
 {
 	r->idiag_family = sk->sk_family;

@@ -89,6 +95,7 @@ static void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
 	r->id.idiag_dst[0] = sk->sk_daddr;
 	}
 }
+EXPORT_SYMBOL_GPL(inet_diag_msg_common_fill);
 
 static size_t inet_sk_attr_size(void)
 {

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-04-09 04:53 UTC #4 lore raw

PATCH v2 4/6 sctp: add the sctp_diag.c file

This one will implement all the interface of inet_diag, inet_diag_handler. which includes sctp_diag_dump, sctp_diag_dump_one and sctp_diag_get_info.

It will work as a modules, and register inet_diag_handler when loading.

Signed-off-by Xin Long <[email protected]>

---
 include/uapi/linux/inet_diag.h |   2 +
 net/sctp/Kconfig               |   4 +
 net/sctp/Makefile              |   1 +
 net/sctp/sctp_diag.c           | 581 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 588 insertions(+)
 create mode 100644 net/sctp/sctp_diag.c

include/uapi/linux/inet_diag.h +2 -0

index 68a1f71..f5f3629 100644
@@ -113,6 +113,8 @@ enum {
 	INET_DIAG_DCTCPINFO,
 	INET_DIAG_PROTOCOL,  /* response attribute only */
 	INET_DIAG_SKV6ONLY,
+	INET_DIAG_LOCALS,
+	INET_DIAG_PEERS,
 };
 
 #define INET_DIAG_MAX INET_DIAG_SKV6ONLY

net/sctp/Kconfig +4 -0

index 71c1a59..d9c04dc 100644
@@ -99,5 +99,9 @@ config SCTP_COOKIE_HMAC_SHA1
 	select CRYPTO_HMAC if SCTP_COOKIE_HMAC_SHA1
 	select CRYPTO_SHA1 if SCTP_COOKIE_HMAC_SHA1
 
+config INET_SCTP_DIAG
+	depends on INET_DIAG
+	def_tristate INET_DIAG
+
 
 endif # IP_SCTP

net/sctp/Makefile +1 -0

index 3b4ffb0..0fca582 100644
@@ -4,6 +4,7 @@
 
 obj-$(CONFIG_IP_SCTP) += sctp.o
 obj-$(CONFIG_NET_SCTPPROBE) += sctp_probe.o
+obj-$(CONFIG_INET_SCTP_DIAG) += sctp_diag.o
 
 sctp-y := sm_statetable.o sm_statefuns.o sm_sideeffect.o \
 	  protocol.o endpointola.o associola.o \

net/sctp/sctp_diag.c new file +579 -0

579 lines not on this page · raw message

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-04-09 04:53 UTC #5 lore raw

PATCH v2 5/6 sctp: merge the seq_start/next/exits in remaddrs and assocs

In sctp proc, these three functions in remaddrs and assocs are the same. we should merge them into one.

Signed-off-by Xin Long <[email protected]>

---
 net/sctp/proc.c | 45 +++++++++------------------------------------
 1 file changed, 9 insertions(+), 36 deletions(-)

net/sctp/proc.c +9 ~~-36~~

index dd8492f..9fe1393 100644
@@ -282,7 +282,7 @@ struct sctp_ht_iter {
 	struct rhashtable_iter hti;
 };
 
-static void *sctp_assocs_seq_start(struct seq_file *seq, loff_t *pos)
+static void *sctp_transport_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct sctp_ht_iter *iter = seq->private;
 	int err = sctp_transport_walk_start(&iter->hti);

@@ -293,14 +293,14 @@ static void *sctp_assocs_seq_start(struct seq_file *seq, loff_t *pos)
 	return sctp_transport_get_idx(seq_file_net(seq), &iter->hti, *pos);
 }
 
-static void sctp_assocs_seq_stop(struct seq_file *seq, void *v)
+static void sctp_transport_seq_stop(struct seq_file *seq, void *v)
 {
 	struct sctp_ht_iter *iter = seq->private;
 
 	sctp_transport_walk_stop(&iter->hti);
 }
 
-static void *sctp_assocs_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+static void *sctp_transport_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct sctp_ht_iter *iter = seq->private;

@@ -367,9 +367,9 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 }
 
 static const struct seq_operations sctp_assoc_ops = {
-	.start = sctp_assocs_seq_start,
-	.next  = sctp_assocs_seq_next,
-	.stop  = sctp_assocs_seq_stop,
+	.start = sctp_transport_seq_start,
+	.next  = sctp_transport_seq_next,
+	.stop  = sctp_transport_seq_stop,
 	.show  = sctp_assocs_seq_show,
 };

@@ -406,33 +406,6 @@ void sctp_assocs_proc_exit(struct net *net)
 	remove_proc_entry("assocs", net->sctp.proc_net_sctp);
 }
 
-static void *sctp_remaddr_seq_start(struct seq_file *seq, loff_t *pos)
-{
-	struct sctp_ht_iter *iter = seq->private;
-	int err = sctp_transport_walk_start(&iter->hti);
-
-	if (err)
-		return ERR_PTR(err);
-
-	return sctp_transport_get_idx(seq_file_net(seq), &iter->hti, *pos);
-}
-
-static void *sctp_remaddr_seq_next(struct seq_file *seq, void *v, loff_t *pos)
-{
-	struct sctp_ht_iter *iter = seq->private;
-
-	++*pos;
-
-	return sctp_transport_get_next(seq_file_net(seq), &iter->hti);
-}
-
-static void sctp_remaddr_seq_stop(struct seq_file *seq, void *v)
-{
-	struct sctp_ht_iter *iter = seq->private;
-
-	sctp_transport_walk_stop(&iter->hti);
-}
-
 static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 {
 	struct sctp_association *assoc;

@@ -506,9 +479,9 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 }
 
 static const struct seq_operations sctp_remaddr_ops = {
-	.start = sctp_remaddr_seq_start,
-	.next  = sctp_remaddr_seq_next,
-	.stop  = sctp_remaddr_seq_stop,
+	.start = sctp_transport_seq_start,
+	.next  = sctp_transport_seq_next,
+	.stop  = sctp_transport_seq_stop,
 	.show  = sctp_remaddr_seq_show,
 };

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-04-09 04:53 UTC #6 lore raw

PATCH v2 6/6 sctp: fix some rhashtable functions using in sctp proc/diag

When rhashtable_walk_init return err, no release function should be called, and when rhashtable_walk_start return err, we should only invoke rhashtable_walk_exit to release the source.

But now when sctp_transport_walk_start return err, we just call rhashtable_walk_stop/exit, and never care about if rhashtable_walk_init or start return err, which is so bad.

We will fix it by calling rhashtable_walk_exit if rhashtable_walk_start return err in sctp_transport_walk_start, and if sctp_transport_walk_start return err, we do not need to call sctp_transport_walk_stop any more.

For sctp proc, we will use 'iter->start_fail' to decide if we will call rhashtable_walk_stop/exit.

Signed-off-by Xin Long <[email protected]>

---
 net/sctp/proc.c   |  7 ++++++-
 net/sctp/socket.c | 15 ++++++++++-----
 2 files changed, 16 insertions(+), 6 deletions(-)

net/sctp/proc.c +6 -1

index 9fe1393..4cb5aed 100644
@@ -280,6 +280,7 @@ void sctp_eps_proc_exit(struct net *net)
 struct sctp_ht_iter {
 	struct seq_net_private p;
 	struct rhashtable_iter hti;
+	int start_fail;
 };
 
 static void *sctp_transport_seq_start(struct seq_file *seq, loff_t *pos)

@@ -287,8 +288,10 @@ static void *sctp_transport_seq_start(struct seq_file *seq, loff_t *pos)
 	struct sctp_ht_iter *iter = seq->private;
 	int err = sctp_transport_walk_start(&iter->hti);
 
-	if (err)
+	if (err) {
+		iter->start_fail = 1;
 		return ERR_PTR(err);
+	}
 
 	return sctp_transport_get_idx(seq_file_net(seq), &iter->hti, *pos);
 }

@@ -297,6 +300,8 @@ static void sctp_transport_seq_stop(struct seq_file *seq, void *v)
 {
 	struct sctp_ht_iter *iter = seq->private;
 
+	if (iter->start_fail)
+		return;
 	sctp_transport_walk_stop(&iter->hti);
 }

net/sctp/socket.c +10 -5

index b0bf6c7..473a40c 100644
@@ -4298,8 +4298,12 @@ int sctp_transport_walk_start(struct rhashtable_iter *iter)
 		return err;
 
 	err = rhashtable_walk_start(iter);
+	if (err && err != -EAGAIN) {
+		rhashtable_walk_exit(iter);
+		return err;
+	}
 
-	return err = -EAGAIN ? 0 : err;
+	return 0;
 }
 
 void sctp_transport_walk_stop(struct rhashtable_iter *iter)

@@ -4388,11 +4392,12 @@ EXPORT_SYMBOL_GPL(sctp_transport_lookup_process);
 int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
 			    struct net *net, int pos, void *p) {
 	struct rhashtable_iter hti;
-	int err = 0;
 	void *obj;
+	int err;
 
-	if (sctp_transport_walk_start(&hti))
-		goto out;
+	err = sctp_transport_walk_start(&hti);
+	if (err)
+		return err;
 
 	sctp_transport_get_idx(net, &hti, pos);
 	obj = sctp_transport_get_next(net, &hti);

@@ -4406,8 +4411,8 @@ int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
 		if (err)
 			break;
 	}
-out:
 	sctp_transport_walk_stop(&hti);
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(sctp_for_each_transport);

Signature · 1 line

2.1.0

Eric Dumazet

2016-04-09 05:16 UTC #11 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Xin Long

Quoted 69 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

Signed-off-by Xin Long <[email protected]>

---
 include/linux/sctp.h    | 65 +++++++++++++++++++++++++++++++++++++
 include/net/sctp/sctp.h |  3 ++
 net/sctp/socket.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)

include/linux/sctp.h +50 -0

index a9414fd..a448ebc 100644
@@ -705,4 +705,69 @@ typedef struct sctp_auth_chunk {
 	sctp_authhdr_t auth_hdr;
 } __packed sctp_auth_chunk_t;
 
+struct sctp_info {
+	__u32	sctpi_tag;
+	__u32	sctpi_state;
+	__u32	sctpi_rwnd;
+	__u16	sctpi_unackdata;
+	__u16	sctpi_penddata;
+	__u16	sctpi_instrms;
+	__u16	sctpi_outstrms;
+	__u32	sctpi_fragmentation_point;
+	__u32	sctpi_inqueue;
+	__u32	sctpi_outqueue;
+	__u32	sctpi_overall_error;
+	__u32	sctpi_max_burst;
+	__u32	sctpi_maxseg;
+	__u32	sctpi_peer_rwnd;
+	__u32	sctpi_peer_tag;
+	__u8	sctpi_peer_capable;
+	__u8	sctpi_peer_sack;
+
+	/* assoc status info */
+	__u64	sctpi_isacks;
+	__u64	sctpi_osacks;
+	__u64	sctpi_opackets;
+	__u64	sctpi_ipackets;
+	__u64	sctpi_rtxchunks;
+	__u64	sctpi_outofseqtsns;
+	__u64	sctpi_idupchunks;
+	__u64	sctpi_gapcnt;
+	__u64	sctpi_ouodchunks;
+	__u64	sctpi_iuodchunks;
+	__u64	sctpi_oodchunks;
+	__u64	sctpi_iodchunks;
+	__u64	sctpi_octrlchunks;
+	__u64	sctpi_ictrlchunks;
+
+	/* primary transport info */
+	struct sockaddr_storage	sctpi_p_address;
+	__s32	sctpi_p_state;
+	__u32	sctpi_p_cwnd;
+	__u32	sctpi_p_srtt;
+	__u32	sctpi_p_rto;
+	__u32	sctpi_p_hbinterval;
+	__u32	sctpi_p_pathmaxrxt;
+	__u32	sctpi_p_sackdelay;
+	__u32	sctpi_p_sackfreq;
+	__u32	sctpi_p_ssthresh;
+	__u32	sctpi_p_partial_bytes_acked;
+	__u32	sctpi_p_flight_size;
+	__u16	sctpi_p_error;
+

include/linux/sctp.h typedef struct sctp_auth_chunk { · line 767

Quoted 10 lines

include/linux/sctp.h +10 -0

+	/* sctp sock info */
+	__u32	sctpi_s_autoclose;
+	__u32	sctpi_s_adaptation_ind;
+	__u32	sctpi_s_pd_point;
+	__u8	sctpi_s_nodelay;
+	__u8	sctpi_s_disable_fragments;
+	__u8	sctpi_s_v4mapped;
+	__u8	sctpi_s_frag_interleave;
+};
+

Lots of holes in this structure...

Eric Dumazet

2016-04-09 05:19 UTC #16 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Xin Long

Quoted 29 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

net/sctp/socket.c +23 -0

+int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
+		       struct sctp_info *info)
+{
+	struct sctp_transport *prim;
+	struct list_head *pos, *temp;
+	int mask;
+
+	memset(info, 0, sizeof(*info));
+	if (!asoc) {
+		struct sctp_sock *sp = sctp_sk(sk);
+
+		info->sctpi_s_autoclose = sp->autoclose;
+		info->sctpi_s_adaptation_ind = sp->adaptation_ind;
+		info->sctpi_s_pd_point = sp->pd_point;
+		info->sctpi_s_nodelay = sp->nodelay;
+		info->sctpi_s_disable_fragments = sp->disable_fragments;
+		info->sctpi_s_v4mapped = sp->v4mapped;
+		info->sctpi_s_frag_interleave = sp->frag_interleave;
+
+		return 0;
+	}
+
+	info->sctpi_tag = asoc->c.my_vtag;

net/sctp/socket.c static void sctp_shutdown(struct sock *sk, int how) · line 4237

Quoted 10 lines

net/sctp/socket.c +10 -0

+	info->sctpi_state = asoc->state;
+	info->sctpi_rwnd = asoc->a_rwnd;
+	info->sctpi_unackdata = asoc->unack_data;
+	info->sctpi_penddata = sctp_tsnmap_pending(&asoc->peer.tsn_map);
+	info->sctpi_instrms = asoc->c.sinit_max_instreams;
+	info->sctpi_outstrms = asoc->c.sinit_num_ostreams;
+	list_for_each_safe(pos, temp, &asoc->base.inqueue.in_chunk_list)
+		info->sctpi_inqueue++;
+	list_for_each_safe(pos, temp, &asoc->outqueue.out_chunk_list)
+		info->sctpi_outqueue++;

Is this safe ?

Do you own the lock on socket or whatever lock protecting this list ?

Quoted 42 lines

net/sctp/socket.c +42 -0

+	info->sctpi_overall_error = asoc->overall_error_count;
+	info->sctpi_max_burst = asoc->max_burst;
+	info->sctpi_maxseg = asoc->frag_point;
+	info->sctpi_peer_rwnd = asoc->peer.rwnd;
+	info->sctpi_peer_tag = asoc->c.peer_vtag;
+
+	mask = asoc->peer.ecn_capable << 1;
+	mask = (mask | asoc->peer.ipv4_address) << 1;
+	mask = (mask | asoc->peer.ipv6_address) << 1;
+	mask = (mask | asoc->peer.hostname_address) << 1;
+	mask = (mask | asoc->peer.asconf_capable) << 1;
+	mask = (mask | asoc->peer.prsctp_capable) << 1;
+	mask = (mask | asoc->peer.auth_capable);
+	info->sctpi_peer_capable = mask;
+	mask = asoc->peer.sack_needed << 1;
+	mask = (mask | asoc->peer.sack_generation) << 1;
+	mask = (mask | asoc->peer.zero_window_announced);
+	info->sctpi_peer_sack = mask;
+
+	info->sctpi_isacks = asoc->stats.isacks;
+	info->sctpi_osacks = asoc->stats.osacks;
+	info->sctpi_opackets = asoc->stats.opackets;
+	info->sctpi_ipackets = asoc->stats.ipackets;
+	info->sctpi_rtxchunks = asoc->stats.rtxchunks;
+	info->sctpi_outofseqtsns = asoc->stats.outofseqtsns;
+	info->sctpi_idupchunks = asoc->stats.idupchunks;
+	info->sctpi_gapcnt = asoc->stats.gapcnt;
+	info->sctpi_ouodchunks = asoc->stats.ouodchunks;
+	info->sctpi_iuodchunks = asoc->stats.iuodchunks;
+	info->sctpi_oodchunks = asoc->stats.oodchunks;
+	info->sctpi_iodchunks = asoc->stats.iodchunks;
+	info->sctpi_octrlchunks = asoc->stats.octrlchunks;
+	info->sctpi_ictrlchunks = asoc->stats.ictrlchunks;
+
+	prim = asoc->peer.primary_path;
+	memcpy(&info->sctpi_p_address, &prim->ipaddr,
+	       sizeof(struct sockaddr_storage));
+	info->sctpi_p_state = prim->state;
+	info->sctpi_p_cwnd = prim->cwnd;
+	info->sctpi_p_srtt = prim->srtt;
+	info->sctpi_p_rto = jiffies_to_msecs(prim->rto);
+	info->sctpi_p_hbinterval = prim->hbinterval;

net/sctp/socket.c static void sctp_shutdown(struct sock *sk, int how) · line 4289

Quoted 10 lines

net/sctp/socket.c +10 -0

+	info->sctpi_p_pathmaxrxt = prim->pathmaxrxt;
+	info->sctpi_p_sackdelay = jiffies_to_msecs(prim->sackdelay);
+	info->sctpi_p_ssthresh = prim->ssthresh;
+	info->sctpi_p_partial_bytes_acked = prim->partial_bytes_acked;
+	info->sctpi_p_flight_size = prim->flight_size;
+	info->sctpi_p_error = prim->error_count;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sctp_get_sctp_info);

info is not guaranteed to be aligned on 8 bytes.

You need to use put_unaligned()

Check commit ff5d749772018 ("tcp: beware of alignments in tcp_get_info()") for details.

Eric Dumazet

2016-04-09 05:51 UTC #7 lore raw

Re: [PATCHv2 net-next 4/6] sctp: add the sctp_diag.c file

In reply to Xin Long

Quoted 90 lines · Xin Long

This one will implement all the interface of inet_diag, inet_diag_handler. which includes sctp_diag_dump, sctp_diag_dump_one and sctp_diag_get_info.

net/sctp/sctp_diag.c new file +86 -0

+static int inet_assoc_diag_fill(struct sock *sk,
+				struct sctp_association *asoc,
+				struct sk_buff *skb,
+				const struct inet_diag_req_v2 *req,
+				struct user_namespace *user_ns,
+				int portid, u32 seq, u16 nlmsg_flags,
+				const struct nlmsghdr *unlh)
+{
+	const struct inet_sock *inet = inet_sk(sk);
+	const struct inet_diag_handler *handler;
+	int ext = req->idiag_ext;
+	struct inet_diag_msg *r;
+	struct nlmsghdr  *nlh;
+	struct nlattr *attr;
+	void *info = NULL;
+	union sctp_addr laddr, paddr;
+	struct dst_entry *dst;
+	struct sctp_infox infox;
+
+	handler = inet_diag_get_handler(req->sdiag_protocol);
+	BUG_ON(!handler);
+
+	nlh = nlmsg_put(skb, portid, seq, unlh->nlmsg_type, sizeof(*r),
+			nlmsg_flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	r = nlmsg_data(nlh);
+	BUG_ON(!sk_fullsock(sk));
+
+	laddr = list_entry(asoc->base.bind_addr.address_list.next,
+			   struct sctp_sockaddr_entry, list)->a;
+	paddr = asoc->peer.primary_path->ipaddr;
+	dst = asoc->peer.primary_path->dst;
+
+	r->idiag_family = sk->sk_family;
+	r->id.idiag_sport = htons(asoc->base.bind_addr.port);
+	r->id.idiag_dport = htons(asoc->peer.port);
+	r->id.idiag_if = dst ? dst->dev->ifindex : 0;
+	sock_diag_save_cookie(sk, r->id.idiag_cookie);
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (sk->sk_family = AF_INET6) {
+		*(struct in6_addr *)r->id.idiag_src = laddr.v6.sin6_addr;
+		*(struct in6_addr *)r->id.idiag_dst = paddr.v6.sin6_addr;
+	} else
+#endif
+	{
+		memset(&r->id.idiag_src, 0, sizeof(r->id.idiag_src));
+		memset(&r->id.idiag_dst, 0, sizeof(r->id.idiag_dst));
+
+		r->id.idiag_src[0] = laddr.v4.sin_addr.s_addr;
+		r->id.idiag_dst[0] = paddr.v4.sin_addr.s_addr;
+	}
+
+	r->idiag_state = asoc->state;
+	r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
+	r->idiag_retrans = asoc->rtx_data_chunks;
+#define EXPIRES_IN_MS(tmo)  DIV_ROUND_UP((tmo - jiffies) * 1000, HZ)
+	r->idiag_expires > +		EXPIRES_IN_MS(asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX]);
+#undef EXPIRES_IN_MS
+
+	if (nla_put_u8(skb, INET_DIAG_SHUTDOWN, sk->sk_shutdown))
+		goto errout;
+
+	/* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+	 * hence this needs to be included regardless of socket family.
+	 */
+	if (ext & (1 << (INET_DIAG_TOS - 1)))
+		if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
+			goto errout;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (r->idiag_family = AF_INET6) {
+		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+			if (nla_put_u8(skb, INET_DIAG_TCLASS,
+				       inet6_sk(sk)->tclass) < 0)
+				goto errout;
+
+		if (((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
+		    nla_put_u8(skb, INET_DIAG_SKV6ONLY, ipv6_only_sock(sk)))
+			goto errout;
+	}
+#endif
+
+	r->idiag_uid = from_kuid_munged(user_ns, sock_i_uid(sk));

net/sctp/sctp_diag.c line 153

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+	r->idiag_inode = sock_i_ino(sk);
+
+	if (ext & (1 << (INET_DIAG_MEMINFO - 1))) {
+		struct inet_diag_meminfo minfo = {
+			.idiag_rmem = sk_rmem_alloc_get(sk),
+			.idiag_wmem = sk->sk_wmem_queued,
+			.idiag_fmem = sk->sk_forward_alloc,
+			.idiag_tmem = sk_wmem_alloc_get(sk),
+		};
+

All this code looks familiar.

Why inet_sk_diag_fill() is not used instead ?

Quoted 94 lines

net/sctp/sctp_diag.c new file +94 -0

+		if (nla_put(skb, INET_DIAG_MEMINFO, sizeof(minfo), &minfo) < 0)
+			goto errout;
+	}
+
+	if (ext & (1 << (INET_DIAG_SKMEMINFO - 1)))
+		if (sock_diag_put_meminfo(sk, skb, INET_DIAG_SKMEMINFO))
+			goto errout;
+
+	if ((ext & (1 << (INET_DIAG_INFO - 1))) && handler->idiag_info_size) {
+		attr = nla_reserve(skb, INET_DIAG_INFO,
+				   handler->idiag_info_size);
+		if (!attr)
+			goto errout;
+
+		info = nla_data(attr);
+	}
+	infox.sctpinfo = (struct sctp_info *)info;
+	infox.asoc = asoc;
+	handler->idiag_get_info(sk, r, &infox);
+
+	if (ext & (1 << (INET_DIAG_CONG - 1)))
+		if (nla_put_string(skb, INET_DIAG_CONG, "reno") < 0)
+			goto errout;
+
+	if (inet_sctp_fill_laddrs(skb, &asoc->base.bind_addr.address_list))
+		goto errout;
+
+	if (inet_sctp_fill_paddrs(skb, asoc))
+		goto errout;
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+errout:
+	nlmsg_cancel(skb, nlh);
+	return -EMSGSIZE;
+}
+
+static int inet_ep_diag_fill(struct sock *sk, struct sctp_endpoint *ep,
+			     struct sk_buff *skb,
+			     const struct inet_diag_req_v2 *req,
+			     struct user_namespace *user_ns,
+			     u32 portid, u32 seq, u16 nlmsg_flags,
+			     const struct nlmsghdr *unlh)
+{
+	const struct inet_sock *inet = inet_sk(sk);
+	const struct inet_diag_handler *handler;
+	int ext = req->idiag_ext;
+	struct inet_diag_msg *r;
+	struct nlmsghdr  *nlh;
+	struct nlattr *attr;
+	void *info = NULL;
+	struct sctp_infox infox;
+
+	handler = inet_diag_get_handler(req->sdiag_protocol);
+	BUG_ON(!handler);
+
+	nlh = nlmsg_put(skb, portid, seq, unlh->nlmsg_type, sizeof(*r),
+			nlmsg_flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	r = nlmsg_data(nlh);
+	BUG_ON(!sk_fullsock(sk));
+
+	inet_diag_msg_common_fill(r, sk);
+	r->idiag_state = sk->sk_state;
+	r->idiag_timer = 0;
+	r->idiag_retrans = 0;
+
+	if (nla_put_u8(skb, INET_DIAG_SHUTDOWN, sk->sk_shutdown))
+		goto errout;
+
+	/* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+	 * hence this needs to be included regardless of socket family.
+	 */
+	if (ext & (1 << (INET_DIAG_TOS - 1)))
+		if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
+			goto errout;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (r->idiag_family = AF_INET6) {
+		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+			if (nla_put_u8(skb, INET_DIAG_TCLASS,
+				       inet6_sk(sk)->tclass) < 0)
+				goto errout;
+
+		if (((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
+		    nla_put_u8(skb, INET_DIAG_SKV6ONLY, ipv6_only_sock(sk)))
+			goto errout;
+	}
+#endif
+
+	r->idiag_uid = from_kuid_munged(user_ns, sock_i_uid(sk));

net/sctp/sctp_diag.c line 257

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+	r->idiag_inode = sock_i_ino(sk);
+
+	if (ext & (1 << (INET_DIAG_MEMINFO - 1))) {
+		struct inet_diag_meminfo minfo = {
+			.idiag_rmem = sk_rmem_alloc_get(sk),
+			.idiag_wmem = sk->sk_wmem_queued,
+			.idiag_fmem = sk->sk_forward_alloc,
+			.idiag_tmem = sk_wmem_alloc_get(sk),
+		};
+

Again, looks a lot of duplication.

Also you missed that INET_DIAG_MEMINFO is kind of obsolete, now we have sock_diag_put_meminfo()

Quoted 32 lines

net/sctp/sctp_diag.c new file +32 -0

+		if (nla_put(skb, INET_DIAG_MEMINFO, sizeof(minfo), &minfo) < 0)
+			goto errout;
+	}
+
+	if (ext & (1 << (INET_DIAG_SKMEMINFO - 1)))
+		if (sock_diag_put_meminfo(sk, skb, INET_DIAG_SKMEMINFO))
+			goto errout;
+
+	if ((ext & (1 << (INET_DIAG_INFO - 1))) && handler->idiag_info_size) {
+		attr = nla_reserve(skb, INET_DIAG_INFO,
+				   handler->idiag_info_size);
+		if (!attr)
+			goto errout;
+
+		info = nla_data(attr);
+	}
+	infox.sctpinfo = (struct sctp_info *)info;
+	infox.asoc = NULL;
+	handler->idiag_get_info(sk, r, &infox);
+
+	if (inet_sctp_fill_laddrs(skb, &ep->base.bind_addr.address_list))
+		goto errout;
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+errout:
+	nlmsg_cancel(skb, nlh);
+	return -EMSGSIZE;
+}
+
+static size_t inet_assoc_attr_size(struct sctp_association *asoc)

net/sctp/sctp_diag.c line 299

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+{
+	int addrlen = sizeof(struct sockaddr_storage);
+	int addrcnt = 0;
+	struct sctp_sockaddr_entry *laddr;
+
+	list_for_each_entry_rcu(laddr, &asoc->base.bind_addr.address_list,
+				list)
+		addrcnt++;
+
+	return	  nla_total_size(sizeof(struct tcp_info))

Are you sure you want to use tcp_info ???

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+		+ nla_total_size(1) /* INET_DIAG_SHUTDOWN */
+		+ nla_total_size(1) /* INET_DIAG_TOS */
+		+ nla_total_size(1) /* INET_DIAG_TCLASS */
+		+ nla_total_size(addrlen * asoc->peer.transport_count)
+		+ nla_total_size(addrlen * addrcnt)
+		+ nla_total_size(sizeof(struct inet_diag_meminfo))
+		+ nla_total_size(sizeof(struct inet_diag_msg))
+		+ nla_total_size(sizeof(struct sctp_info))
+		+ 64;
+}

Jamal Hadi Salim

2016-04-09 15:16 UTC #20 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Xin Long

Appreciate these patches. Finally some love for sctp. Small comment below:

Quoted 69 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

Signed-off-by Xin Long <[email protected]>

---
  include/linux/sctp.h    | 65 +++++++++++++++++++++++++++++++++++++
  include/net/sctp/sctp.h |  3 ++
  net/sctp/socket.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 154 insertions(+)

include/linux/sctp.h +50 -0

index a9414fd..a448ebc 100644
@@ -705,4 +705,69 @@ typedef struct sctp_auth_chunk {
  	sctp_authhdr_t auth_hdr;
  } __packed sctp_auth_chunk_t;
 
+struct sctp_info {
+	__u32	sctpi_tag;
+	__u32	sctpi_state;
+	__u32	sctpi_rwnd;
+	__u16	sctpi_unackdata;
+	__u16	sctpi_penddata;
+	__u16	sctpi_instrms;
+	__u16	sctpi_outstrms;
+	__u32	sctpi_fragmentation_point;
+	__u32	sctpi_inqueue;
+	__u32	sctpi_outqueue;
+	__u32	sctpi_overall_error;
+	__u32	sctpi_max_burst;
+	__u32	sctpi_maxseg;
+	__u32	sctpi_peer_rwnd;
+	__u32	sctpi_peer_tag;
+	__u8	sctpi_peer_capable;
+	__u8	sctpi_peer_sack;
+
+	/* assoc status info */
+	__u64	sctpi_isacks;
+	__u64	sctpi_osacks;
+	__u64	sctpi_opackets;
+	__u64	sctpi_ipackets;
+	__u64	sctpi_rtxchunks;
+	__u64	sctpi_outofseqtsns;
+	__u64	sctpi_idupchunks;
+	__u64	sctpi_gapcnt;
+	__u64	sctpi_ouodchunks;
+	__u64	sctpi_iuodchunks;
+	__u64	sctpi_oodchunks;
+	__u64	sctpi_iodchunks;
+	__u64	sctpi_octrlchunks;
+	__u64	sctpi_ictrlchunks;
+
+	/* primary transport info */
+	struct sockaddr_storage	sctpi_p_address;
+	__s32	sctpi_p_state;
+	__u32	sctpi_p_cwnd;
+	__u32	sctpi_p_srtt;
+	__u32	sctpi_p_rto;
+	__u32	sctpi_p_hbinterval;
+	__u32	sctpi_p_pathmaxrxt;
+	__u32	sctpi_p_sackdelay;
+	__u32	sctpi_p_sackfreq;
+	__u32	sctpi_p_ssthresh;
+	__u32	sctpi_p_partial_bytes_acked;
+	__u32	sctpi_p_flight_size;
+	__u16	sctpi_p_error;
+

include/linux/sctp.h typedef struct sctp_auth_chunk { · line 767

Quoted 10 lines

include/linux/sctp.h +10 -0

+	/* sctp sock info */
+	__u32	sctpi_s_autoclose;
+	__u32	sctpi_s_adaptation_ind;
+	__u32	sctpi_s_pd_point;
+	__u8	sctpi_s_nodelay;
+	__u8	sctpi_s_disable_fragments;
+	__u8	sctpi_s_v4mapped;
+	__u8	sctpi_s_frag_interleave;
+};
+

Can you double check to make sure this is 32 bit aligned (no holes) maybe in your case 64 bit aligned?
Sticking + __u16 sctpi_p_error in there seems
to kill it.

Also, any plans to do the netlink events and destroy features?

cheers,
jamal

Jamal Hadi Salim

2016-04-09 15:19 UTC #12 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Eric Dumazet

Quoted 4 lines · Eric Dumazet

Lots of holes in this structure...

I may have mentioned to you that there is 8 bit hole in tcp_info too ;-> (above tcpi_rto). Adding an 8 bit explicit pad seems useful since it is already in the wild. I was going to send the patch after netdev11 but forgot.

cheers,
jamal

Xin Long

author maintainer

2016-04-09 15:40 UTC #8 lore raw

Re: [PATCHv2 net-next 4/6] sctp: add the sctp_diag.c file

In reply to Eric Dumazet

Quoted 104 lines · Eric Dumazet

Quoted 90 lines · Xin Long

This one will implement all the interface of inet_diag, inet_diag_handler. which includes sctp_diag_dump, sctp_diag_dump_one and sctp_diag_get_info.

net/sctp/sctp_diag.c new file +86 -0

+static int inet_assoc_diag_fill(struct sock *sk,
+                             struct sctp_association *asoc,
+                             struct sk_buff *skb,
+                             const struct inet_diag_req_v2 *req,
+                             struct user_namespace *user_ns,
+                             int portid, u32 seq, u16 nlmsg_flags,
+                             const struct nlmsghdr *unlh)
+{
+     const struct inet_sock *inet = inet_sk(sk);
+     const struct inet_diag_handler *handler;
+     int ext = req->idiag_ext;
+     struct inet_diag_msg *r;
+     struct nlmsghdr  *nlh;
+     struct nlattr *attr;
+     void *info = NULL;
+     union sctp_addr laddr, paddr;
+     struct dst_entry *dst;
+     struct sctp_infox infox;
+
+     handler = inet_diag_get_handler(req->sdiag_protocol);
+     BUG_ON(!handler);
+
+     nlh = nlmsg_put(skb, portid, seq, unlh->nlmsg_type, sizeof(*r),
+                     nlmsg_flags);
+     if (!nlh)
+             return -EMSGSIZE;
+
+     r = nlmsg_data(nlh);
+     BUG_ON(!sk_fullsock(sk));
+
+     laddr = list_entry(asoc->base.bind_addr.address_list.next,
+                        struct sctp_sockaddr_entry, list)->a;
+     paddr = asoc->peer.primary_path->ipaddr;
+     dst = asoc->peer.primary_path->dst;
+
+     r->idiag_family = sk->sk_family;
+     r->id.idiag_sport = htons(asoc->base.bind_addr.port);
+     r->id.idiag_dport = htons(asoc->peer.port);
+     r->id.idiag_if = dst ? dst->dev->ifindex : 0;
+     sock_diag_save_cookie(sk, r->id.idiag_cookie);
+
+#if IS_ENABLED(CONFIG_IPV6)
+     if (sk->sk_family = AF_INET6) {
+             *(struct in6_addr *)r->id.idiag_src = laddr.v6.sin6_addr;
+             *(struct in6_addr *)r->id.idiag_dst = paddr.v6.sin6_addr;
+     } else
+#endif
+     {
+             memset(&r->id.idiag_src, 0, sizeof(r->id.idiag_src));
+             memset(&r->id.idiag_dst, 0, sizeof(r->id.idiag_dst));
+
+             r->id.idiag_src[0] = laddr.v4.sin_addr.s_addr;
+             r->id.idiag_dst[0] = paddr.v4.sin_addr.s_addr;
+     }
+
+     r->idiag_state = asoc->state;
+     r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
+     r->idiag_retrans = asoc->rtx_data_chunks;
+#define EXPIRES_IN_MS(tmo)  DIV_ROUND_UP((tmo - jiffies) * 1000, HZ)
+     r->idiag_expires >> +             EXPIRES_IN_MS(asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX]);
+#undef EXPIRES_IN_MS
+
+     if (nla_put_u8(skb, INET_DIAG_SHUTDOWN, sk->sk_shutdown))
+             goto errout;
+
+     /* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+      * hence this needs to be included regardless of socket family.
+      */
+     if (ext & (1 << (INET_DIAG_TOS - 1)))
+             if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
+                     goto errout;
+
+#if IS_ENABLED(CONFIG_IPV6)
+     if (r->idiag_family = AF_INET6) {
+             if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+                     if (nla_put_u8(skb, INET_DIAG_TCLASS,
+                                    inet6_sk(sk)->tclass) < 0)
+                             goto errout;
+
+             if (((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
+                 nla_put_u8(skb, INET_DIAG_SKV6ONLY, ipv6_only_sock(sk)))
+                     goto errout;
+     }
+#endif
+
+     r->idiag_uid = from_kuid_munged(user_ns, sock_i_uid(sk));

net/sctp/sctp_diag.c line 153

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+     r->idiag_inode = sock_i_ino(sk);
+
+     if (ext & (1 << (INET_DIAG_MEMINFO - 1))) {
+             struct inet_diag_meminfo minfo = {
+                     .idiag_rmem = sk_rmem_alloc_get(sk),
+                     .idiag_wmem = sk->sk_wmem_queued,
+                     .idiag_fmem = sk->sk_forward_alloc,
+                     .idiag_tmem = sk_wmem_alloc_get(sk),
+             };
+

All this code looks familiar.

Quoted 2 lines

Why inet_sk_diag_fill() is not used instead ?

it's hard to reuse inet_sk_diag_fill(), cause some of them are from assoc.

yes, there are some duplicate codes. if we want to avoid this. we have to extract new function for this part, and it will change more in inet_diag.

Quoted 107 lines

Quoted 94 lines

net/sctp/sctp_diag.c new file +94 -0

+             if (nla_put(skb, INET_DIAG_MEMINFO, sizeof(minfo), &minfo) < 0)
+                     goto errout;
+     }
+
+     if (ext & (1 << (INET_DIAG_SKMEMINFO - 1)))
+             if (sock_diag_put_meminfo(sk, skb, INET_DIAG_SKMEMINFO))
+                     goto errout;
+
+     if ((ext & (1 << (INET_DIAG_INFO - 1))) && handler->idiag_info_size) {
+             attr = nla_reserve(skb, INET_DIAG_INFO,
+                                handler->idiag_info_size);
+             if (!attr)
+                     goto errout;
+
+             info = nla_data(attr);
+     }
+     infox.sctpinfo = (struct sctp_info *)info;
+     infox.asoc = asoc;
+     handler->idiag_get_info(sk, r, &infox);
+
+     if (ext & (1 << (INET_DIAG_CONG - 1)))
+             if (nla_put_string(skb, INET_DIAG_CONG, "reno") < 0)
+                     goto errout;
+
+     if (inet_sctp_fill_laddrs(skb, &asoc->base.bind_addr.address_list))
+             goto errout;
+
+     if (inet_sctp_fill_paddrs(skb, asoc))
+             goto errout;
+
+     nlmsg_end(skb, nlh);
+     return 0;
+
+errout:
+     nlmsg_cancel(skb, nlh);
+     return -EMSGSIZE;
+}
+
+static int inet_ep_diag_fill(struct sock *sk, struct sctp_endpoint *ep,
+                          struct sk_buff *skb,
+                          const struct inet_diag_req_v2 *req,
+                          struct user_namespace *user_ns,
+                          u32 portid, u32 seq, u16 nlmsg_flags,
+                          const struct nlmsghdr *unlh)
+{
+     const struct inet_sock *inet = inet_sk(sk);
+     const struct inet_diag_handler *handler;
+     int ext = req->idiag_ext;
+     struct inet_diag_msg *r;
+     struct nlmsghdr  *nlh;
+     struct nlattr *attr;
+     void *info = NULL;
+     struct sctp_infox infox;
+
+     handler = inet_diag_get_handler(req->sdiag_protocol);
+     BUG_ON(!handler);
+
+     nlh = nlmsg_put(skb, portid, seq, unlh->nlmsg_type, sizeof(*r),
+                     nlmsg_flags);
+     if (!nlh)
+             return -EMSGSIZE;
+
+     r = nlmsg_data(nlh);
+     BUG_ON(!sk_fullsock(sk));
+
+     inet_diag_msg_common_fill(r, sk);
+     r->idiag_state = sk->sk_state;
+     r->idiag_timer = 0;
+     r->idiag_retrans = 0;
+
+     if (nla_put_u8(skb, INET_DIAG_SHUTDOWN, sk->sk_shutdown))
+             goto errout;
+
+     /* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+      * hence this needs to be included regardless of socket family.
+      */
+     if (ext & (1 << (INET_DIAG_TOS - 1)))
+             if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
+                     goto errout;
+
+#if IS_ENABLED(CONFIG_IPV6)
+     if (r->idiag_family = AF_INET6) {
+             if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+                     if (nla_put_u8(skb, INET_DIAG_TCLASS,
+                                    inet6_sk(sk)->tclass) < 0)
+                             goto errout;
+
+             if (((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
+                 nla_put_u8(skb, INET_DIAG_SKV6ONLY, ipv6_only_sock(sk)))
+                     goto errout;
+     }
+#endif
+
+     r->idiag_uid = from_kuid_munged(user_ns, sock_i_uid(sk));

net/sctp/sctp_diag.c line 257

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+     r->idiag_inode = sock_i_ino(sk);
+
+     if (ext & (1 << (INET_DIAG_MEMINFO - 1))) {
+             struct inet_diag_meminfo minfo = {
+                     .idiag_rmem = sk_rmem_alloc_get(sk),
+                     .idiag_wmem = sk->sk_wmem_queued,
+                     .idiag_fmem = sk->sk_forward_alloc,
+                     .idiag_tmem = sk_wmem_alloc_get(sk),
+             };
+

Again, looks a lot of duplication.

Quoted 3 lines

Also you missed that INET_DIAG_MEMINFO is kind of obsolete, now we have sock_diag_put_meminfo()

you meant we can remove it here ?
yes, it seems similar with INET_DIAG_SKMEMINFO.
but I do not know if userpace may use INET_DIAG_MEMINFO now.

Quoted 37 lines

net/sctp/sctp_diag.c new file +36 -0

+             if (nla_put(skb, INET_DIAG_MEMINFO, sizeof(minfo), &minfo) < 0)
+                     goto errout;
+     }
+
+     if (ext & (1 << (INET_DIAG_SKMEMINFO - 1)))
+             if (sock_diag_put_meminfo(sk, skb, INET_DIAG_SKMEMINFO))
+                     goto errout;
+
+     if ((ext & (1 << (INET_DIAG_INFO - 1))) && handler->idiag_info_size) {
+             attr = nla_reserve(skb, INET_DIAG_INFO,
+                                handler->idiag_info_size);
+             if (!attr)
+                     goto errout;
+
+             info = nla_data(attr);
+     }
+     infox.sctpinfo = (struct sctp_info *)info;
+     infox.asoc = NULL;
+     handler->idiag_get_info(sk, r, &infox);
+
+     if (inet_sctp_fill_laddrs(skb, &ep->base.bind_addr.address_list))
+             goto errout;
+
+     nlmsg_end(skb, nlh);
+     return 0;
+
+errout:
+     nlmsg_cancel(skb, nlh);
+     return -EMSGSIZE;
+}
+
+static size_t inet_assoc_attr_size(struct sctp_association *asoc)
+{
+     int addrlen = sizeof(struct sockaddr_storage);
+     int addrcnt = 0;
+     struct sctp_sockaddr_entry *laddr;

Quoted 8 lines

net/sctp/sctp_diag.c line 299
Quoted 6 lines
net/sctp/sctp_diag.c new file +6 -0
+
+     list_for_each_entry_rcu(laddr, &asoc->base.bind_addr.address_list,
+                             list)
+             addrcnt++;
+
+     return    nla_total_size(sizeof(struct tcp_info))
Are you sure you want to use tcp_info ???

here is a mistake, it should be:
nla_total_size(sizeof(struct sctp_info))

thanks.

Quoted 9 lines

net/sctp/sctp_diag.c line 307

Quoted 9 lines

net/sctp/sctp_diag.c new file +8 -0

+             + nla_total_size(1) /* INET_DIAG_SHUTDOWN */
+             + nla_total_size(1) /* INET_DIAG_TOS */
+             + nla_total_size(1) /* INET_DIAG_TCLASS */
+             + nla_total_size(addrlen * asoc->peer.transport_count)
+             + nla_total_size(addrlen * addrcnt)
+             + nla_total_size(sizeof(struct inet_diag_meminfo))
+             + nla_total_size(sizeof(struct inet_diag_msg))
+             + nla_total_size(sizeof(struct sctp_info))

and will remove this one.

Quoted 5 lines

Quoted 2 lines
(quoted diff) +2 -0
+             + 64;
+}

Xin Long

author maintainer

2016-04-09 15:45 UTC #15 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Eric Dumazet

Quoted 74 lines · Eric Dumazet

Quoted 73 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

Signed-off-by Xin Long <[email protected]>

---
 include/linux/sctp.h    | 65 +++++++++++++++++++++++++++++++++++++
 include/net/sctp/sctp.h |  3 ++
 net/sctp/socket.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)

include/linux/sctp.h +54 -0

index a9414fd..a448ebc 100644
@@ -705,4 +705,69 @@ typedef struct sctp_auth_chunk {
      sctp_authhdr_t auth_hdr;
 } __packed sctp_auth_chunk_t;
 
+struct sctp_info {
+     __u32   sctpi_tag;
+     __u32   sctpi_state;
+     __u32   sctpi_rwnd;
+     __u16   sctpi_unackdata;
+     __u16   sctpi_penddata;
+     __u16   sctpi_instrms;
+     __u16   sctpi_outstrms;
+     __u32   sctpi_fragmentation_point;
+     __u32   sctpi_inqueue;
+     __u32   sctpi_outqueue;
+     __u32   sctpi_overall_error;
+     __u32   sctpi_max_burst;
+     __u32   sctpi_maxseg;
+     __u32   sctpi_peer_rwnd;
+     __u32   sctpi_peer_tag;
+     __u8    sctpi_peer_capable;
+     __u8    sctpi_peer_sack;
+
+     /* assoc status info */
+     __u64   sctpi_isacks;
+     __u64   sctpi_osacks;
+     __u64   sctpi_opackets;
+     __u64   sctpi_ipackets;
+     __u64   sctpi_rtxchunks;
+     __u64   sctpi_outofseqtsns;
+     __u64   sctpi_idupchunks;
+     __u64   sctpi_gapcnt;
+     __u64   sctpi_ouodchunks;
+     __u64   sctpi_iuodchunks;
+     __u64   sctpi_oodchunks;
+     __u64   sctpi_iodchunks;
+     __u64   sctpi_octrlchunks;
+     __u64   sctpi_ictrlchunks;
+
+     /* primary transport info */
+     struct sockaddr_storage sctpi_p_address;
+     __s32   sctpi_p_state;
+     __u32   sctpi_p_cwnd;
+     __u32   sctpi_p_srtt;
+     __u32   sctpi_p_rto;
+     __u32   sctpi_p_hbinterval;
+     __u32   sctpi_p_pathmaxrxt;
+     __u32   sctpi_p_sackdelay;
+     __u32   sctpi_p_sackfreq;
+     __u32   sctpi_p_ssthresh;
+     __u32   sctpi_p_partial_bytes_acked;
+     __u32   sctpi_p_flight_size;
+     __u16   sctpi_p_error;
+
+     /* sctp sock info */
+     __u32   sctpi_s_autoclose;
+     __u32   sctpi_s_adaptation_ind;
+     __u32   sctpi_s_pd_point;

Quoted 10 lines

include/linux/sctp.h typedef struct sctp_auth_chunk { · line 767
Quoted 6 lines
include/linux/sctp.h +6 -0
+     __u8    sctpi_s_nodelay;
+     __u8    sctpi_s_disable_fragments;
+     __u8    sctpi_s_v4mapped;
+     __u8    sctpi_s_frag_interleave;
+};
+
Lots of holes in this structure...

will check and improve it later.
thanks.

Xin Long

author maintainer

2016-04-09 16:10 UTC #17 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Eric Dumazet

Quoted 43 lines · Eric Dumazet

Quoted 29 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

net/sctp/socket.c +23 -0

+int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
+                    struct sctp_info *info)
+{
+     struct sctp_transport *prim;
+     struct list_head *pos, *temp;
+     int mask;
+
+     memset(info, 0, sizeof(*info));
+     if (!asoc) {
+             struct sctp_sock *sp = sctp_sk(sk);
+
+             info->sctpi_s_autoclose = sp->autoclose;
+             info->sctpi_s_adaptation_ind = sp->adaptation_ind;
+             info->sctpi_s_pd_point = sp->pd_point;
+             info->sctpi_s_nodelay = sp->nodelay;
+             info->sctpi_s_disable_fragments = sp->disable_fragments;
+             info->sctpi_s_v4mapped = sp->v4mapped;
+             info->sctpi_s_frag_interleave = sp->frag_interleave;
+
+             return 0;
+     }
+
+     info->sctpi_tag = asoc->c.my_vtag;

net/sctp/socket.c static void sctp_shutdown(struct sock *sk, int how) · line 4237

Quoted 10 lines

net/sctp/socket.c +10 -0

+     info->sctpi_state = asoc->state;
+     info->sctpi_rwnd = asoc->a_rwnd;
+     info->sctpi_unackdata = asoc->unack_data;
+     info->sctpi_penddata = sctp_tsnmap_pending(&asoc->peer.tsn_map);
+     info->sctpi_instrms = asoc->c.sinit_max_instreams;
+     info->sctpi_outstrms = asoc->c.sinit_num_ostreams;
+     list_for_each_safe(pos, temp, &asoc->base.inqueue.in_chunk_list)
+             info->sctpi_inqueue++;
+     list_for_each_safe(pos, temp, &asoc->outqueue.out_chunk_list)
+             info->sctpi_outqueue++;

Is this safe ?

Quoted 2 lines

Do you own the lock on socket or whatever lock protecting this list ?

there are 2 places will call these codes,
1. sctp_diag_dump -> sctp_for_each_transport -> sctp_tsp_dump this one will use lock_sock to protect them. I think this one is ok.

1. sctp_diag_dump_one -> sctp_transport_lookup_process-> sctp_tsp_dump_one this one just holds the tsp. and we're using list_for_each_safe here now, isn't it enough ?

Quoted 56 lines

Quoted 43 lines

net/sctp/socket.c +42 -0

+     info->sctpi_overall_error = asoc->overall_error_count;
+     info->sctpi_max_burst = asoc->max_burst;
+     info->sctpi_maxseg = asoc->frag_point;
+     info->sctpi_peer_rwnd = asoc->peer.rwnd;
+     info->sctpi_peer_tag = asoc->c.peer_vtag;
+
+     mask = asoc->peer.ecn_capable << 1;
+     mask = (mask | asoc->peer.ipv4_address) << 1;
+     mask = (mask | asoc->peer.ipv6_address) << 1;
+     mask = (mask | asoc->peer.hostname_address) << 1;
+     mask = (mask | asoc->peer.asconf_capable) << 1;
+     mask = (mask | asoc->peer.prsctp_capable) << 1;
+     mask = (mask | asoc->peer.auth_capable);
+     info->sctpi_peer_capable = mask;
+     mask = asoc->peer.sack_needed << 1;
+     mask = (mask | asoc->peer.sack_generation) << 1;
+     mask = (mask | asoc->peer.zero_window_announced);
+     info->sctpi_peer_sack = mask;
+
+     info->sctpi_isacks = asoc->stats.isacks;
+     info->sctpi_osacks = asoc->stats.osacks;
+     info->sctpi_opackets = asoc->stats.opackets;
+     info->sctpi_ipackets = asoc->stats.ipackets;
+     info->sctpi_rtxchunks = asoc->stats.rtxchunks;
+     info->sctpi_outofseqtsns = asoc->stats.outofseqtsns;
+     info->sctpi_idupchunks = asoc->stats.idupchunks;
+     info->sctpi_gapcnt = asoc->stats.gapcnt;
+     info->sctpi_ouodchunks = asoc->stats.ouodchunks;
+     info->sctpi_iuodchunks = asoc->stats.iuodchunks;
+     info->sctpi_oodchunks = asoc->stats.oodchunks;
+     info->sctpi_iodchunks = asoc->stats.iodchunks;
+     info->sctpi_octrlchunks = asoc->stats.octrlchunks;
+     info->sctpi_ictrlchunks = asoc->stats.ictrlchunks;
+
+     prim = asoc->peer.primary_path;
+     memcpy(&info->sctpi_p_address, &prim->ipaddr,
+            sizeof(struct sockaddr_storage));
+     info->sctpi_p_state = prim->state;
+     info->sctpi_p_cwnd = prim->cwnd;
+     info->sctpi_p_srtt = prim->srtt;
+     info->sctpi_p_rto = jiffies_to_msecs(prim->rto);
+     info->sctpi_p_hbinterval = prim->hbinterval;

net/sctp/socket.c static void sctp_shutdown(struct sock *sk, int how) · line 4289

Quoted 10 lines

net/sctp/socket.c +10 -0

+     info->sctpi_p_pathmaxrxt = prim->pathmaxrxt;
+     info->sctpi_p_sackdelay = jiffies_to_msecs(prim->sackdelay);
+     info->sctpi_p_ssthresh = prim->ssthresh;
+     info->sctpi_p_partial_bytes_acked = prim->partial_bytes_acked;
+     info->sctpi_p_flight_size = prim->flight_size;
+     info->sctpi_p_error = prim->error_count;
+
+     return 0;
+}
+EXPORT_SYMBOL_GPL(sctp_get_sctp_info);

info is not guaranteed to be aligned on 8 bytes.

Quoted 4 lines

You need to use put_unaligned()
Check commit ff5d749772018 ("tcp: beware of alignments in tcp_get_info()") for details.

Ok, I will.

Quoted 3 lines

Eric Dumazet

2016-04-09 17:21 UTC #13 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Jamal Hadi Salim

Quoted 8 lines · Jamal Hadi Salim

Quoted 4 lines · Eric Dumazet
Lots of holes in this structure...

Quoted 4 lines

I may have mentioned to you that there is 8 bit hole in tcp_info too ;-> (above tcpi_rto). Adding an 8 bit explicit pad seems useful since it is already in the wild. I was going to send the patch after netdev11 but forgot.

Well, once a hole is there, nothing we can do really, because of compatibility with old kernels / old binaries.

But when a _new_ structure is defined, this is the time where we can ask for doing sensible things ;)

Eric Dumazet

2016-04-09 17:23 UTC #9 lore raw

Re: [PATCHv2 net-next 4/6] sctp: add the sctp_diag.c file

In reply to Xin Long

Quoted 4 lines · Xin Long

you meant we can remove it here ?
yes, it seems similar with INET_DIAG_SKMEMINFO.
but I do not know if userpace may use INET_DIAG_MEMINFO now.

You are adding new features here. No problem of legacy code.

Anyway, as I said you really need to reuse code, not copy paste please.

Eric Dumazet

2016-04-09 17:29 UTC #18 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Xin Long

Quoted 46 lines · Xin Long

Quoted 43 lines · Eric Dumazet

Quoted 29 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

net/sctp/socket.c +23 -0

+int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
+                    struct sctp_info *info)
+{
+     struct sctp_transport *prim;
+     struct list_head *pos, *temp;
+     int mask;
+
+     memset(info, 0, sizeof(*info));
+     if (!asoc) {
+             struct sctp_sock *sp = sctp_sk(sk);
+
+             info->sctpi_s_autoclose = sp->autoclose;
+             info->sctpi_s_adaptation_ind = sp->adaptation_ind;
+             info->sctpi_s_pd_point = sp->pd_point;
+             info->sctpi_s_nodelay = sp->nodelay;
+             info->sctpi_s_disable_fragments = sp->disable_fragments;
+             info->sctpi_s_v4mapped = sp->v4mapped;
+             info->sctpi_s_frag_interleave = sp->frag_interleave;
+
+             return 0;
+     }
+
+     info->sctpi_tag = asoc->c.my_vtag;

net/sctp/socket.c static void sctp_shutdown(struct sock *sk, int how) · line 4237

Quoted 10 lines

net/sctp/socket.c +10 -0

+     info->sctpi_state = asoc->state;
+     info->sctpi_rwnd = asoc->a_rwnd;
+     info->sctpi_unackdata = asoc->unack_data;
+     info->sctpi_penddata = sctp_tsnmap_pending(&asoc->peer.tsn_map);
+     info->sctpi_instrms = asoc->c.sinit_max_instreams;
+     info->sctpi_outstrms = asoc->c.sinit_num_ostreams;
+     list_for_each_safe(pos, temp, &asoc->base.inqueue.in_chunk_list)
+             info->sctpi_inqueue++;
+     list_for_each_safe(pos, temp, &asoc->outqueue.out_chunk_list)
+             info->sctpi_outqueue++;

Is this safe ?

Quoted 2 lines

Do you own the lock on socket or whatever lock protecting this list ?

Quoted 8 lines

there are 2 places will call these codes,
1. sctp_diag_dump -> sctp_for_each_transport -> sctp_tsp_dump this one will use lock_sock to protect them. I think this one is ok.
1. sctp_diag_dump_one -> sctp_transport_lookup_process-> sctp_tsp_dump_one this one just holds the tsp. and we're using list_for_each_safe here now, isn't it enough ?

You tell me ;)

For sure in tcp_get_info() the socket is sometimes locked, and sometimes is not locked, depending on the caller.

So we had to use only lockless accesses.

Eric Dumazet

2016-04-09 17:31 UTC #19 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Xin Long

Quoted 3 lines · Xin Long

1. sctp_diag_dump_one -> sctp_transport_lookup_process-> sctp_tsp_dump_one this one just holds the tsp. and we're using list_for_each_safe here now, isn't it enough ?

list_for_each_safe is 'safe' if you do a list_del() yourself.

It is not safe if other cpus are adding/deleting items in the list while this thread is iterating it.

Xin Long

author maintainer

2016-04-10 06:42 UTC #21 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Jamal Hadi Salim

Quoted 85 lines · Jamal Hadi Salim

Appreciate these patches. Finally some love for sctp. Small comment below:

Quoted 80 lines · Xin Long

sctp_diag will dump some important details of sctp's assoc or ep, we use sctp_info to describe them, sctp_get_sctp_info to get them, and export it to sctp_diag.ko.

Signed-off-by Xin Long <[email protected]>

---
  include/linux/sctp.h    | 65 +++++++++++++++++++++++++++++++++++++
  include/net/sctp/sctp.h |  3 ++
  net/sctp/socket.c       | 86
+++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 154 insertions(+)

include/linux/sctp.h +59 -0

index a9414fd..a448ebc 100644
@@ -705,4 +705,69 @@ typedef struct sctp_auth_chunk {
        sctp_authhdr_t auth_hdr;
  } __packed sctp_auth_chunk_t;
 
+struct sctp_info {
+       __u32   sctpi_tag;
+       __u32   sctpi_state;
+       __u32   sctpi_rwnd;
+       __u16   sctpi_unackdata;
+       __u16   sctpi_penddata;
+       __u16   sctpi_instrms;
+       __u16   sctpi_outstrms;
+       __u32   sctpi_fragmentation_point;
+       __u32   sctpi_inqueue;
+       __u32   sctpi_outqueue;
+       __u32   sctpi_overall_error;
+       __u32   sctpi_max_burst;
+       __u32   sctpi_maxseg;
+       __u32   sctpi_peer_rwnd;
+       __u32   sctpi_peer_tag;
+       __u8    sctpi_peer_capable;
+       __u8    sctpi_peer_sack;
+
+       /* assoc status info */
+       __u64   sctpi_isacks;
+       __u64   sctpi_osacks;
+       __u64   sctpi_opackets;
+       __u64   sctpi_ipackets;
+       __u64   sctpi_rtxchunks;
+       __u64   sctpi_outofseqtsns;
+       __u64   sctpi_idupchunks;
+       __u64   sctpi_gapcnt;
+       __u64   sctpi_ouodchunks;
+       __u64   sctpi_iuodchunks;
+       __u64   sctpi_oodchunks;
+       __u64   sctpi_iodchunks;
+       __u64   sctpi_octrlchunks;
+       __u64   sctpi_ictrlchunks;
+
+       /* primary transport info */
+       struct sockaddr_storage sctpi_p_address;
+       __s32   sctpi_p_state;
+       __u32   sctpi_p_cwnd;
+       __u32   sctpi_p_srtt;
+       __u32   sctpi_p_rto;
+       __u32   sctpi_p_hbinterval;
+       __u32   sctpi_p_pathmaxrxt;
+       __u32   sctpi_p_sackdelay;
+       __u32   sctpi_p_sackfreq;
+       __u32   sctpi_p_ssthresh;
+       __u32   sctpi_p_partial_bytes_acked;
+       __u32   sctpi_p_flight_size;
+       __u16   sctpi_p_error;
+
+       /* sctp sock info */
+       __u32   sctpi_s_autoclose;
+       __u32   sctpi_s_adaptation_ind;
+       __u32   sctpi_s_pd_point;
+       __u8    sctpi_s_nodelay;
+       __u8    sctpi_s_disable_fragments;
+       __u8    sctpi_s_v4mapped;
+       __u8    sctpi_s_frag_interleave;
+};

Quoted 8 lines

include/linux/sctp.h typedef struct sctp_auth_chunk { · line 767
Quoted 1 line
include/linux/sctp.h +1 -0
+
Can you double check to make sure this is 32 bit aligned (no holes) maybe in your case 64 bit aligned?
Sticking + __u16 sctpi_p_error in there seems
to kill it.

yes, I will check this structure all.

Quoted 2 lines

Also, any plans to do the netlink events and destroy features?

yeah, maybe after this one, we will do it.
but for destroy feature, do you have any idea to test, cause ss of iproute seems cannot cover it, even .dump_one, neither. do we have to write code to test them?

netlink events? sorry, what's that for? I didn't see it in tcp_diag.

Quoted 3 lines

cheers,
jamal

Jamal Hadi Salim

2016-04-10 13:02 UTC #14 lore raw

Re: [PATCHv2 net-next 1/6] sctp: add sctp_info dump api for sctp_diag

In reply to Eric Dumazet

Quoted 7 lines · Eric Dumazet

Well, once a hole is there, nothing we can do really, because of compatibility with old kernels / old binaries.
But when a _new_ structure is defined, this is the time where we can ask for doing sensible things ;)

This one is fixable. sizeof() already includes the accounting of the pad. something like:

include/uapi/linux/tcp.h +1 -0

index fe95446..52542eb 100644
@@ -158,6 +158,7 @@ struct tcp_info {
         __u8    tcpi_options;
         __u8    tcpi_snd_wscale : 4, tcpi_rcv_wscale : 4;
 
+       __u8    pad;    /*reuse this space if you need 8bits for something*/
         __u32   tcpi_rto;
         __u32   tcpi_ato;
         __u32   tcpi_snd_mss;

cheers,
jamal

Marcelo Ricardo Leitner

maintainer

2016-04-11 21:21 UTC #10 lore raw

Re: [PATCHv2 net-next 4/6] sctp: add the sctp_diag.c file

In reply to Eric Dumazet

Quoted 59 lines · Eric Dumazet

Quoted 58 lines · Xin Long

This one will implement all the interface of inet_diag, inet_diag_handler. which includes sctp_diag_dump, sctp_diag_dump_one and sctp_diag_get_info.

net/sctp/sctp_diag.c new file +54 -0

+static int inet_assoc_diag_fill(struct sock *sk,
+				struct sctp_association *asoc,
+				struct sk_buff *skb,
+				const struct inet_diag_req_v2 *req,
+				struct user_namespace *user_ns,
+				int portid, u32 seq, u16 nlmsg_flags,
+				const struct nlmsghdr *unlh)
+{
+	const struct inet_sock *inet = inet_sk(sk);
+	const struct inet_diag_handler *handler;
+	int ext = req->idiag_ext;
+	struct inet_diag_msg *r;
+	struct nlmsghdr  *nlh;
+	struct nlattr *attr;
+	void *info = NULL;
+	union sctp_addr laddr, paddr;
+	struct dst_entry *dst;
+	struct sctp_infox infox;
+
+	handler = inet_diag_get_handler(req->sdiag_protocol);
+	BUG_ON(!handler);
+
+	nlh = nlmsg_put(skb, portid, seq, unlh->nlmsg_type, sizeof(*r),
+			nlmsg_flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	r = nlmsg_data(nlh);
+	BUG_ON(!sk_fullsock(sk));
+
+	laddr = list_entry(asoc->base.bind_addr.address_list.next,
+			   struct sctp_sockaddr_entry, list)->a;
+	paddr = asoc->peer.primary_path->ipaddr;
+	dst = asoc->peer.primary_path->dst;
+
+	r->idiag_family = sk->sk_family;
+	r->id.idiag_sport = htons(asoc->base.bind_addr.port);
+	r->id.idiag_dport = htons(asoc->peer.port);
+	r->id.idiag_if = dst ? dst->dev->ifindex : 0;
+	sock_diag_save_cookie(sk, r->id.idiag_cookie);
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (sk->sk_family = AF_INET6) {
+		*(struct in6_addr *)r->id.idiag_src = laddr.v6.sin6_addr;
+		*(struct in6_addr *)r->id.idiag_dst = paddr.v6.sin6_addr;
+	} else
+#endif
+	{
+		memset(&r->id.idiag_src, 0, sizeof(r->id.idiag_src));
+		memset(&r->id.idiag_dst, 0, sizeof(r->id.idiag_dst));
+
+		r->id.idiag_src[0] = laddr.v4.sin_addr.s_addr;
+		r->id.idiag_dst[0] = paddr.v4.sin_addr.s_addr;
+	}

Quoted 8 lines

net/sctp/sctp_diag.c line 119

Quoted 8 lines

net/sctp/sctp_diag.c new file +8 -0

+
+	r->idiag_state = asoc->state;
+	r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
+	r->idiag_retrans = asoc->rtx_data_chunks;
+#define EXPIRES_IN_MS(tmo)  DIV_ROUND_UP((tmo - jiffies) * 1000, HZ)
+	r->idiag_expires > > +		EXPIRES_IN_MS(asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX]);
+#undef EXPIRES_IN_MS
+

vvv

Quoted 37 lines

Quoted 24 lines

net/sctp/sctp_diag.c new file +24 -0

+	if (nla_put_u8(skb, INET_DIAG_SHUTDOWN, sk->sk_shutdown))
+		goto errout;
+
+	/* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+	 * hence this needs to be included regardless of socket family.
+	 */
+	if (ext & (1 << (INET_DIAG_TOS - 1)))
+		if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
+			goto errout;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (r->idiag_family = AF_INET6) {
+		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+			if (nla_put_u8(skb, INET_DIAG_TCLASS,
+				       inet6_sk(sk)->tclass) < 0)
+				goto errout;
+
+		if (((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
+		    nla_put_u8(skb, INET_DIAG_SKV6ONLY, ipv6_only_sock(sk)))
+			goto errout;
+	}
+#endif
+
+	r->idiag_uid = from_kuid_munged(user_ns, sock_i_uid(sk));

net/sctp/sctp_diag.c line 153

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+	r->idiag_inode = sock_i_ino(sk);
+
+	if (ext & (1 << (INET_DIAG_MEMINFO - 1))) {
+		struct inet_diag_meminfo minfo = {
+			.idiag_rmem = sk_rmem_alloc_get(sk),
+			.idiag_wmem = sk->sk_wmem_queued,
+			.idiag_fmem = sk->sk_forward_alloc,
+			.idiag_tmem = sk_wmem_alloc_get(sk),
+		};
+

All this code looks familiar.

Quoted 1 line

Why inet_sk_diag_fill() is not used instead ?

Actually we have an issue here. idiag_tmem is using sk_wmem_alloc_get() directly, but it shouldn't. SCTP supports using sndbuf per socket or per assoc on that socket. We need an if and report the correct value, as it's done in sctp_wspace().

For inet_ep_diag_fill, it's reporting an endpoint, so this is not needed as we don't have this option in there.

Quoted 7 lines

net/sctp/sctp_diag.c line 160

Quoted 7 lines

net/sctp/sctp_diag.c new file +7 -0

+		if (nla_put(skb, INET_DIAG_MEMINFO, sizeof(minfo), &minfo) < 0)
+			goto errout;
+	}
+
+	if (ext & (1 << (INET_DIAG_SKMEMINFO - 1)))
+		if (sock_diag_put_meminfo(sk, skb, INET_DIAG_SKMEMINFO))
+			goto errout;

Same happens in sock_diag_put_meminfo().

I highlighted the section that would have been common to inet_sk_diag_fill() if it wasn't for this wmem_alloc difference using vvv and ^^^ marks. There is one or another common lines out of it. I suggest we try to factor out this common code but only from within these two functions in sctp_diag, leaving inet_diag alone for now, just until this soak a bit more.

Quoted 9 lines

net/sctp/sctp_diag.c line 169

Quoted 9 lines

net/sctp/sctp_diag.c new file +9 -0

+
+	if ((ext & (1 << (INET_DIAG_INFO - 1))) && handler->idiag_info_size) {
+		attr = nla_reserve(skb, INET_DIAG_INFO,
+				   handler->idiag_info_size);
+		if (!attr)
+			goto errout;
+
+		info = nla_data(attr);
+	}

^^^

Quoted 7 lines

net/sctp/sctp_diag.c line 176

Quoted 7 lines

net/sctp/sctp_diag.c new file +7 -0

+	infox.sctpinfo = (struct sctp_info *)info;
+	infox.asoc = asoc;
+	handler->idiag_get_info(sk, r, &infox);
+
+	if (ext & (1 << (INET_DIAG_CONG - 1)))
+		if (nla_put_string(skb, INET_DIAG_CONG, "reno") < 0)
+			goto errout;

the above block is not common to inet_sk_diag_fill

Quoted 151 lines

Quoted 71 lines

net/sctp/sctp_diag.c new file +71 -0

+
+	if (inet_sctp_fill_laddrs(skb, &asoc->base.bind_addr.address_list))
+		goto errout;
+
+	if (inet_sctp_fill_paddrs(skb, asoc))
+		goto errout;
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+errout:
+	nlmsg_cancel(skb, nlh);
+	return -EMSGSIZE;
+}
+
+static int inet_ep_diag_fill(struct sock *sk, struct sctp_endpoint *ep,
+			     struct sk_buff *skb,
+			     const struct inet_diag_req_v2 *req,
+			     struct user_namespace *user_ns,
+			     u32 portid, u32 seq, u16 nlmsg_flags,
+			     const struct nlmsghdr *unlh)
+{
+	const struct inet_sock *inet = inet_sk(sk);
+	const struct inet_diag_handler *handler;
+	int ext = req->idiag_ext;
+	struct inet_diag_msg *r;
+	struct nlmsghdr  *nlh;
+	struct nlattr *attr;
+	void *info = NULL;
+	struct sctp_infox infox;
+
+	handler = inet_diag_get_handler(req->sdiag_protocol);
+	BUG_ON(!handler);
+
+	nlh = nlmsg_put(skb, portid, seq, unlh->nlmsg_type, sizeof(*r),
+			nlmsg_flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	r = nlmsg_data(nlh);
+	BUG_ON(!sk_fullsock(sk));
+
+	inet_diag_msg_common_fill(r, sk);
+	r->idiag_state = sk->sk_state;
+	r->idiag_timer = 0;
+	r->idiag_retrans = 0;
+
+	if (nla_put_u8(skb, INET_DIAG_SHUTDOWN, sk->sk_shutdown))
+		goto errout;
+
+	/* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+	 * hence this needs to be included regardless of socket family.
+	 */
+	if (ext & (1 << (INET_DIAG_TOS - 1)))
+		if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
+			goto errout;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (r->idiag_family = AF_INET6) {
+		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+			if (nla_put_u8(skb, INET_DIAG_TCLASS,
+				       inet6_sk(sk)->tclass) < 0)
+				goto errout;
+
+		if (((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) &&
+		    nla_put_u8(skb, INET_DIAG_SKV6ONLY, ipv6_only_sock(sk)))
+			goto errout;
+	}
+#endif
+
+	r->idiag_uid = from_kuid_munged(user_ns, sock_i_uid(sk));

net/sctp/sctp_diag.c line 257

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+	r->idiag_inode = sock_i_ino(sk);
+
+	if (ext & (1 << (INET_DIAG_MEMINFO - 1))) {
+		struct inet_diag_meminfo minfo = {
+			.idiag_rmem = sk_rmem_alloc_get(sk),
+			.idiag_wmem = sk->sk_wmem_queued,
+			.idiag_fmem = sk->sk_forward_alloc,
+			.idiag_tmem = sk_wmem_alloc_get(sk),
+		};
+

Again, looks a lot of duplication.

Also you missed that INET_DIAG_MEMINFO is kind of obsolete, now we have sock_diag_put_meminfo()

Quoted 32 lines

net/sctp/sctp_diag.c new file +32 -0

+		if (nla_put(skb, INET_DIAG_MEMINFO, sizeof(minfo), &minfo) < 0)
+			goto errout;
+	}
+
+	if (ext & (1 << (INET_DIAG_SKMEMINFO - 1)))
+		if (sock_diag_put_meminfo(sk, skb, INET_DIAG_SKMEMINFO))
+			goto errout;
+
+	if ((ext & (1 << (INET_DIAG_INFO - 1))) && handler->idiag_info_size) {
+		attr = nla_reserve(skb, INET_DIAG_INFO,
+				   handler->idiag_info_size);
+		if (!attr)
+			goto errout;
+
+		info = nla_data(attr);
+	}
+	infox.sctpinfo = (struct sctp_info *)info;
+	infox.asoc = NULL;
+	handler->idiag_get_info(sk, r, &infox);
+
+	if (inet_sctp_fill_laddrs(skb, &ep->base.bind_addr.address_list))
+		goto errout;
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+errout:
+	nlmsg_cancel(skb, nlh);
+	return -EMSGSIZE;
+}
+
+static size_t inet_assoc_attr_size(struct sctp_association *asoc)

net/sctp/sctp_diag.c line 299

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+{
+	int addrlen = sizeof(struct sockaddr_storage);
+	int addrcnt = 0;
+	struct sctp_sockaddr_entry *laddr;
+
+	list_for_each_entry_rcu(laddr, &asoc->base.bind_addr.address_list,
+				list)
+		addrcnt++;
+
+	return	  nla_total_size(sizeof(struct tcp_info))

Are you sure you want to use tcp_info ???

net/sctp/sctp_diag.c line 309

Quoted 10 lines

net/sctp/sctp_diag.c new file +10 -0

+		+ nla_total_size(1) /* INET_DIAG_SHUTDOWN */
+		+ nla_total_size(1) /* INET_DIAG_TOS */
+		+ nla_total_size(1) /* INET_DIAG_TCLASS */
+		+ nla_total_size(addrlen * asoc->peer.transport_count)
+		+ nla_total_size(addrlen * addrcnt)
+		+ nla_total_size(sizeof(struct inet_diag_meminfo))
+		+ nla_total_size(sizeof(struct inet_diag_msg))
+		+ nla_total_size(sizeof(struct sctp_info))
+		+ 64;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html