[PATCH net-next v2 0/5] Make tcp-metrics source-address aware

[PATCH net-next v2 0/5] Make tcp-metrics source-address aware

[Christoph Paasch]

Currently tcp-metrics only stores per-destination addresses. This brings
problems, when a host has multiple interfaces (e.g., a smartphone having
WiFi/3G):

For example, a host contacting a server over WiFi will store the tcp-metrics
per destination IP. If then the host contacts the same server over 3G, the
same tcp-metrics will be used, although the path-characteristics are completly
different (e.g., the ssthresh is probably not the same).

In case of TFO this is not a problem, as the server will provide us a new cookie
once he saw our SYN+DATA with an incorrect cookie.
It may be (in case of carrier-grade NAT), that we keep the same public IP but
have a different private IP. Thus, we better reuse the old cookie even if our
source-IP has changed. However, this scenario is probably very uncommon, as 
carriers try to provide the same src-IP to the clients behind their CGN.

Patches 1 + 2 add the source-IP to the tcp metrics.

Patches 3 to 5 modify the netlink-api to support the source-IP. From now on,
when using the command "ip tcp_metrics delete address ADDRESS" all entries
which match this destination IP will be deleted.

Today's iproute2 will complain when doing "ip tcp_metrics flush PREFIX" if
several entries are present for the same destination-IP but with different
source-IPs:

root@client:~/test# ip tcp_metrics
10.2.1.2 age 3.640sec rtt 16250us rttvar 15000us cwnd 10
10.2.1.2 age 4.030sec rtt 18750us rttvar 15000us cwnd 10
root@client:~/test# ip tcp_metrics flush 10.2.1.2/16
Failed to send flush request
: No such process


Follow-up patches will modify iproute2 to handle this correctly and allow
specifying the source-IP in the get/del commands.


v2: Added the patch that allows to selectively get/del of tcp-metrics based
    on src-IP and moved the patch that adds the new netlink attribute before
    the other patches.

Christoph Paasch (5):
  tcp: metrics: rename tcpm_addr to tcpm_daddr
  tcp: metrics: Add source-address to tcp-metrics
  tcp: metrics: New netlink attribute for src IP and dumped in netlink
    reply
  tcp: metrics: Delete all entries matching a certain destination
  tcp: metrics: Allow selective get/del of tcp-metrics based on src IP

 include/uapi/linux/tcp_metrics.h |   2 +
 net/ipv4/tcp_metrics.c           | 151 ++++++++++++++++++++++++++-------------
 2 files changed, 105 insertions(+), 48 deletions(-)

-- 
1.8.3.2

[PATCH net-next v2 1/5] tcp: metrics: rename tcpm_addr to tcpm_daddr

[Christoph Paasch]

As we will add also the source-address, we rename all accesses to the
tcp-metrics address to use "daddr".

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 72 +++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 06493736fbc8..f9b5f519a4ea 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -31,7 +31,7 @@ struct tcp_fastopen_metrics {
 
 struct tcp_metrics_block {
 	struct tcp_metrics_block __rcu	*tcpm_next;
-	struct inetpeer_addr		tcpm_addr;
+	struct inetpeer_addr		tcpm_daddr;
 	unsigned long			tcpm_stamp;
 	u32				tcpm_ts;
 	u32				tcpm_ts_stamp;
@@ -131,7 +131,7 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm, struct dst_entry *dst,
 }
 
 static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
-					  struct inetpeer_addr *addr,
+					  struct inetpeer_addr *daddr,
 					  unsigned int hash,
 					  bool reclaim)
 {
@@ -155,7 +155,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		if (!tm)
 			goto out_unlock;
 	}
-	tm->tcpm_addr = *addr;
+	tm->tcpm_daddr = *daddr;
 
 	tcpm_suck_dst(tm, dst, true);
 
@@ -189,7 +189,7 @@ static struct tcp_metrics_block *tcp_get_encode(struct tcp_metrics_block *tm, in
 	return NULL;
 }
 
-static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *addr,
+static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *daddr,
 						   struct net *net, unsigned int hash)
 {
 	struct tcp_metrics_block *tm;
@@ -197,7 +197,7 @@ static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *a
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, addr))
+		if (addr_same(&tm->tcpm_daddr, daddr))
 			break;
 		depth++;
 	}
@@ -208,19 +208,19 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 						       struct dst_entry *dst)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net;
 
-	addr.family = req->rsk_ops->family;
-	switch (addr.family) {
+	daddr.family = req->rsk_ops->family;
+	switch (daddr.family) {
 	case AF_INET:
-		addr.addr.a4 = inet_rsk(req)->ir_rmt_addr;
-		hash = (__force unsigned int) addr.addr.a4;
+		daddr.addr.a4 = inet_rsk(req)->ir_rmt_addr;
+		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		*(struct in6_addr *)addr.addr.a6 = inet_rsk(req)->ir_v6_rmt_addr;
+		*(struct in6_addr *)daddr.addr.a6 = inet_rsk(req)->ir_v6_rmt_addr;
 		hash = ipv6_addr_hash(&inet_rsk(req)->ir_v6_rmt_addr);
 		break;
 #endif
@@ -233,7 +233,7 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, &addr))
+		if (addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	tcpm_check_stamp(tm, dst);
@@ -243,19 +243,19 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock *tw)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net;
 
-	addr.family = tw->tw_family;
-	switch (addr.family) {
+	daddr.family = tw->tw_family;
+	switch (daddr.family) {
 	case AF_INET:
-		addr.addr.a4 = tw->tw_daddr;
-		hash = (__force unsigned int) addr.addr.a4;
+		daddr.addr.a4 = tw->tw_daddr;
+		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		*(struct in6_addr *)addr.addr.a6 = tw->tw_v6_daddr;
+		*(struct in6_addr *)daddr.addr.a6 = tw->tw_v6_daddr;
 		hash = ipv6_addr_hash(&tw->tw_v6_daddr);
 		break;
 #endif
@@ -268,7 +268,7 @@ static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, &addr))
+		if (addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	return tm;
@@ -279,20 +279,20 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 						 bool create)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net;
 	bool reclaim;
 
-	addr.family = sk->sk_family;
-	switch (addr.family) {
+	daddr.family = sk->sk_family;
+	switch (daddr.family) {
 	case AF_INET:
-		addr.addr.a4 = inet_sk(sk)->inet_daddr;
-		hash = (__force unsigned int) addr.addr.a4;
+		daddr.addr.a4 = inet_sk(sk)->inet_daddr;
+		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		*(struct in6_addr *)addr.addr.a6 = sk->sk_v6_daddr;
+		*(struct in6_addr *)daddr.addr.a6 = sk->sk_v6_daddr;
 		hash = ipv6_addr_hash(&sk->sk_v6_daddr);
 		break;
 #endif
@@ -303,14 +303,14 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 	net = dev_net(dst->dev);
 	hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log);
 
-	tm = __tcp_get_metrics(&addr, net, hash);
+	tm = __tcp_get_metrics(&daddr, net, hash);
 	reclaim = false;
 	if (tm == TCP_METRICS_RECLAIM_PTR) {
 		reclaim = true;
 		tm = NULL;
 	}
 	if (!tm && create)
-		tm = tcpm_new(dst, &addr, hash, reclaim);
+		tm = tcpm_new(dst, &daddr, hash, reclaim);
 	else
 		tcpm_check_stamp(tm, dst);
 
@@ -724,15 +724,15 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
 	struct nlattr *nest;
 	int i;
 
-	switch (tm->tcpm_addr.family) {
+	switch (tm->tcpm_daddr.family) {
 	case AF_INET:
 		if (nla_put_be32(msg, TCP_METRICS_ATTR_ADDR_IPV4,
-				tm->tcpm_addr.addr.a4) < 0)
+				tm->tcpm_daddr.addr.a4) < 0)
 			goto nla_put_failure;
 		break;
 	case AF_INET6:
 		if (nla_put(msg, TCP_METRICS_ATTR_ADDR_IPV6, 16,
-			    tm->tcpm_addr.addr.a6) < 0)
+			    tm->tcpm_daddr.addr.a6) < 0)
 			goto nla_put_failure;
 		break;
 	default:
@@ -882,14 +882,14 @@ static int parse_nl_addr(struct genl_info *info, struct inetpeer_addr *addr,
 static int tcp_metrics_nl_cmd_get(struct sk_buff *skb, struct genl_info *info)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct sk_buff *msg;
 	struct net *net = genl_info_net(info);
 	void *reply;
 	int ret;
 
-	ret = parse_nl_addr(info, &addr, &hash, 0);
+	ret = parse_nl_addr(info, &daddr, &hash, 0);
 	if (ret < 0)
 		return ret;
 
@@ -907,7 +907,7 @@ static int tcp_metrics_nl_cmd_get(struct sk_buff *skb, struct genl_info *info)
 	rcu_read_lock();
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_addr, &addr)) {
+		if (addr_same(&tm->tcpm_daddr, &daddr)) {
 			ret = tcp_metrics_fill_info(msg, tm);
 			break;
 		}
@@ -962,12 +962,12 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	struct tcpm_hash_bucket *hb;
 	struct tcp_metrics_block *tm;
 	struct tcp_metrics_block __rcu **pp;
-	struct inetpeer_addr addr;
+	struct inetpeer_addr daddr;
 	unsigned int hash;
 	struct net *net = genl_info_net(info);
 	int ret;
 
-	ret = parse_nl_addr(info, &addr, &hash, 1);
+	ret = parse_nl_addr(info, &daddr, &hash, 1);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)
@@ -979,7 +979,7 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&tcp_metrics_lock);
 	for (tm = deref_locked_genl(*pp); tm;
 	     pp = &tm->tcpm_next, tm = deref_locked_genl(*pp)) {
-		if (addr_same(&tm->tcpm_addr, &addr)) {
+		if (addr_same(&tm->tcpm_daddr, &daddr)) {
 			*pp = tm->tcpm_next;
 			break;
 		}
-- 
1.8.3.2

[PATCH net-next v2 2/5] tcp: metrics: Add source-address to tcp-metrics

[Christoph Paasch]

We add the source-address to the tcp-metrics, so that different metrics
will be used per source/destination-pair. We use the destination-hash to
store the metric inside the hash-table. That way, deleting and dumping
via "ip tcp_metrics" is easy.

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index f9b5f519a4ea..de32aa41a846 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -31,6 +31,7 @@ struct tcp_fastopen_metrics {
 
 struct tcp_metrics_block {
 	struct tcp_metrics_block __rcu	*tcpm_next;
+	struct inetpeer_addr		tcpm_saddr;
 	struct inetpeer_addr		tcpm_daddr;
 	unsigned long			tcpm_stamp;
 	u32				tcpm_ts;
@@ -131,6 +132,7 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm, struct dst_entry *dst,
 }
 
 static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
+					  struct inetpeer_addr *saddr,
 					  struct inetpeer_addr *daddr,
 					  unsigned int hash,
 					  bool reclaim)
@@ -155,6 +157,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		if (!tm)
 			goto out_unlock;
 	}
+	tm->tcpm_saddr = *saddr;
 	tm->tcpm_daddr = *daddr;
 
 	tcpm_suck_dst(tm, dst, true);
@@ -189,7 +192,8 @@ static struct tcp_metrics_block *tcp_get_encode(struct tcp_metrics_block *tm, in
 	return NULL;
 }
 
-static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *daddr,
+static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *saddr,
+						   const struct inetpeer_addr *daddr,
 						   struct net *net, unsigned int hash)
 {
 	struct tcp_metrics_block *tm;
@@ -197,7 +201,8 @@ static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *d
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, daddr))
+		if (addr_same(&tm->tcpm_saddr, saddr) &&
+		    addr_same(&tm->tcpm_daddr, daddr))
 			break;
 		depth++;
 	}
@@ -208,18 +213,21 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 						       struct dst_entry *dst)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net;
 
+	saddr.family = req->rsk_ops->family;
 	daddr.family = req->rsk_ops->family;
 	switch (daddr.family) {
 	case AF_INET:
+		saddr.addr.a4 = inet_rsk(req)->ir_loc_addr;
 		daddr.addr.a4 = inet_rsk(req)->ir_rmt_addr;
 		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		*(struct in6_addr *)saddr.addr.a6 = inet_rsk(req)->ir_v6_loc_addr;
 		*(struct in6_addr *)daddr.addr.a6 = inet_rsk(req)->ir_v6_rmt_addr;
 		hash = ipv6_addr_hash(&inet_rsk(req)->ir_v6_rmt_addr);
 		break;
@@ -233,7 +241,8 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, &daddr))
+		if (addr_same(&tm->tcpm_saddr, &saddr) &&
+		    addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	tcpm_check_stamp(tm, dst);
@@ -243,18 +252,21 @@ static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,
 static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock *tw)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net;
 
+	saddr.family = tw->tw_family;
 	daddr.family = tw->tw_family;
 	switch (daddr.family) {
 	case AF_INET:
+		saddr.addr.a4 = tw->tw_rcv_saddr;
 		daddr.addr.a4 = tw->tw_daddr;
 		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		*(struct in6_addr *)saddr.addr.a6 = tw->tw_v6_rcv_saddr;
 		*(struct in6_addr *)daddr.addr.a6 = tw->tw_v6_daddr;
 		hash = ipv6_addr_hash(&tw->tw_v6_daddr);
 		break;
@@ -268,7 +280,8 @@ static struct tcp_metrics_block *__tcp_get_metrics_tw(struct inet_timewait_sock
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, &daddr))
+		if (addr_same(&tm->tcpm_saddr, &saddr) &&
+		    addr_same(&tm->tcpm_daddr, &daddr))
 			break;
 	}
 	return tm;
@@ -279,19 +292,22 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 						 bool create)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net;
 	bool reclaim;
 
+	saddr.family = sk->sk_family;
 	daddr.family = sk->sk_family;
 	switch (daddr.family) {
 	case AF_INET:
+		saddr.addr.a4 = inet_sk(sk)->inet_saddr;
 		daddr.addr.a4 = inet_sk(sk)->inet_daddr;
 		hash = (__force unsigned int) daddr.addr.a4;
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
+		*(struct in6_addr *)saddr.addr.a6 = sk->sk_v6_rcv_saddr;
 		*(struct in6_addr *)daddr.addr.a6 = sk->sk_v6_daddr;
 		hash = ipv6_addr_hash(&sk->sk_v6_daddr);
 		break;
@@ -303,14 +319,14 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk,
 	net = dev_net(dst->dev);
 	hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log);
 
-	tm = __tcp_get_metrics(&daddr, net, hash);
+	tm = __tcp_get_metrics(&saddr, &daddr, net, hash);
 	reclaim = false;
 	if (tm == TCP_METRICS_RECLAIM_PTR) {
 		reclaim = true;
 		tm = NULL;
 	}
 	if (!tm && create)
-		tm = tcpm_new(dst, &daddr, hash, reclaim);
+		tm = tcpm_new(dst, &saddr, &daddr, hash, reclaim);
 	else
 		tcpm_check_stamp(tm, dst);
 
-- 
1.8.3.2

[PATCH net-next v2 3/5] tcp: metrics: New netlink attribute for src IP and dumped in netlink reply

[Christoph Paasch]

This patch adds a new netlink attribute for the source-IP and appends it
to the netlink reply. Now, iproute2 can have access to the source-IP.

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 include/uapi/linux/tcp_metrics.h | 2 ++
 net/ipv4/tcp_metrics.c           | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/uapi/linux/tcp_metrics.h b/include/uapi/linux/tcp_metrics.h
index cb5157b55f32..54a37b13f2c4 100644
--- a/include/uapi/linux/tcp_metrics.h
+++ b/include/uapi/linux/tcp_metrics.h
@@ -35,6 +35,8 @@ enum {
 	TCP_METRICS_ATTR_FOPEN_SYN_DROPS,	/* u16, count of drops */
 	TCP_METRICS_ATTR_FOPEN_SYN_DROP_TS,	/* msecs age */
 	TCP_METRICS_ATTR_FOPEN_COOKIE,		/* binary */
+	TCP_METRICS_ATTR_SADDR_IPV4,		/* u32 */
+	TCP_METRICS_ATTR_SADDR_IPV6,		/* binary */
 
 	__TCP_METRICS_ATTR_MAX,
 };
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index de32aa41a846..199659f7a871 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -745,11 +745,17 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
 		if (nla_put_be32(msg, TCP_METRICS_ATTR_ADDR_IPV4,
 				tm->tcpm_daddr.addr.a4) < 0)
 			goto nla_put_failure;
+		if (nla_put_be32(msg, TCP_METRICS_ATTR_SADDR_IPV4,
+				tm->tcpm_saddr.addr.a4) < 0)
+			goto nla_put_failure;
 		break;
 	case AF_INET6:
 		if (nla_put(msg, TCP_METRICS_ATTR_ADDR_IPV6, 16,
 			    tm->tcpm_daddr.addr.a6) < 0)
 			goto nla_put_failure;
+		if (nla_put(msg, TCP_METRICS_ATTR_SADDR_IPV6, 16,
+			    tm->tcpm_saddr.addr.a6) < 0)
+			goto nla_put_failure;
 		break;
 	default:
 		return -EAFNOSUPPORT;
-- 
1.8.3.2

[PATCH net-next v2 4/5] tcp: metrics: Delete all entries matching a certain destination

[Christoph Paasch]

As we now can have multiple entries per destination-IP, the "ip
tcp_metrics delete address ADDRESS" command deletes all of them.

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 199659f7a871..e150f264c8e2 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -982,7 +982,7 @@ static int tcp_metrics_flush_all(struct net *net)
 static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 {
 	struct tcpm_hash_bucket *hb;
-	struct tcp_metrics_block *tm;
+	struct tcp_metrics_block *tm, *tmlist = NULL;
 	struct tcp_metrics_block __rcu **pp;
 	struct inetpeer_addr daddr;
 	unsigned int hash;
@@ -999,17 +999,22 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	hb = net->ipv4.tcp_metrics_hash + hash;
 	pp = &hb->chain;
 	spin_lock_bh(&tcp_metrics_lock);
-	for (tm = deref_locked_genl(*pp); tm;
-	     pp = &tm->tcpm_next, tm = deref_locked_genl(*pp)) {
+	for (tm = deref_locked_genl(*pp); tm; tm = deref_locked_genl(*pp)) {
 		if (addr_same(&tm->tcpm_daddr, &daddr)) {
 			*pp = tm->tcpm_next;
-			break;
+			tm->tcpm_next = tmlist;
+			tmlist = tm;
+		} else {
+			pp = &tm->tcpm_next;
 		}
 	}
 	spin_unlock_bh(&tcp_metrics_lock);
-	if (!tm)
+	if (!tmlist)
 		return -ESRCH;
-	kfree_rcu(tm, rcu_head);
+	for (tm = tmlist; tm; tm = tmlist) {
+		tmlist = tm->tcpm_next;
+		kfree_rcu(tm, rcu_head);
+	}
 	return 0;
 }
 
-- 
1.8.3.2

[PATCH net-next v2 5/5] tcp: metrics: Allow selective get/del of tcp-metrics based on src IP

[Christoph Paasch]

We want to be able to get/del tcp-metrics based on the src IP. This
patch adds the necessary parsing of the netlink attribute and if the
source address is set, it will match on this one too.

Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
 net/ipv4/tcp_metrics.c | 48 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index e150f264c8e2..699a42faab9c 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -877,44 +877,66 @@ done:
 	return skb->len;
 }
 
-static int parse_nl_addr(struct genl_info *info, struct inetpeer_addr *addr,
-			 unsigned int *hash, int optional)
+static int __parse_nl_addr(struct genl_info *info, struct inetpeer_addr *addr,
+			   unsigned int *hash, int optional, int v4, int v6)
 {
 	struct nlattr *a;
 
-	a = info->attrs[TCP_METRICS_ATTR_ADDR_IPV4];
+	a = info->attrs[v4];
 	if (a) {
 		addr->family = AF_INET;
 		addr->addr.a4 = nla_get_be32(a);
-		*hash = (__force unsigned int) addr->addr.a4;
+		if (hash)
+			*hash = (__force unsigned int) addr->addr.a4;
 		return 0;
 	}
-	a = info->attrs[TCP_METRICS_ATTR_ADDR_IPV6];
+	a = info->attrs[v6];
 	if (a) {
 		if (nla_len(a) != sizeof(struct in6_addr))
 			return -EINVAL;
 		addr->family = AF_INET6;
 		memcpy(addr->addr.a6, nla_data(a), sizeof(addr->addr.a6));
-		*hash = ipv6_addr_hash((struct in6_addr *) addr->addr.a6);
+		if (hash)
+			*hash = ipv6_addr_hash((struct in6_addr *) addr->addr.a6);
 		return 0;
 	}
 	return optional ? 1 : -EAFNOSUPPORT;
 }
 
+static int parse_nl_addr(struct genl_info *info, struct inetpeer_addr *addr,
+			 unsigned int *hash, int optional)
+{
+	return __parse_nl_addr(info, addr, hash, optional,
+			       TCP_METRICS_ATTR_ADDR_IPV4,
+			       TCP_METRICS_ATTR_ADDR_IPV6);
+}
+
+static int parse_nl_saddr(struct genl_info *info, struct inetpeer_addr *addr)
+{
+	return __parse_nl_addr(info, addr, NULL, 0,
+			       TCP_METRICS_ATTR_SADDR_IPV4,
+			       TCP_METRICS_ATTR_SADDR_IPV6);
+}
+
 static int tcp_metrics_nl_cmd_get(struct sk_buff *skb, struct genl_info *info)
 {
 	struct tcp_metrics_block *tm;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct sk_buff *msg;
 	struct net *net = genl_info_net(info);
 	void *reply;
 	int ret;
+	bool src = true;
 
 	ret = parse_nl_addr(info, &daddr, &hash, 0);
 	if (ret < 0)
 		return ret;
 
+	ret = parse_nl_saddr(info, &saddr);
+	if (ret < 0)
+		src = false;
+
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
@@ -929,7 +951,8 @@ static int tcp_metrics_nl_cmd_get(struct sk_buff *skb, struct genl_info *info)
 	rcu_read_lock();
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
-		if (addr_same(&tm->tcpm_daddr, &daddr)) {
+		if (addr_same(&tm->tcpm_daddr, &daddr) &&
+		    (!src || addr_same(&tm->tcpm_saddr, &saddr))) {
 			ret = tcp_metrics_fill_info(msg, tm);
 			break;
 		}
@@ -984,23 +1007,28 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	struct tcpm_hash_bucket *hb;
 	struct tcp_metrics_block *tm, *tmlist = NULL;
 	struct tcp_metrics_block __rcu **pp;
-	struct inetpeer_addr daddr;
+	struct inetpeer_addr saddr, daddr;
 	unsigned int hash;
 	struct net *net = genl_info_net(info);
 	int ret;
+	bool src = true;
 
 	ret = parse_nl_addr(info, &daddr, &hash, 1);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)
 		return tcp_metrics_flush_all(net);
+	ret = parse_nl_saddr(info, &saddr);
+	if (ret < 0)
+		src = false;
 
 	hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log);
 	hb = net->ipv4.tcp_metrics_hash + hash;
 	pp = &hb->chain;
 	spin_lock_bh(&tcp_metrics_lock);
 	for (tm = deref_locked_genl(*pp); tm; tm = deref_locked_genl(*pp)) {
-		if (addr_same(&tm->tcpm_daddr, &daddr)) {
+		if (addr_same(&tm->tcpm_daddr, &daddr) &&
+		    (!src || addr_same(&tm->tcpm_saddr, &saddr))) {
 			*pp = tm->tcpm_next;
 			tm->tcpm_next = tmlist;
 			tmlist = tm;
-- 
1.8.3.2

Re: [PATCH net-next v2 2/5] tcp: metrics: Add source-address to tcp-metrics

[Eric Dumazet]

On Wed, 2014-01-08 at 16:05 +0100, Christoph Paasch wrote:
> We add the source-address to the tcp-metrics, so that different metrics
> will be used per source/destination-pair. We use the destination-hash to
> store the metric inside the hash-table. That way, deleting and dumping
> via "ip tcp_metrics" is easy.

Note that this has the following problem :

Some applications use a set of source IP addresses to overcome the 64K
port limitation.

tcp_metrics uses a hard-coded TCP_METRICS_RECLAIM_DEPTH value of 5,
meaning that cache wont be able to store more than 5 source IP addresses
(reaching one particular remote IP).

Re: [PATCH net-next v2 2/5] tcp: metrics: Add source-address to tcp-metrics

[Christoph Paasch]

Hello Eric,

On 08/01/14 - 09:55:51, Eric Dumazet wrote:
> On Wed, 2014-01-08 at 16:05 +0100, Christoph Paasch wrote:
> > We add the source-address to the tcp-metrics, so that different metrics
> > will be used per source/destination-pair. We use the destination-hash to
> > store the metric inside the hash-table. That way, deleting and dumping
> > via "ip tcp_metrics" is easy.
> 
> Note that this has the following problem :
> 
> Some applications use a set of source IP addresses to overcome the 64K
> port limitation.

Ok, did not know about that.

> tcp_metrics uses a hard-coded TCP_METRICS_RECLAIM_DEPTH value of 5,
> meaning that cache wont be able to store more than 5 source IP addresses
> (reaching one particular remote IP).

Maybe we could do something like the below (yet untested). That way we allow
up to 32 entries with the same destination but different source and still
only 5 with different destinations.

I guess 32 * 64K connections is enough. :)
We could also make TCP_METRICS_RECLAIM_DEPTH(_DST) a tunable.


diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 699a42faab9c..0418ac318e7d 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -181,13 +181,18 @@ static void tcpm_check_stamp(struct tcp_metrics_block *tm, struct dst_entry *dst
 }
 
 #define TCP_METRICS_RECLAIM_DEPTH	5
+#define TCP_METRICS_RECLAIM_DEPTH_DST	32
 #define TCP_METRICS_RECLAIM_PTR		(struct tcp_metrics_block *) 0x1UL
 
-static struct tcp_metrics_block *tcp_get_encode(struct tcp_metrics_block *tm, int depth)
+static struct tcp_metrics_block *tcp_get_encode(struct tcp_metrics_block *tm,
+						int depth_general,
+						int depth_dst)
 {
 	if (tm)
 		return tm;
-	if (depth > TCP_METRICS_RECLAIM_DEPTH)
+	if (depth_general > TCP_METRICS_RECLAIM_DEPTH)
+		return TCP_METRICS_RECLAIM_PTR;
+	if (depth_dst > TCP_METRICS_RECLAIM_DEPTH_DST)
 		return TCP_METRICS_RECLAIM_PTR;
 	return NULL;
 }
@@ -197,16 +202,19 @@ static struct tcp_metrics_block *__tcp_get_metrics(const struct inetpeer_addr *s
 						   struct net *net, unsigned int hash)
 {
 	struct tcp_metrics_block *tm;
-	int depth = 0;
+	int depth_dst = 0, depth_general = 0;
 
 	for (tm = rcu_dereference(net->ipv4.tcp_metrics_hash[hash].chain); tm;
 	     tm = rcu_dereference(tm->tcpm_next)) {
 		if (addr_same(&tm->tcpm_saddr, saddr) &&
 		    addr_same(&tm->tcpm_daddr, daddr))
 			break;
-		depth++;
+		if (addr_same(&tm->tcpm_daddr, daddr))
+			depth_dst++;
+		else
+			depth_general++;
 	}
-	return tcp_get_encode(tm, depth);
+	return tcp_get_encode(tm, depth_general, depth_dst);
 }
 
 static struct tcp_metrics_block *__tcp_get_metrics_req(struct request_sock *req,

Re: [PATCH net-next v2 2/5] tcp: metrics: Add source-address to tcp-metrics

[Eric Dumazet]

On Wed, 2014-01-08 at 23:43 +0100, Christoph Paasch wrote:
> Hello Eric,
> 
> On 08/01/14 - 09:55:51, Eric Dumazet wrote:
> > On Wed, 2014-01-08 at 16:05 +0100, Christoph Paasch wrote:
> > > We add the source-address to the tcp-metrics, so that different metrics
> > > will be used per source/destination-pair. We use the destination-hash to
> > > store the metric inside the hash-table. That way, deleting and dumping
> > > via "ip tcp_metrics" is easy.
> > 
> > Note that this has the following problem :
> > 
> > Some applications use a set of source IP addresses to overcome the 64K
> > port limitation.
> 
> Ok, did not know about that.
> 
> > tcp_metrics uses a hard-coded TCP_METRICS_RECLAIM_DEPTH value of 5,
> > meaning that cache wont be able to store more than 5 source IP addresses
> > (reaching one particular remote IP).
> 
> Maybe we could do something like the below (yet untested). That way we allow
> up to 32 entries with the same destination but different source and still
> only 5 with different destinations.
> 
> I guess 32 * 64K connections is enough. :)
> We could also make TCP_METRICS_RECLAIM_DEPTH(_DST) a tunable.

Well, not sure if this is a problem anyway, and if we want extra
complexity for this rare use case, considering tcp metrics for
high number of flows sharing a common path is unlikely to be useful
(with exception of Fast Open, but again it must be rare)

Re: [PATCH net-next v2 2/5] tcp: metrics: Add source-address to tcp-metrics

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 08 Jan 2014 15:13:46 -0800

> On Wed, 2014-01-08 at 23:43 +0100, Christoph Paasch wrote:
>> Hello Eric,
>> 
>> On 08/01/14 - 09:55:51, Eric Dumazet wrote:
>> > On Wed, 2014-01-08 at 16:05 +0100, Christoph Paasch wrote:
>> > > We add the source-address to the tcp-metrics, so that different metrics
>> > > will be used per source/destination-pair. We use the destination-hash to
>> > > store the metric inside the hash-table. That way, deleting and dumping
>> > > via "ip tcp_metrics" is easy.
>> > 
>> > Note that this has the following problem :
>> > 
>> > Some applications use a set of source IP addresses to overcome the 64K
>> > port limitation.
>> 
>> Ok, did not know about that.
>> 
>> > tcp_metrics uses a hard-coded TCP_METRICS_RECLAIM_DEPTH value of 5,
>> > meaning that cache wont be able to store more than 5 source IP addresses
>> > (reaching one particular remote IP).
>> 
>> Maybe we could do something like the below (yet untested). That way we allow
>> up to 32 entries with the same destination but different source and still
>> only 5 with different destinations.
>> 
>> I guess 32 * 64K connections is enough. :)
>> We could also make TCP_METRICS_RECLAIM_DEPTH(_DST) a tunable.
> 
> Well, not sure if this is a problem anyway, and if we want extra
> complexity for this rare use case, considering tcp metrics for
> high number of flows sharing a common path is unlikely to be useful
> (with exception of Fast Open, but again it must be rare)

I think we are overthinking this, even for the aforementioned case.

If people report that this is a real problem they are hitting, and
not just with constructed test cases, we can work on a solution.

Re: [PATCH net-next v2 0/5] Make tcp-metrics source-address aware

[David Miller]

From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Wed,  8 Jan 2014 16:05:54 +0100

> Currently tcp-metrics only stores per-destination addresses. This brings
> problems, when a host has multiple interfaces (e.g., a smartphone having
> WiFi/3G):
> 
> For example, a host contacting a server over WiFi will store the tcp-metrics
> per destination IP. If then the host contacts the same server over 3G, the
> same tcp-metrics will be used, although the path-characteristics are completly
> different (e.g., the ssthresh is probably not the same).
> 
> In case of TFO this is not a problem, as the server will provide us a new cookie
> once he saw our SYN+DATA with an incorrect cookie.
> It may be (in case of carrier-grade NAT), that we keep the same public IP but
> have a different private IP. Thus, we better reuse the old cookie even if our
> source-IP has changed. However, this scenario is probably very uncommon, as 
> carriers try to provide the same src-IP to the clients behind their CGN.
> 
> Patches 1 + 2 add the source-IP to the tcp metrics.
> 
> Patches 3 to 5 modify the netlink-api to support the source-IP. From now on,
> when using the command "ip tcp_metrics delete address ADDRESS" all entries
> which match this destination IP will be deleted.
> 
> Today's iproute2 will complain when doing "ip tcp_metrics flush PREFIX" if
> several entries are present for the same destination-IP but with different
> source-IPs:
> 
> root@client:~/test# ip tcp_metrics
> 10.2.1.2 age 3.640sec rtt 16250us rttvar 15000us cwnd 10
> 10.2.1.2 age 4.030sec rtt 18750us rttvar 15000us cwnd 10
> root@client:~/test# ip tcp_metrics flush 10.2.1.2/16
> Failed to send flush request
> : No such process
> 
> 
> Follow-up patches will modify iproute2 to handle this correctly and allow
> specifying the source-IP in the get/del commands.
> 
> 
> v2: Added the patch that allows to selectively get/del of tcp-metrics based
>     on src-IP and moved the patch that adds the new netlink attribute before
>     the other patches.

Looks good, series applied, thanks Christoph.

Re: [PATCH net-next v2 2/5] tcp: metrics: Add source-address to tcp-metrics

[Hannes Frederic Sowa]

On Fri, Jan 10, 2014 at 05:37:11PM -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 08 Jan 2014 15:13:46 -0800
> 
> > On Wed, 2014-01-08 at 23:43 +0100, Christoph Paasch wrote:
> >> Hello Eric,
> >> 
> >> On 08/01/14 - 09:55:51, Eric Dumazet wrote:
> >> > On Wed, 2014-01-08 at 16:05 +0100, Christoph Paasch wrote:
> >> > > We add the source-address to the tcp-metrics, so that different metrics
> >> > > will be used per source/destination-pair. We use the destination-hash to
> >> > > store the metric inside the hash-table. That way, deleting and dumping
> >> > > via "ip tcp_metrics" is easy.
> >> > 
> >> > Note that this has the following problem :
> >> > 
> >> > Some applications use a set of source IP addresses to overcome the 64K
> >> > port limitation.
> >> 
> >> Ok, did not know about that.
> >> 
> >> > tcp_metrics uses a hard-coded TCP_METRICS_RECLAIM_DEPTH value of 5,
> >> > meaning that cache wont be able to store more than 5 source IP addresses
> >> > (reaching one particular remote IP).
> >> 
> >> Maybe we could do something like the below (yet untested). That way we allow
> >> up to 32 entries with the same destination but different source and still
> >> only 5 with different destinations.
> >> 
> >> I guess 32 * 64K connections is enough. :)
> >> We could also make TCP_METRICS_RECLAIM_DEPTH(_DST) a tunable.
> > 
> > Well, not sure if this is a problem anyway, and if we want extra
> > complexity for this rare use case, considering tcp metrics for
> > high number of flows sharing a common path is unlikely to be useful
> > (with exception of Fast Open, but again it must be rare)
> 
> I think we are overthinking this, even for the aforementioned case.
> 
> If people report that this is a real problem they are hitting, and
> not just with constructed test cases, we can work on a solution.

I was thinking if this would be a problem then one could switch to not
use the source address but something like the interface group index as
the additional match. Seems to fit into the weak end host model.

Greetings,

  Hannes