Netdev List
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/8] xdp: make stack perform remove and add selftests
From: Jakub Kicinski @ 2017-12-01 23:08 UTC (permalink / raw)
  To: netdev; +Cc: oss-drivers, daniel, jiri, alexei.starovoitov, Jakub Kicinski

Hi!

The purpose of this series is to add a software model of BPF offloads
to make it easier for everyone to test them and make some of the more
arcane rules and assumptions more clear.

The series starts with 3 patches aiming to make XDP handling in the
drivers less error prone.  Currently driver authors have to remember
to free XDP programs if XDP is active during unregister.  With this
series the core will disable XDP on its own.  It will take place
after close, drivers are not expected to perform reconfiguration
when disabling XDP on a downed device.

Next two patches add the software netdev driver, followed by a python
test which exercises all the corner cases which came to my mind.

Test needs to be run as root.  It will print basic information to
stdout, but can also create a more detailed log of all commands
when --log option is passed.  Log is in Emacs Org-mode format.

  ./tools/testing/selftests/bpf/test_offload.py --log /tmp/log

Last two patches replace the SR-IOV API implementation of dummy.

v3:
 - move the freeing of vfs to release (Phil).
v2:
 - free device from the release function;
 - use bus-based name generatin instead of netdev name.
v1:
 - replace the SR-IOV API implementation of dummy;
 - make the dev_xdp_uninstall() also handle the XDP generic (Daniel).

Jakub Kicinski (8):
  net: xdp: avoid output parameters when querying XDP prog
  net: xdp: report flags program was installed with on query
  net: xdp: make the stack take care of the tear down
  netdevsim: add software driver for testing offloads
  netdevsim: add bpf offload support
  selftests/bpf: add offload test based on netdevsim
  netdevsim: add SR-IOV functionality
  net: dummy: remove fake SR-IOV functionality

 MAINTAINERS                                        |   5 +
 drivers/net/Kconfig                                |  11 +
 drivers/net/Makefile                               |   1 +
 drivers/net/dummy.c                                | 215 +------
 drivers/net/ethernet/broadcom/bnxt/bnxt.c          |   2 -
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |   3 -
 drivers/net/ethernet/netronome/nfp/bpf/main.c      |   7 -
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |   4 +-
 drivers/net/ethernet/qlogic/qede/qede_main.c       |   4 -
 drivers/net/netdevsim/Makefile                     |   7 +
 drivers/net/netdevsim/bpf.c                        | 373 +++++++++++
 drivers/net/netdevsim/netdev.c                     | 502 +++++++++++++++
 drivers/net/netdevsim/netdevsim.h                  |  78 +++
 drivers/net/tun.c                                  |   4 -
 include/linux/netdevice.h                          |   5 +-
 net/core/dev.c                                     |  53 +-
 net/core/rtnetlink.c                               |   6 +-
 tools/testing/selftests/bpf/Makefile               |   5 +-
 tools/testing/selftests/bpf/sample_ret0.c          |   7 +
 tools/testing/selftests/bpf/test_offload.py        | 681 +++++++++++++++++++++
 20 files changed, 1715 insertions(+), 258 deletions(-)
 create mode 100644 drivers/net/netdevsim/Makefile
 create mode 100644 drivers/net/netdevsim/bpf.c
 create mode 100644 drivers/net/netdevsim/netdev.c
 create mode 100644 drivers/net/netdevsim/netdevsim.h
 create mode 100644 tools/testing/selftests/bpf/sample_ret0.c
 create mode 100755 tools/testing/selftests/bpf/test_offload.py

-- 
2.15.0

^ permalink raw reply

* [PATCH net 2/2] tcp: use IPCB instead of TCP_SKB_CB in inet_exact_dif_match()
From: Eric Dumazet @ 2017-12-01 23:08 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, David Ahern, James Morris,
	Casey Schaufler, David Ahern
In-Reply-To: <20171201230813.18776-1-edumazet@google.com>

From: David Ahern <dsahern@gmail.com>

After this fix : ("tcp: add tcp_v4_fill_cb()/tcp_v4_restore_cb()"),
socket lookups happen while skb->cb[] has not been mangled yet by TCP.

Fixes: a04a480d4392 ("net: Require exact match for TCP socket lookups if dif is l3mdev")
Signed-off-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/tcp.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4e09398009c10a72478b43d3cffc24ba01612b91..6998707e81f343ef8d893c0b2ba16db541082230 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -844,12 +844,11 @@ static inline int tcp_v6_sdif(const struct sk_buff *skb)
 }
 #endif
 
-/* TCP_SKB_CB reference means this can not be used from early demux */
 static inline bool inet_exact_dif_match(struct net *net, struct sk_buff *skb)
 {
 #if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
 	if (!net->ipv4.sysctl_tcp_l3mdev_accept &&
-	    skb && ipv4_l3mdev_skb(TCP_SKB_CB(skb)->header.h4.flags))
+	    skb && ipv4_l3mdev_skb(IPCB(skb)->flags))
 		return true;
 #endif
 	return false;
-- 
2.15.0.531.g2ccb3012c9-goog

^ permalink raw reply related

* [PATCH net 1/2] tcp: add tcp_v4_fill_cb()/tcp_v4_restore_cb()
From: Eric Dumazet @ 2017-12-01 23:08 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, David Ahern, James Morris,
	Casey Schaufler
In-Reply-To: <20171201230813.18776-1-edumazet@google.com>

James Morris reported kernel stack corruption bug [1] while
running the SELinux testsuite, and bisected to a recent
commit bffa72cf7f9d ("net: sk_buff rbnode reorg")

We believe this commit is fine, but exposes an older bug.

SELinux code runs from tcp_filter() and might send an ICMP,
expecting IP options to be found in skb->cb[] using regular IPCB placement.

We need to defer TCP mangling of skb->cb[] after tcp_filter() calls.

This patch adds tcp_v4_fill_cb()/tcp_v4_restore_cb() in a very
similar way we added them for IPv6.

[1]
[  339.806024] SELinux: failure in selinux_parse_skb(), unable to parse packet
[  339.822505] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff81745af5
[  339.822505]
[  339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0-rc1-test #15
[  339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS FWKT68A   01/19/2017
[  339.885060] Call Trace:
[  339.896875]  <IRQ>
[  339.908103]  dump_stack+0x63/0x87
[  339.920645]  panic+0xe8/0x248
[  339.932668]  ? ip_push_pending_frames+0x33/0x40
[  339.946328]  ? icmp_send+0x525/0x530
[  339.958861]  ? kfree_skbmem+0x60/0x70
[  339.971431]  __stack_chk_fail+0x1b/0x20
[  339.984049]  icmp_send+0x525/0x530
[  339.996205]  ? netlbl_skbuff_err+0x36/0x40
[  340.008997]  ? selinux_netlbl_err+0x11/0x20
[  340.021816]  ? selinux_socket_sock_rcv_skb+0x211/0x230
[  340.035529]  ? security_sock_rcv_skb+0x3b/0x50
[  340.048471]  ? sk_filter_trim_cap+0x44/0x1c0
[  340.061246]  ? tcp_v4_inbound_md5_hash+0x69/0x1b0
[  340.074562]  ? tcp_filter+0x2c/0x40
[  340.086400]  ? tcp_v4_rcv+0x820/0xa20
[  340.098329]  ? ip_local_deliver_finish+0x71/0x1a0
[  340.111279]  ? ip_local_deliver+0x6f/0xe0
[  340.123535]  ? ip_rcv_finish+0x3a0/0x3a0
[  340.135523]  ? ip_rcv_finish+0xdb/0x3a0
[  340.147442]  ? ip_rcv+0x27c/0x3c0
[  340.158668]  ? inet_del_offload+0x40/0x40
[  340.170580]  ? __netif_receive_skb_core+0x4ac/0x900
[  340.183285]  ? rcu_accelerate_cbs+0x5b/0x80
[  340.195282]  ? __netif_receive_skb+0x18/0x60
[  340.207288]  ? process_backlog+0x95/0x140
[  340.218948]  ? net_rx_action+0x26c/0x3b0
[  340.230416]  ? __do_softirq+0xc9/0x26a
[  340.241625]  ? do_softirq_own_stack+0x2a/0x40
[  340.253368]  </IRQ>
[  340.262673]  ? do_softirq+0x50/0x60
[  340.273450]  ? __local_bh_enable_ip+0x57/0x60
[  340.285045]  ? ip_finish_output2+0x175/0x350
[  340.296403]  ? ip_finish_output+0x127/0x1d0
[  340.307665]  ? nf_hook_slow+0x3c/0xb0
[  340.318230]  ? ip_output+0x72/0xe0
[  340.328524]  ? ip_fragment.constprop.54+0x80/0x80
[  340.340070]  ? ip_local_out+0x35/0x40
[  340.350497]  ? ip_queue_xmit+0x15c/0x3f0
[  340.361060]  ? __kmalloc_reserve.isra.40+0x31/0x90
[  340.372484]  ? __skb_clone+0x2e/0x130
[  340.382633]  ? tcp_transmit_skb+0x558/0xa10
[  340.393262]  ? tcp_connect+0x938/0xad0
[  340.403370]  ? ktime_get_with_offset+0x4c/0xb0
[  340.414206]  ? tcp_v4_connect+0x457/0x4e0
[  340.424471]  ? __inet_stream_connect+0xb3/0x300
[  340.435195]  ? inet_stream_connect+0x3b/0x60
[  340.445607]  ? SYSC_connect+0xd9/0x110
[  340.455455]  ? __audit_syscall_entry+0xaf/0x100
[  340.466112]  ? syscall_trace_enter+0x1d0/0x2b0
[  340.476636]  ? __audit_syscall_exit+0x209/0x290
[  340.487151]  ? SyS_connect+0xe/0x10
[  340.496453]  ? do_syscall_64+0x67/0x1b0
[  340.506078]  ? entry_SYSCALL64_slow_path+0x25/0x25

Fixes: 971f10eca186 ("tcp: better TCP_SKB_CB layout to reduce cache line misses")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: James Morris <james.l.morris@oracle.com>
Tested-by: James Morris <james.l.morris@oracle.com>
Tested-by: Casey Schaufler <casey@schaufler-ca.com>
---
 net/ipv4/tcp_ipv4.c | 59 ++++++++++++++++++++++++++++++++++++-----------------
 net/ipv6/tcp_ipv6.c | 10 +++++----
 2 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
 }
 EXPORT_SYMBOL(tcp_filter);
 
+static void tcp_v4_restore_cb(struct sk_buff *skb)
+{
+	memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
+		sizeof(struct inet_skb_parm));
+}
+
+static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
+			   const struct tcphdr *th)
+{
+	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
+	 * barrier() makes sure compiler wont play fool^Waliasing games.
+	 */
+	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
+		sizeof(struct inet_skb_parm));
+	barrier();
+
+	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
+	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
+				    skb->len - th->doff * 4);
+	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
+	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
+	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
+	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
+	TCP_SKB_CB(skb)->sacked	 = 0;
+	TCP_SKB_CB(skb)->has_rxtstamp =
+			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
+}
+
 /*
  *	From tcp_input.c
  */
@@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
 
 	th = (const struct tcphdr *)skb->data;
 	iph = ip_hdr(skb);
-	/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
-	 * barrier() makes sure compiler wont play fool^Waliasing games.
-	 */
-	memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
-		sizeof(struct inet_skb_parm));
-	barrier();
-
-	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
-	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
-				    skb->len - th->doff * 4);
-	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
-	TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
-	TCP_SKB_CB(skb)->tcp_tw_isn = 0;
-	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
-	TCP_SKB_CB(skb)->sacked	 = 0;
-	TCP_SKB_CB(skb)->has_rxtstamp =
-			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
-
 lookup:
 	sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
 			       th->dest, sdif, &refcounted);
@@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		sock_hold(sk);
 		refcounted = true;
 		nsk = NULL;
-		if (!tcp_filter(sk, skb))
+		if (!tcp_filter(sk, skb)) {
+			th = (const struct tcphdr *)skb->data;
+			iph = ip_hdr(skb);
+			tcp_v4_fill_cb(skb, iph, th);
 			nsk = tcp_check_req(sk, skb, req, false);
+		}
 		if (!nsk) {
 			reqsk_put(req);
 			goto discard_and_relse;
 		}
 		if (nsk == sk) {
 			reqsk_put(req);
+			tcp_v4_restore_cb(skb);
 		} else if (tcp_child_process(sk, nsk, skb)) {
 			tcp_v4_send_reset(nsk, skb);
 			goto discard_and_relse;
@@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		goto discard_and_relse;
 	th = (const struct tcphdr *)skb->data;
 	iph = ip_hdr(skb);
+	tcp_v4_fill_cb(skb, iph, th);
 
 	skb->dev = NULL;
 
@@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
 	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto discard_it;
 
+	tcp_v4_fill_cb(skb, iph, th);
+
 	if (tcp_checksum_complete(skb)) {
 csum_error:
 		__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
@@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		goto discard_it;
 	}
 
+	tcp_v4_fill_cb(skb, iph, th);
+
 	if (tcp_checksum_complete(skb)) {
 		inet_twsk_put(inet_twsk(sk));
 		goto csum_error;
@@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		if (sk2) {
 			inet_twsk_deschedule_put(inet_twsk(sk));
 			sk = sk2;
+			tcp_v4_restore_cb(skb);
 			refcounted = false;
 			goto process;
 		}
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index be11dc13aa705145a83177e17d23594e9416e11a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		struct sock *nsk;
 
 		sk = req->rsk_listener;
-		tcp_v6_fill_cb(skb, hdr, th);
 		if (tcp_v6_inbound_md5_hash(sk, skb)) {
 			sk_drops_add(sk, skb);
 			reqsk_put(req);
@@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		sock_hold(sk);
 		refcounted = true;
 		nsk = NULL;
-		if (!tcp_filter(sk, skb))
+		if (!tcp_filter(sk, skb)) {
+			th = (const struct tcphdr *)skb->data;
+			hdr = ipv6_hdr(skb);
+			tcp_v6_fill_cb(skb, hdr, th);
 			nsk = tcp_check_req(sk, skb, req, false);
+		}
 		if (!nsk) {
 			reqsk_put(req);
 			goto discard_and_relse;
@@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto discard_and_relse;
 
-	tcp_v6_fill_cb(skb, hdr, th);
-
 	if (tcp_v6_inbound_md5_hash(sk, skb))
 		goto discard_and_relse;
 
@@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 		goto discard_and_relse;
 	th = (const struct tcphdr *)skb->data;
 	hdr = ipv6_hdr(skb);
+	tcp_v6_fill_cb(skb, hdr, th);
 
 	skb->dev = NULL;
 
-- 
2.15.0.531.g2ccb3012c9-goog

^ permalink raw reply related

* [PATCH net 0/2] tcp: fix SELinux/Smack corruptions
From: Eric Dumazet @ 2017-12-01 23:08 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, David Ahern, James Morris,
	Casey Schaufler

James Morris reported kernel stack corruption bug that
we tracked back to commit 971f10eca186 ("tcp: better TCP_SKB_CB
layout to reduce cache line misses")

First patch needs to be backported to kernels >= 3.18,
while second patch needs to be backported to kernels >= 4.9, since
this was the time when inet_exact_dif_match appeared.

David Ahern (1):
  tcp: use IPCB instead of TCP_SKB_CB in inet_exact_dif_match()

Eric Dumazet (1):
  tcp: add tcp_v4_fill_cb()/tcp_v4_restore_cb()

 include/net/tcp.h   |  3 +--
 net/ipv4/tcp_ipv4.c | 59 ++++++++++++++++++++++++++++++++++++-----------------
 net/ipv6/tcp_ipv6.c | 10 +++++----
 3 files changed, 47 insertions(+), 25 deletions(-)

-- 
2.15.0.531.g2ccb3012c9-goog

^ permalink raw reply

* [PATCH iproute2 net-next] gre6: add collect metadata support
From: William Tu @ 2017-12-01 23:03 UTC (permalink / raw)
  To: netdev

The patch adds 'external' option to support collect metadata
gre6 tunnel. Example of L3 and L2 gre device:
bash:~# ip link add dev ip6gre123 type ip6gre external
bash:~# ip link add dev ip6gretap123 type ip6gretap external

Signed-off-by: William Tu <u9012063@gmail.com>
---
 ip/link_gre6.c        | 55 ++++++++++++++++++++++++++++++++-------------------
 man/man8/ip-link.8.in |  6 ++++++
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index 0a82eaecf2cd..2cb46ca116d0 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -105,6 +105,7 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u16 encapflags = TUNNEL_ENCAP_FLAG_CSUM6;
 	__u16 encapsport = 0;
 	__u16 encapdport = 0;
+	__u8 metadata = 0;
 	int len;
 	__u32 fwmark = 0;
 	__u32 erspan_idx = 0;
@@ -178,6 +179,9 @@ get_failed:
 		if (greinfo[IFLA_GRE_ENCAP_SPORT])
 			encapsport = rta_getattr_u16(greinfo[IFLA_GRE_ENCAP_SPORT]);
 
+		if (greinfo[IFLA_GRE_COLLECT_METADATA])
+			metadata = 1;
+
 		if (greinfo[IFLA_GRE_ENCAP_DPORT])
 			encapdport = rta_getattr_u16(greinfo[IFLA_GRE_ENCAP_DPORT]);
 
@@ -355,6 +359,8 @@ get_failed:
 			encapflags |= TUNNEL_ENCAP_FLAG_REMCSUM;
 		} else if (strcmp(*argv, "noencap-remcsum") == 0) {
 			encapflags &= ~TUNNEL_ENCAP_FLAG_REMCSUM;
+		} else if (strcmp(*argv, "external") == 0) {
+			metadata = 1;
 		} else if (strcmp(*argv, "fwmark") == 0) {
 			NEXT_ARG();
 			if (strcmp(*argv, "inherit") == 0) {
@@ -388,26 +394,30 @@ get_failed:
 		argc--; argv++;
 	}
 
-	addattr32(n, 1024, IFLA_GRE_IKEY, ikey);
-	addattr32(n, 1024, IFLA_GRE_OKEY, okey);
-	addattr_l(n, 1024, IFLA_GRE_IFLAGS, &iflags, 2);
-	addattr_l(n, 1024, IFLA_GRE_OFLAGS, &oflags, 2);
-	addattr_l(n, 1024, IFLA_GRE_LOCAL, &laddr, sizeof(laddr));
-	addattr_l(n, 1024, IFLA_GRE_REMOTE, &raddr, sizeof(raddr));
-	if (link)
-		addattr32(n, 1024, IFLA_GRE_LINK, link);
-	addattr_l(n, 1024, IFLA_GRE_TTL, &hop_limit, 1);
-	addattr_l(n, 1024, IFLA_GRE_ENCAP_LIMIT, &encap_limit, 1);
-	addattr_l(n, 1024, IFLA_GRE_FLOWINFO, &flowinfo, 4);
-	addattr32(n, 1024, IFLA_GRE_FLAGS, flags);
-	addattr32(n, 1024, IFLA_GRE_FWMARK, fwmark);
-	if (erspan_idx != 0)
-		addattr32(n, 1024, IFLA_GRE_ERSPAN_INDEX, erspan_idx);
-
-	addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
-	addattr16(n, 1024, IFLA_GRE_ENCAP_FLAGS, encapflags);
-	addattr16(n, 1024, IFLA_GRE_ENCAP_SPORT, htons(encapsport));
-	addattr16(n, 1024, IFLA_GRE_ENCAP_DPORT, htons(encapdport));
+	if (!metadata) {
+		addattr32(n, 1024, IFLA_GRE_IKEY, ikey);
+		addattr32(n, 1024, IFLA_GRE_OKEY, okey);
+		addattr_l(n, 1024, IFLA_GRE_IFLAGS, &iflags, 2);
+		addattr_l(n, 1024, IFLA_GRE_OFLAGS, &oflags, 2);
+		addattr_l(n, 1024, IFLA_GRE_LOCAL, &laddr, sizeof(laddr));
+		addattr_l(n, 1024, IFLA_GRE_REMOTE, &raddr, sizeof(raddr));
+		if (link)
+			addattr32(n, 1024, IFLA_GRE_LINK, link);
+		addattr_l(n, 1024, IFLA_GRE_TTL, &hop_limit, 1);
+		addattr_l(n, 1024, IFLA_GRE_ENCAP_LIMIT, &encap_limit, 1);
+		addattr_l(n, 1024, IFLA_GRE_FLOWINFO, &flowinfo, 4);
+		addattr32(n, 1024, IFLA_GRE_FLAGS, flags);
+		addattr32(n, 1024, IFLA_GRE_FWMARK, fwmark);
+		if (erspan_idx != 0)
+			addattr32(n, 1024, IFLA_GRE_ERSPAN_INDEX, erspan_idx);
+
+		addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
+		addattr16(n, 1024, IFLA_GRE_ENCAP_FLAGS, encapflags);
+		addattr16(n, 1024, IFLA_GRE_ENCAP_SPORT, htons(encapsport));
+		addattr16(n, 1024, IFLA_GRE_ENCAP_DPORT, htons(encapdport));
+	} else {
+		addattr_l(n, 1024, IFLA_GRE_COLLECT_METADATA, NULL, 0);
+	}
 
 	return 0;
 }
@@ -426,6 +436,11 @@ static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (!tb)
 		return;
 
+	if (tb[IFLA_GRE_COLLECT_METADATA]) {
+		print_bool(PRINT_ANY, "collect_metadata", "external", true);
+		return;
+	}
+
 	if (tb[IFLA_GRE_FLAGS])
 		flags = rta_getattr_u32(tb[IFLA_GRE_FLAGS]);
 
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index a6a10e577b1f..c9b9bb7b2a4e 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -755,6 +755,8 @@ the following additional arguments are supported:
 .BI "dscp inherit"
 ] [
 .BI dev " PHYS_DEV "
+] [
+.RB [ no ] external
 ]
 
 .in +8
@@ -833,6 +835,10 @@ or
 .IR 00 ".." ff
 when tunneling non-IP packets. The default value is 00.
 
+.sp
+.RB [ no ] external
+- make this tunnel externally controlled (or not, which is the default).
+
 .in -8
 
 .TP
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH net-next] openvswitch: do not propagate headroom updates to internal port
From: Pravin Shelar @ 2017-12-01 22:58 UTC (permalink / raw)
  To: Paolo Abeni
  Cc: Linux Kernel Network Developers, Pravin Shelar, David S. Miller
In-Reply-To: <d8c2af0a398ed201064f39a348a55451bf34cd37.1512052527.git.pabeni@redhat.com>

On Thu, Nov 30, 2017 at 6:35 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> After commit 3a927bc7cf9d ("ovs: propagate per dp max headroom to
> all vports") the need_headroom for the internal vport is updated
> accordingly to the max needed headroom in its datapath.
>
> That avoids the pskb_expand_head() costs when sending/forwarding
> packets towards tunnel devices, at least for some scenarios.
>
> We still require such copy when using the ovs-preferred configuration
> for vxlan tunnels:
>
>     br_int
>   /       \
> tap      vxlan
>            (remote_ip:X)
>
> br_phy
>      \
>     NIC
>
> where the route towards the IP 'X' is via 'br_phy'.
>
> When forwarding traffic from the tap towards the vxlan device, we
> will call pskb_expand_head() in vxlan_build_skb() because
> br-phy->needed_headroom is equal to tun->needed_headroom.
>
> With this change we avoid updating the internal vport needed_headroom,
> so that in the above scenario no head copy is needed, giving 5%
> performance improvement in UDP throughput test.
>
> As a trade-off, packets sent from the internal port towards a tunnel
> device will now experience the head copy overhead. The rationale is
> that the latter use-case is less relevant performance-wise.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Acked-by: Pravin B Shelar <pshelar@ovn.org>

Thanks.

^ permalink raw reply

* Re: [PATCH net-next 1/5] libbpf: add ability to guess program type based on section name
From: Jakub Kicinski @ 2017-12-01 22:46 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Roman Gushchin, netdev, linux-kernel, kernel-team, ast, daniel,
	kafai
In-Reply-To: <614dc433-9445-52df-a202-f7cb1cb79ca1@netronome.com>

On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote:
> Thanks Roman!
> One comment in-line.
> 
> 2017-11-30 13:42 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> > The bpf_prog_load() function will guess program type if it's not
> > specified explicitly. This functionality will be used to implement
> > loading of different programs without asking a user to specify
> > the program type. In first order it will be used by bpftool.
> > 
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 5aa45f89da93..9f2410beaa18 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1721,6 +1721,41 @@ BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT);
> >  BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP);
> >  BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
> >  
> > +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> > +{
> > +	if (!prog->section_name)
> > +		goto err;
> > +
> > +	if (strncmp(prog->section_name, "socket", 6) == 0)
> > +		return BPF_PROG_TYPE_SOCKET_FILTER;
> > +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
> > +		return BPF_PROG_TYPE_KPROBE;
> > +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
> > +		return BPF_PROG_TYPE_KPROBE;
> > +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
> > +		return BPF_PROG_TYPE_TRACEPOINT;
> > +	if (strncmp(prog->section_name, "xdp", 3) == 0)
> > +		return BPF_PROG_TYPE_XDP;
> > +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
> > +		return BPF_PROG_TYPE_PERF_EVENT;
> > +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
> > +		return BPF_PROG_TYPE_CGROUP_SKB;
> > +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
> > +		return BPF_PROG_TYPE_CGROUP_SOCK;
> > +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
> > +		return BPF_PROG_TYPE_CGROUP_DEVICE;
> > +	if (strncmp(prog->section_name, "sockops", 7) == 0)
> > +		return BPF_PROG_TYPE_SOCK_OPS;
> > +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
> > +		return BPF_PROG_TYPE_SK_SKB;  
> 
> I do not really like these hard-coded lengths, maybe we could work out
> something nicer with a bit of pre-processing work? Perhaps something like:
> 
> #define SOCKET_FILTER_SEC_PREFIX "socket"
> #define KPROBE_SEC_PREFIX "kprobe/"
> […]
> 
> #define TRY_TYPE(string, __TYPE)				\
> 	do {							\
> 		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
> 			     sizeof(__TYPE ## _SEC_PREFIX)))	\
> 			return BPF_PROG_TYPE_ ## __TYPE;	\
> 	} while(0);

I like the suggestion, but I think return and goto statements hiding
inside macros are slightly frowned upon in the netdev.  Perhaps just 
a macro that wraps the strncmp() with sizeof would be enough?  Without
the return inside?

> static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> {
> 	if (!prog->section_name)
> 		goto err;
> 
> 	TRY_TYPE(prog->section_name, SOCKET_FILTER);
> 	TRY_TYPE(prog->section_name, KPROBE);
> 	[…]
> 
> err:
> 	pr_warning("…",
> 	           prog->section_name);
> 
> 	return BPF_PROG_TYPE_UNSPEC;
> }

^ permalink raw reply

* Re: [PATCH net-next resubmit 2/2] net: phy: remove generic settings for callbacks config_aneg and read_status from drivers
From: Heiner Kallweit @ 2017-12-01 22:37 UTC (permalink / raw)
  To: David Miller; +Cc: f.fainelli, andrew, netdev@vger.kernel.org
In-Reply-To: <20171201.154239.1972560270159887815.davem@davemloft.net>

Am 01.12.2017 um 21:42 schrieb David Miller:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Thu, 30 Nov 2017 23:47:52 +0100
> 
>> Remove generic settings for callbacks config_aneg and read_status
>> from drivers.
>>
When re-testing I just figured out that in drivers/net/phy/broadcom.c
I mistakenly removed three lines too many.
Do you prefer a fixed version of the patch or just a patch with the
fix?

Sorry, Heiner

>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Applied.
> 

^ permalink raw reply

* Re: [PATCH v2 net-next 3/4] inet: Add a 2nd listener hashtable (port+addr)
From: Eric Dumazet @ 2017-12-01 22:24 UTC (permalink / raw)
  To: Martin KaFai Lau, netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171201205232.3012584-4-kafai@fb.com>

On Fri, 2017-12-01 at 12:52 -0800, Martin KaFai Lau wrote:
> The current listener hashtable is hashed by port only.
> When a process is listening at many IP addresses with the same port
> (e.g.
> [IP1]:443, [IP2]:443... [IPN]:443), the inet[6]_lookup_listener()
> performance is degraded to a link list.  It is prone to syn attack.
> 
> UDP had a similar issue and a second hashtable was added to resolve
> it.
> 
> This patch adds a second hashtable for the listener's sockets.
> The second hashtable is hashed by port and address.
> 
> It cannot reuse the existing skc_portaddr_node which is shared
> with skc_bind_node.  TCP listener needs to use skc_bind_node.
> Instead, this patch adds a hlist_node 'icsk_listen_portaddr_node' to
> the inet_connection_sock which the listener (like TCP) also belongs
> to.
> 
> The new portaddr hashtable may need two lookup (First by IP:PORT.
> Second by INADDR_ANY:PORT if the IP:PORT is a not found).   Hence,
> it implements a similar cut off as UDP such that it will only consult
> the
> new portaddr hashtable if the current port-only hashtable has >10
> sk in the link-list.
> 
> lhash2 and lhash2_mask are added to 'struct inet_hashinfo'.  I take
> this chance to plug a 4 bytes hole.  It is done by first moving
> the existing bind_bucket_cachep up and then add the new
> (int lhash2_mask, *lhash2) after the existing bhash_size.
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>


Nice work, thanks Martin !

Reviewed-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply

* Re: [PATCH net-next 1/5] rhashtable: Don't reset walker table in rhashtable_walk_start
From: Herbert Xu @ 2017-12-01 22:18 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev, rohit
In-Reply-To: <20171201000305.2392-2-tom@quantonium.net>

On Thu, Nov 30, 2017 at 04:03:01PM -0800, Tom Herbert wrote:
> Remove the code that resets the walker table. The walker table should
> only be initialized in the walk init function or when a future table is
> encountered. If the walker table is NULL this is the indication that
> the walk has completed and this information can be used to break a
> multi-call walk in the table (e.g. successive calls to nelink_dump
> that are dumping elements of an rhashtable).
> 
> This also allows us to change rhashtable_walk_start to return void
> since the only error it was returning was -EAGAIN for a table change.
> This patch changes all the callers of rhashtable_walk_start to expect
> void which eliminates logic needed to check the return value for a
> rare condition. Note that -EAGAIN will be returned in a call
> to rhashtable_walk_next which seems to always follow the start
> of the walk so there should be no behavioral change in doing this.
> 
> Signed-off-by: Tom Herbert <tom@quantonium.net>

Doesn't this mean that if a walk encounters a rehash you may end up
missing half or more of the hash table?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* Re: [PATCH net-next v2 7/8] netdevsim: add SR-IOV functionality
From: Jakub Kicinski @ 2017-12-01 22:14 UTC (permalink / raw)
  To: Phil Sutter
  Cc: netdev, davem, daniel, alexei.starovoitov, jiri, oss-drivers,
	Sabrina Dubroca
In-Reply-To: <20171201215828.GV32305@orbyte.nwl.cc>

On Fri, 1 Dec 2017 22:58:29 +0100, Phil Sutter wrote:
> > > > > > +	ret = count;
> > > > > > +exit_unlock:
> > > > > > +	rtnl_unlock();
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}      
> > > > > 
> > > > > [...]
> > > > >     
> > > > > > +static void nsim_free(struct net_device *dev)
> > > > > > +{
> > > > > > +	struct netdevsim *ns = netdev_priv(dev);
> > > > > > +
> > > > > > +	device_unregister(&ns->dev);
> > > > > >  }      
> > > > > 
> > > > > Shouldn't this also kfree(ns->vfconfigs)?    
> > > > 
> > > > It's in uninit, I will move it to release.    
> > > 
> > > Oh, I missed that. If you're certain this won't lead to memleaks, no
> > > objection from my side. :)  
> > 
> > OK, I will respin v3 with the free moved :)  
> 
> So it did leak? I'm glad the traffic I caused wasn't completely
> pointless then. :)

There is a window where it could've been re-enabled and that
would leak, yes.  Thanks for catching it :)

^ permalink raw reply

* Re: [RFC PATCH] net_sched: bulk free tcf_block
From: Cong Wang @ 2017-12-01 22:07 UTC (permalink / raw)
  To: Paolo Abeni
  Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
	David S. Miller
In-Reply-To: <1512126307.3155.26.camel@redhat.com>

On Fri, Dec 1, 2017 at 3:05 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>
> Thank you for the feedback.
>
> I tested your patch and in the above scenario I measure:
>
> real    0m0.017s
> user    0m0.000s
> sys     0m0.017s
>
> so it apparently works well for this case.

Thanks a lot for testing it! I will test it further. If it goes well I will
send a formal patch with your Tested-by unless you object it.


>
> We could still have a storm of rtnl lock/unlock operations while
> deleting a large tc tree with lot of filters, and I think we can reduce
> them with bulk free, evenutally applying it to filters, too.
>
> That will also reduce the pressure on the rtnl lock when e.g. OVS H/W
> offload pushes a lot of rules/sec.
>
> WDYT?
>

Why this is specific to tc filter? From what you are saying, we need to
batch all TC operations (qdisc, filter and action) rather than just filter?

In short term, I think batching rtnl lock/unlock is a good optimization,
so I have no objection. For long term, I think we need to revise RTNL
lock and probably move it down to each layer, but clearly it requires
much more work.

Thanks.

^ permalink raw reply

* Re: [PATCH net-next v2 7/8] netdevsim: add SR-IOV functionality
From: Phil Sutter @ 2017-12-01 21:58 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, davem, daniel, alexei.starovoitov, jiri, oss-drivers,
	Sabrina Dubroca
In-Reply-To: <20171201134509.2b1dc343@cakuba.netronome.com>

On Fri, Dec 01, 2017 at 01:45:09PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 22:36:52 +0100, Phil Sutter wrote:
> > On Fri, Dec 01, 2017 at 12:14:07PM -0800, Jakub Kicinski wrote:
> > > On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:  
> > > > On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> > > > [...]  
> > > > > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > > > > +{
> > > > > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > > > > +				GFP_KERNEL);
> > > > > +	if (!ns->vfconfigs)
> > > > > +		return -ENOMEM;
> > > > > +	ns->num_vfs = num_vfs;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void nsim_vfs_disable(struct netdevsim *ns)
> > > > > +{
> > > > > +	kfree(ns->vfconfigs);
> > > > > +	ns->vfconfigs = NULL;
> > > > > +	ns->num_vfs = 0;
> > > > > +}    
> > > > 
> > > > Why not something like:
> > > > 
> > > > | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> > > > | {
> > > > | 	void *ptr = krealloc(ns->vfconfigs,
> > > > | 			     num_vfs * sizeof(struct nsim_vf_config),
> > > > | 			     GFP_KERNEL);
> > > > | 
> > > > | 	if (!ptr)
> > > > | 		return -ENOMEM;
> > > > | 
> > > > | 	ns->vfconfigs = ptr;
> > > > | 	ns->num_vfs = num_vfs;
> > > > | 	return 0;
> > > > | }  
> > > 
> > > Um.  It either frees or allocates, never reallocates so I felt realloc
> > > is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
> > > will have to specify __GFP_ZERO.  It's not a calloc so there could be
> > > potentially some overflows?  
> > 
> > I don't understand: How can overflows happen if I use malloc() instead
> > of calloc()?
> 
> The multiplication may overflow.  That's why we have kmalloc_array().
> Note this explicit check in kmalloc_array() (which is also called by
> kcalloc):
> 
> 	if (size != 0 && n > SIZE_MAX / size)
> 		return NULL;
> 
> Where:
> 
> #define SIZE_MAX	(~(size_t)0)

Ah, I see. Thanks for educating me on this!

> > > > > +	ret = count;
> > > > > +exit_unlock:
> > > > > +	rtnl_unlock();
> > > > > +
> > > > > +	return ret;
> > > > > +}    
> > > > 
> > > > [...]
> > > >   
> > > > > +static void nsim_free(struct net_device *dev)
> > > > > +{
> > > > > +	struct netdevsim *ns = netdev_priv(dev);
> > > > > +
> > > > > +	device_unregister(&ns->dev);
> > > > >  }    
> > > > 
> > > > Shouldn't this also kfree(ns->vfconfigs)?  
> > > 
> > > It's in uninit, I will move it to release.  
> > 
> > Oh, I missed that. If you're certain this won't lead to memleaks, no
> > objection from my side. :)
> 
> OK, I will respin v3 with the free moved :)

So it did leak? I'm glad the traffic I caused wasn't completely
pointless then. :)

Thanks, Phil

^ permalink raw reply

* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
From: Cong Wang @ 2017-12-01 21:46 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <20171201175619.GC2396@nanopsycho>

On Fri, Dec 1, 2017 at 9:56 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>
> Isn't this here so user may specify a ifindex of netdev which is not yet
> present on the system (not sure how much sense that would make though...)

How is this even possible? If an ifindex is not present, we return ENODEV:

        if (parm->ifindex) {
                dev = __dev_get_by_index(net, parm->ifindex);
                if (dev == NULL) {
                        if (exists)
                                tcf_idr_release(*a, bind);
                        return -ENODEV;
                }

^ permalink raw reply

* Re: [PATCH net-next v2 7/8] netdevsim: add SR-IOV functionality
From: Jakub Kicinski @ 2017-12-01 21:45 UTC (permalink / raw)
  To: Phil Sutter
  Cc: netdev, davem, daniel, alexei.starovoitov, jiri, oss-drivers,
	Sabrina Dubroca
In-Reply-To: <20171201213652.GT32305@orbyte.nwl.cc>

On Fri, 1 Dec 2017 22:36:52 +0100, Phil Sutter wrote:
> On Fri, Dec 01, 2017 at 12:14:07PM -0800, Jakub Kicinski wrote:
> > On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:  
> > > On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> > > [...]  
> > > > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > > > +{
> > > > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > > > +				GFP_KERNEL);
> > > > +	if (!ns->vfconfigs)
> > > > +		return -ENOMEM;
> > > > +	ns->num_vfs = num_vfs;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void nsim_vfs_disable(struct netdevsim *ns)
> > > > +{
> > > > +	kfree(ns->vfconfigs);
> > > > +	ns->vfconfigs = NULL;
> > > > +	ns->num_vfs = 0;
> > > > +}    
> > > 
> > > Why not something like:
> > > 
> > > | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> > > | {
> > > | 	void *ptr = krealloc(ns->vfconfigs,
> > > | 			     num_vfs * sizeof(struct nsim_vf_config),
> > > | 			     GFP_KERNEL);
> > > | 
> > > | 	if (!ptr)
> > > | 		return -ENOMEM;
> > > | 
> > > | 	ns->vfconfigs = ptr;
> > > | 	ns->num_vfs = num_vfs;
> > > | 	return 0;
> > > | }  
> > 
> > Um.  It either frees or allocates, never reallocates so I felt realloc
> > is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
> > will have to specify __GFP_ZERO.  It's not a calloc so there could be
> > potentially some overflows?  
> 
> I don't understand: How can overflows happen if I use malloc() instead
> of calloc()?

The multiplication may overflow.  That's why we have kmalloc_array().
Note this explicit check in kmalloc_array() (which is also called by
kcalloc):

	if (size != 0 && n > SIZE_MAX / size)
		return NULL;

Where:

#define SIZE_MAX	(~(size_t)0)

> > > > +	ret = count;
> > > > +exit_unlock:
> > > > +	rtnl_unlock();
> > > > +
> > > > +	return ret;
> > > > +}    
> > > 
> > > [...]
> > >   
> > > > +static void nsim_free(struct net_device *dev)
> > > > +{
> > > > +	struct netdevsim *ns = netdev_priv(dev);
> > > > +
> > > > +	device_unregister(&ns->dev);
> > > >  }    
> > > 
> > > Shouldn't this also kfree(ns->vfconfigs)?  
> > 
> > It's in uninit, I will move it to release.  
> 
> Oh, I missed that. If you're certain this won't lead to memleaks, no
> objection from my side. :)

OK, I will respin v3 with the free moved :)

^ permalink raw reply

* Re: [PATCH net-next v2 8/8] net: dummy: remove fake SR-IOV functionality
From: Phil Sutter @ 2017-12-01 21:41 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, davem, daniel, alexei.starovoitov, jiri, oss-drivers,
	Sabrina Dubroca
In-Reply-To: <20171201121952.144c64db@cakuba.netronome.com>

On Fri, Dec 01, 2017 at 12:19:52PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 14:46:34 +0100, Phil Sutter wrote:
> > On Thu, Nov 30, 2017 at 05:35:40PM -0800, Jakub Kicinski wrote:
> > > netdevsim driver seems like a better place for fake SR-IOV
> > > functionality.  Remove the code previously added to dummy.
> > > 
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>  
> > 
> > Acked-by: Phil Sutter <phil@nwl.cc>
> 
> Thanks!
> 
> Did you have an opportunity to run your tests against this?  I didn't
> find anything that uses dummy's SR-IOV in selftests.

In fact, at Red Hat nobody uses dummy for iproute SR-IOV testing yet
(which was the motivation for it in the first place). Hence why I didn't
see a problem with moving it from dummy over to something else.

Hopefully upstream iproute will at some point contain a testsuite which
makes use of this, but sadly that's still wishful thinking. :(

Cheers, Phil

^ permalink raw reply

* Re: [PATCH net-next v2 7/8] netdevsim: add SR-IOV functionality
From: Phil Sutter @ 2017-12-01 21:36 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, davem, daniel, alexei.starovoitov, jiri, oss-drivers,
	Sabrina Dubroca
In-Reply-To: <20171201121407.4f802f18@cakuba.netronome.com>

On Fri, Dec 01, 2017 at 12:14:07PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:
> > On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> > [...]
> > > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > > +{
> > > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > > +				GFP_KERNEL);
> > > +	if (!ns->vfconfigs)
> > > +		return -ENOMEM;
> > > +	ns->num_vfs = num_vfs;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void nsim_vfs_disable(struct netdevsim *ns)
> > > +{
> > > +	kfree(ns->vfconfigs);
> > > +	ns->vfconfigs = NULL;
> > > +	ns->num_vfs = 0;
> > > +}  
> > 
> > Why not something like:
> > 
> > | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> > | {
> > | 	void *ptr = krealloc(ns->vfconfigs,
> > | 			     num_vfs * sizeof(struct nsim_vf_config),
> > | 			     GFP_KERNEL);
> > | 
> > | 	if (!ptr)
> > | 		return -ENOMEM;
> > | 
> > | 	ns->vfconfigs = ptr;
> > | 	ns->num_vfs = num_vfs;
> > | 	return 0;
> > | }
> 
> Um.  It either frees or allocates, never reallocates so I felt realloc
> is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
> will have to specify __GFP_ZERO.  It's not a calloc so there could be
> potentially some overflows?

I don't understand: How can overflows happen if I use malloc() instead
of calloc()?

> > > +static ssize_t
> > > +nsim_numvfs_store(struct device *dev, struct device_attribute *attr,
> > > +		  const char *buf, size_t count)
> > > +{
> > > +	struct netdevsim *ns = to_nsim(dev);
> > > +	unsigned int num_vfs;
> > > +	int ret;
> > > +
> > > +	ret = kstrtouint(buf, 0, &num_vfs);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	rtnl_lock();
> > > +	if (ns->num_vfs == num_vfs)
> > > +		goto exit_good;  
> > 
> > Then replace this:
> > 
> > > +	if (ns->num_vfs && num_vfs) {
> > > +		ret = -EBUSY;
> > > +		goto exit_unlock;
> > > +	}
> > > +
> > > +	if (num_vfs) {
> > > +		ret = nsim_vfs_enable(ns, num_vfs);
> > > +		if (ret)
> > > +			goto exit_unlock;
> > > +	} else {
> > > +		nsim_vfs_disable(ns);
> > > +	}  
> > 
> > with just:
> > 
> > |	nsim_vfs_set(ns, num_vfs);
> 
> I'm trying to mirror the PCI subsystem behaviour here, which only
> allows enable or disable, not increase.  I felt we should follow how
> real devices behave:
> 
> 	/* enable VFs */
> 	if (pdev->sriov->num_VFs) {
> 		dev_warn(&pdev->dev, "%d VFs already enabled. Disable before enabling %d VFs\n",
> 			 pdev->sriov->num_VFs, num_vfs);
> 		return -EBUSY;
> 	}
> 
> So IOW this is intentional.

Ah, I see. Yes, then it makes sense! Keeping this virtual VF
functionality as close to real ones as possible is certainly feasible.

> > > +	ret = count;
> > > +exit_unlock:
> > > +	rtnl_unlock();
> > > +
> > > +	return ret;
> > > +}  
> > 
> > [...]
> > 
> > > +static void nsim_free(struct net_device *dev)
> > > +{
> > > +	struct netdevsim *ns = netdev_priv(dev);
> > > +
> > > +	device_unregister(&ns->dev);
> > >  }  
> > 
> > Shouldn't this also kfree(ns->vfconfigs)?
> 
> It's in uninit, I will move it to release.

Oh, I missed that. If you're certain this won't lead to memleaks, no
objection from my side. :)

Cheers, Phil

^ permalink raw reply

* Re: [PATCH net-next 00/11] net: ethernet: ti: cpsw/ale clean up and optimization
From: David Miller @ 2017-12-01 21:36 UTC (permalink / raw)
  To: grygorii.strashko
  Cc: netdev, nsekhar, linux-kernel, linux-omap, ivan.khoronzhuk,
	m-karicheri2
In-Reply-To: <20171201002120.13483-1-grygorii.strashko@ti.com>

From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Thu, 30 Nov 2017 18:21:09 -0600

> This is set of non critical clean ups and optimizations for TI
> CPSW and ALE drivers.
> 
> Rebased on top on net-next.

Series applied, thank you.

^ permalink raw reply

* Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info_v1'
From: Daniel Borkmann @ 2017-12-01 21:34 UTC (permalink / raw)
  To: Al Viro, Kees Cook
  Cc: Shmulik Ladkani, Willem de Bruijn, Pablo Neira Ayuso,
	Linus Torvalds, David Miller, LKML, Network Development,
	Christoph Hellwig, Thomas Garnier, Jann Horn
In-Reply-To: <20171201034859.GN21978@ZenIV.linux.org.uk>

On 12/01/2017 04:48 AM, Al Viro wrote:
> On Fri, Dec 01, 2017 at 01:33:04AM +0000, Al Viro wrote:
> 
>> Use of file descriptors should be limited to "got a number from userland,
>> convert to struct file *" on the way in and "install struct file * into
>> descriptor table and return the descriptor to userland" on the way out.
>> And the latter - *ONLY* after the last possible point of failure.  Once
>> a file reference is inserted into descriptor table, that's it - you
>> can't undo that.
>>
>> The only way to use bpf_obj_get_user() is to pass its return value to
>> userland.  As return value of syscall - not even put_user() (for that
>> you'd need to reserve the descriptor, copy it to userland and only
>> then attach struct file * to it).
>>
>> The whole approach stinks - what it needs is something that would
>> take struct filename * and return struct bpf_prog * or struct file *
>> reference.  With bpf_obj_get_user() and this thing implemented
>> via that.

Agree, the "fix" is completely buggy due to fd being exposed to user
space during that period of time ...

>> I'm looking into that thing...
> 
> What it tries to pull off is something not far from
> 
> static struct bpf_prog *__get_prog(struct inode *inode, enum bpf_prog_type type)
> {
> 	struct bpf_prog *prog;
> 	int err = inode_permission(inode, FMODE_READ | FMODE_WRITE);
> 	if (err)
> 		return ERR_PTR(err);
> 
> 	if (inode->i_op == &bpf_map_iops)
> 		return ERR_PTR(-EINVAL);
> 
> 	if (inode->i_op != &bpf_prog_iops)
> 		return ERR_PTR(-EACCES);
> 
> 	prog = inode->i_private;
> 	err = security_bpf_prog(prog);
> 	if (err < 0)
> 		return ERR_PTR(err);
> 
> 	if (!bpf_prog_get_ok(prog, &type, false))
> 		return ERR_PTR(-EINVAL);
> 
> 	return bpf_prog_inc(prog);
> }
> 
> struct bpf_prog *get_prog_path_type(const char *name, enum bpf_prog_type type)
> {
> 	struct path path;
> 	struct bpf_prog *prog;
> 	int err = kern_path(name, LOOKUP_FOLLOW, &path);
> 	if (err)
> 		return ERR_PTR(err);
> 	prog = __get_prog(d_backing_inode(path.dentry), type);
> 	if (!IS_ERR(prog))
> 		touch_atime(&path);
> 	path_put(&path);
> 	return prog;
> }
> 
> static int __bpf_mt_check_path(const char *path, struct bpf_prog **ret)
> {
> 	*ret = get_prog_path_type(path, BPF_PROG_TYPE_SOCKET_FILTER);
>         return PTR_ERR_OR_ZERO(*ret);
> }
> 
> That skips all tracepoint random shite (pardon the triple redundance) and makes
> a somewhat arbitrary change for touch_atime() logics.  And, of course, it is
> not even compile-tested.
> 
> Something similar to get_prog_path_type() above might make for a usable
> primitive, IMO...

The above looks good to me!

^ permalink raw reply

* Re: [PATCH net-next V2 1/2] net-next: use five-tuple hash for sk_txhash
From: Tom Herbert @ 2017-12-01 21:15 UTC (permalink / raw)
  To: Shaohua Li
  Cc: Linux Kernel Network Developers, David S. Miller, Martin Lau,
	Eric Dumazet, flo, Cong Wang, Shaohua Li
In-Reply-To: <e52c22258a70b5b4adfdc96362992f8a285a9a3c.1512161808.git.shli@fb.com>

On Fri, Dec 1, 2017 at 1:00 PM, Shaohua Li <shli@kernel.org> wrote:
> From: Shaohua Li <shli@fb.com>
>
> We are using sk_txhash to calculate flowlabel, but sk_txhash isn't
> always available, for example, in inet_timewait_sock. This causes
> problem for reset packet, which will have a different flowlabel. This
> causes our router doesn't correctly close tcp connection. We are using
> flowlabel to do load balance. Routers in the path maintain connection
> state. So if flow label changes, the packet is routed through a
> different router. In this case, the old router doesn't get the reset
> packet to close the tcp connection.
>
> Per Tom's suggestion, we switch back to five-tuple hash, so we can
> reconstruct correct flowlabel for reset packet.
>
Thanks for doing this!

> At most places, we already have the flowi info, so we directly use it
> build sk_txhash. For synack, we do this after route search. At that
> time, we have the flowi info ready, so don't need to create the flowi
> info again.
>
> I don't change sk_rethink_txhash() though, it still uses random hash,
> which is the whole point to select a different path after a negative
> routing advise.
>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Florent Fourcot <flo@fourcot.fr>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Tom Herbert <tom@herbertland.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  include/net/sock.h    | 18 ++++--------------
>  include/net/tcp.h     |  2 +-
>  net/ipv4/datagram.c   |  2 +-
>  net/ipv4/syncookies.c |  4 +++-
>  net/ipv4/tcp_input.c  |  1 -
>  net/ipv4/tcp_ipv4.c   | 17 ++++++++++++-----
>  net/ipv4/tcp_output.c |  1 -
>  net/ipv6/datagram.c   |  4 +++-
>  net/ipv6/syncookies.c |  3 ++-
>  net/ipv6/tcp_ipv6.c   | 18 +++++++++++++-----
>  10 files changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 79e1a2c..640db0f 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1729,22 +1729,12 @@ static inline kuid_t sock_net_uid(const struct net *net, const struct sock *sk)
>         return sk ? sk->sk_uid : make_kuid(net->user_ns, 0);
>  }
>
> -static inline u32 net_tx_rndhash(void)
> -{
> -       u32 v = prandom_u32();
> -
> -       return v ?: 1;
> -}
> -
> -static inline void sk_set_txhash(struct sock *sk)
> -{
> -       sk->sk_txhash = net_tx_rndhash();
> -}
> -
>  static inline void sk_rethink_txhash(struct sock *sk)
>  {
> -       if (sk->sk_txhash)
> -               sk_set_txhash(sk);
> +       if (sk->sk_txhash) {
> +               u32 v = prandom_u32();
> +               sk->sk_txhash = v ?: 1;
> +       }

We'll need to add configuration about whether rethink is done at all.
Conservative approach is probably to disable it by default. That is
the default behavior of the stack is that flow label is consistent for
lifetime of a flow.

>  }
>
>  static inline struct dst_entry *
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4e09398..a5c28be 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1840,7 +1840,7 @@ struct tcp_request_sock_ops {
>                                  __u16 *mss);
>  #endif
>         struct dst_entry *(*route_req)(const struct sock *sk, struct flowi *fl,
> -                                      const struct request_sock *req);
> +                                      struct request_sock *req);
>         u32 (*init_seq)(const struct sk_buff *skb);
>         u32 (*init_ts_off)(const struct net *net, const struct sk_buff *skb);
>         int (*send_synack)(const struct sock *sk, struct dst_entry *dst,
> diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
> index f915abf..ed9ccb7 100644
> --- a/net/ipv4/datagram.c
> +++ b/net/ipv4/datagram.c
> @@ -74,7 +74,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
>         inet->inet_daddr = fl4->daddr;
>         inet->inet_dport = usin->sin_port;
>         sk->sk_state = TCP_ESTABLISHED;
> -       sk_set_txhash(sk);
> +       sk->sk_txhash = get_hash_from_flowi4(fl4);

Maybe keep sk_set_txhash but add an argument that gives the hash.
Hiding behind a function gives us the place to add/change logic in the
future.

>         inet->inet_id = jiffies;
>
>         sk_dst_set(sk, &rt->dst);
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index fda37f2..76f1cf6 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -335,7 +335,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
>         treq->rcv_isn           = ntohl(th->seq) - 1;
>         treq->snt_isn           = cookie;
>         treq->ts_off            = 0;
> -       treq->txhash            = net_tx_rndhash();
>         req->mss                = mss;
>         ireq->ir_num            = ntohs(th->dest);
>         ireq->ir_rmt_port       = th->source;
> @@ -376,6 +375,9 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
>                            opt->srr ? opt->faddr : ireq->ir_rmt_addr,
>                            ireq->ir_loc_addr, th->source, th->dest, sk->sk_uid);
>         security_req_classify_flow(req, flowi4_to_flowi(&fl4));
> +
> +       treq->txhash = get_hash_from_flowi4(&fl4);
> +
>         rt = ip_route_output_key(sock_net(sk), &fl4);
>         if (IS_ERR(rt)) {
>                 reqsk_free(req);
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 734cfc8..e886c28 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6288,7 +6288,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>         }
>
>         tcp_rsk(req)->snt_isn = isn;
> -       tcp_rsk(req)->txhash = net_tx_rndhash();
>         tcp_openreq_init_rwin(req, sk, dst);
>         if (!want_cookie) {
>                 tcp_reqsk_record_syn(sk, req, skb);
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index c6bc0c4..0e71b1b 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -222,7 +222,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>         if (err)
>                 goto failure;
>
> -       sk_set_txhash(sk);
> +       sk->sk_txhash = get_hash_from_flowi4(fl4);
>
>         rt = ip_route_newports(fl4, rt, orig_sport, orig_dport,
>                                inet->inet_sport, inet->inet_dport, sk);
> @@ -871,8 +871,12 @@ static int tcp_v4_send_synack(const struct sock *sk, struct dst_entry *dst,
>         struct sk_buff *skb;
>
>         /* First, grab a route. */
> -       if (!dst && (dst = inet_csk_route_req(sk, &fl4, req)) == NULL)
> -               return -1;
> +       if (!dst) {
> +               if ((dst = inet_csk_route_req(sk, &fl4, req)) == NULL)
> +                       return -1;
> +
> +               tcp_rsk(req)->txhash = get_hash_from_flowi4(&fl4);
> +       }
>
>         skb = tcp_make_synack(sk, dst, req, foc, synack_type);
>
> @@ -1278,9 +1282,12 @@ static void tcp_v4_init_req(struct request_sock *req,
>
>  static struct dst_entry *tcp_v4_route_req(const struct sock *sk,
>                                           struct flowi *fl,
> -                                         const struct request_sock *req)
> +                                         struct request_sock *req)
>  {
> -       return inet_csk_route_req(sk, &fl->u.ip4, req);
> +       struct dst_entry *dst = inet_csk_route_req(sk, &fl->u.ip4, req);
> +       if (dst)
> +               tcp_rsk(req)->txhash = get_hash_from_flowi4(&fl->u.ip4);

This is also apprearing enough to maybe warrant a function to set hash
in a request_sock.

> +       return dst;
>  }
>
>  struct request_sock_ops tcp_request_sock_ops __read_mostly = {
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index a4d214c..94e56cb 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3752,7 +3752,6 @@ int tcp_rtx_synack(const struct sock *sk, struct request_sock *req)
>         struct flowi fl;
>         int res;
>
> -       tcp_rsk(req)->txhash = net_tx_rndhash();
>         res = af_ops->send_synack(sk, NULL, &fl, req, NULL, TCP_SYNACK_NORMAL);
>         if (!res) {
>                 __TCP_INC_STATS(sock_net(sk), TCP_MIB_RETRANSSEGS);
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index a1f9187..c13286c 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -150,6 +150,7 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
>         int                     addr_type;
>         int                     err;
>         __be32                  fl6_flowlabel = 0;
> +       struct flowi6           fl6;
>
>         if (usin->sin6_family == AF_INET) {
>                 if (__ipv6_only_sock(sk))
> @@ -260,7 +261,8 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
>         }
>
>         sk->sk_state = TCP_ESTABLISHED;
> -       sk_set_txhash(sk);
> +       ip6_datagram_flow_key_init(&fl6, sk);
> +       sk->sk_txhash = get_hash_from_flowi6(&fl6);
>  out:
>         return err;
>  }
> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index e7a3a6b..3ba9401 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -216,7 +216,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>         treq->rcv_isn = ntohl(th->seq) - 1;
>         treq->snt_isn = cookie;
>         treq->ts_off = 0;
> -       treq->txhash = net_tx_rndhash();
>
>         /*
>          * We need to lookup the dst_entry to get the correct window size.
> @@ -238,6 +237,8 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>                 fl6.flowi6_uid = sk->sk_uid;
>                 security_req_classify_flow(req, flowi6_to_flowi(&fl6));
>
> +               treq->txhash = get_hash_from_flowi6(&fl6);
> +
>                 dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
>                 if (IS_ERR(dst))
>                         goto out_free;
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6bb98c9..a1a5802 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -286,7 +286,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
>         if (err)
>                 goto late_failure;
>
> -       sk_set_txhash(sk);
> +       sk->sk_txhash = get_hash_from_flowi6(&fl6);
>
>         if (likely(!tp->repair)) {
>                 if (!tp->write_seq)
> @@ -470,9 +470,13 @@ static int tcp_v6_send_synack(const struct sock *sk, struct dst_entry *dst,
>         int err = -ENOMEM;
>
>         /* First, grab a route. */
> -       if (!dst && (dst = inet6_csk_route_req(sk, fl6, req,
> +       if (!dst) {
> +               if ((dst = inet6_csk_route_req(sk, fl6, req,
>                                                IPPROTO_TCP)) == NULL)
> -               goto done;
> +                       goto done;
> +
> +               tcp_rsk(req)->txhash = get_hash_from_flowi6(fl6);
> +       }
>
>         skb = tcp_make_synack(sk, dst, req, foc, synack_type);
>
> @@ -743,9 +747,13 @@ static void tcp_v6_init_req(struct request_sock *req,
>
>  static struct dst_entry *tcp_v6_route_req(const struct sock *sk,
>                                           struct flowi *fl,
> -                                         const struct request_sock *req)
> +                                         struct request_sock *req)
>  {
> -       return inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
> +       struct dst_entry *dst;
> +       dst = inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
> +       if (dst)
> +               tcp_rsk(req)->txhash = get_hash_from_flowi6(&fl->u.ip6);
> +       return dst;
>  }
>
>  struct request_sock_ops tcp6_request_sock_ops __read_mostly = {
> --
> 2.9.5
>

^ permalink raw reply

* [PATCH net-next V2 2/2] net-next: copy user configured flowlabel to reset packet
From: Shaohua Li @ 2017-12-01 21:00 UTC (permalink / raw)
  To: netdev, davem; +Cc: kafai, eric.dumazet, flo, xiyou.wangcong, tom, Shaohua Li
In-Reply-To: <cover.1512161808.git.shli@fb.com>

From: Shaohua Li <shli@fb.com>

Reset packet doesn't use user configured flowlabel, instead, it always
uses 0. This will cause inconsistency for flowlabel. tw sock already
records flowlabel info, so we can directly use it.

Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Florent Fourcot <flo@fourcot.fr>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Tom Herbert <tom@herbertland.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 net/ipv6/tcp_ipv6.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index a1a5802..9b678cd 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -901,6 +901,8 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
 	struct sock *sk1 = NULL;
 #endif
 	int oif = 0;
+	u8 tclass = 0;
+	__be32 flowlabel = 0;
 
 	if (th->rst)
 		return;
@@ -954,7 +956,21 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
 		trace_tcp_send_reset(sk, skb);
 	}
 
-	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1, 0, 0);
+	if (sk) {
+		if (sk_fullsock(sk)) {
+			struct ipv6_pinfo *np = inet6_sk(sk);
+
+			tclass = np->tclass;
+			flowlabel = np->flow_label & IPV6_FLOWLABEL_MASK;
+		} else {
+			struct inet_timewait_sock *tw = inet_twsk(sk);
+
+			tclass = tw->tw_tclass;
+			flowlabel = cpu_to_be32(tw->tw_flowlabel);
+		}
+	}
+	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1,
+		tclass, flowlabel);
 
 #ifdef CONFIG_TCP_MD5SIG
 out:
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next V2 1/2] net-next: use five-tuple hash for sk_txhash
From: Shaohua Li @ 2017-12-01 21:00 UTC (permalink / raw)
  To: netdev, davem; +Cc: kafai, eric.dumazet, flo, xiyou.wangcong, tom, Shaohua Li
In-Reply-To: <cover.1512161808.git.shli@fb.com>

From: Shaohua Li <shli@fb.com>

We are using sk_txhash to calculate flowlabel, but sk_txhash isn't
always available, for example, in inet_timewait_sock. This causes
problem for reset packet, which will have a different flowlabel. This
causes our router doesn't correctly close tcp connection. We are using
flowlabel to do load balance. Routers in the path maintain connection
state. So if flow label changes, the packet is routed through a
different router. In this case, the old router doesn't get the reset
packet to close the tcp connection.

Per Tom's suggestion, we switch back to five-tuple hash, so we can
reconstruct correct flowlabel for reset packet.

At most places, we already have the flowi info, so we directly use it
build sk_txhash. For synack, we do this after route search. At that
time, we have the flowi info ready, so don't need to create the flowi
info again.

I don't change sk_rethink_txhash() though, it still uses random hash,
which is the whole point to select a different path after a negative
routing advise.

Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Florent Fourcot <flo@fourcot.fr>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Tom Herbert <tom@herbertland.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 include/net/sock.h    | 18 ++++--------------
 include/net/tcp.h     |  2 +-
 net/ipv4/datagram.c   |  2 +-
 net/ipv4/syncookies.c |  4 +++-
 net/ipv4/tcp_input.c  |  1 -
 net/ipv4/tcp_ipv4.c   | 17 ++++++++++++-----
 net/ipv4/tcp_output.c |  1 -
 net/ipv6/datagram.c   |  4 +++-
 net/ipv6/syncookies.c |  3 ++-
 net/ipv6/tcp_ipv6.c   | 18 +++++++++++++-----
 10 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 79e1a2c..640db0f 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1729,22 +1729,12 @@ static inline kuid_t sock_net_uid(const struct net *net, const struct sock *sk)
 	return sk ? sk->sk_uid : make_kuid(net->user_ns, 0);
 }
 
-static inline u32 net_tx_rndhash(void)
-{
-	u32 v = prandom_u32();
-
-	return v ?: 1;
-}
-
-static inline void sk_set_txhash(struct sock *sk)
-{
-	sk->sk_txhash = net_tx_rndhash();
-}
-
 static inline void sk_rethink_txhash(struct sock *sk)
 {
-	if (sk->sk_txhash)
-		sk_set_txhash(sk);
+	if (sk->sk_txhash) {
+		u32 v = prandom_u32();
+		sk->sk_txhash = v ?: 1;
+	}
 }
 
 static inline struct dst_entry *
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4e09398..a5c28be 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1840,7 +1840,7 @@ struct tcp_request_sock_ops {
 				 __u16 *mss);
 #endif
 	struct dst_entry *(*route_req)(const struct sock *sk, struct flowi *fl,
-				       const struct request_sock *req);
+				       struct request_sock *req);
 	u32 (*init_seq)(const struct sk_buff *skb);
 	u32 (*init_ts_off)(const struct net *net, const struct sk_buff *skb);
 	int (*send_synack)(const struct sock *sk, struct dst_entry *dst,
diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
index f915abf..ed9ccb7 100644
--- a/net/ipv4/datagram.c
+++ b/net/ipv4/datagram.c
@@ -74,7 +74,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
 	inet->inet_daddr = fl4->daddr;
 	inet->inet_dport = usin->sin_port;
 	sk->sk_state = TCP_ESTABLISHED;
-	sk_set_txhash(sk);
+	sk->sk_txhash = get_hash_from_flowi4(fl4);
 	inet->inet_id = jiffies;
 
 	sk_dst_set(sk, &rt->dst);
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index fda37f2..76f1cf6 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -335,7 +335,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	treq->rcv_isn		= ntohl(th->seq) - 1;
 	treq->snt_isn		= cookie;
 	treq->ts_off		= 0;
-	treq->txhash		= net_tx_rndhash();
 	req->mss		= mss;
 	ireq->ir_num		= ntohs(th->dest);
 	ireq->ir_rmt_port	= th->source;
@@ -376,6 +375,9 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 			   opt->srr ? opt->faddr : ireq->ir_rmt_addr,
 			   ireq->ir_loc_addr, th->source, th->dest, sk->sk_uid);
 	security_req_classify_flow(req, flowi4_to_flowi(&fl4));
+
+	treq->txhash = get_hash_from_flowi4(&fl4);
+
 	rt = ip_route_output_key(sock_net(sk), &fl4);
 	if (IS_ERR(rt)) {
 		reqsk_free(req);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 734cfc8..e886c28 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6288,7 +6288,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	}
 
 	tcp_rsk(req)->snt_isn = isn;
-	tcp_rsk(req)->txhash = net_tx_rndhash();
 	tcp_openreq_init_rwin(req, sk, dst);
 	if (!want_cookie) {
 		tcp_reqsk_record_syn(sk, req, skb);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c6bc0c4..0e71b1b 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -222,7 +222,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	if (err)
 		goto failure;
 
-	sk_set_txhash(sk);
+	sk->sk_txhash = get_hash_from_flowi4(fl4);
 
 	rt = ip_route_newports(fl4, rt, orig_sport, orig_dport,
 			       inet->inet_sport, inet->inet_dport, sk);
@@ -871,8 +871,12 @@ static int tcp_v4_send_synack(const struct sock *sk, struct dst_entry *dst,
 	struct sk_buff *skb;
 
 	/* First, grab a route. */
-	if (!dst && (dst = inet_csk_route_req(sk, &fl4, req)) == NULL)
-		return -1;
+	if (!dst) {
+		if ((dst = inet_csk_route_req(sk, &fl4, req)) == NULL)
+			return -1;
+
+		tcp_rsk(req)->txhash = get_hash_from_flowi4(&fl4);
+	}
 
 	skb = tcp_make_synack(sk, dst, req, foc, synack_type);
 
@@ -1278,9 +1282,12 @@ static void tcp_v4_init_req(struct request_sock *req,
 
 static struct dst_entry *tcp_v4_route_req(const struct sock *sk,
 					  struct flowi *fl,
-					  const struct request_sock *req)
+					  struct request_sock *req)
 {
-	return inet_csk_route_req(sk, &fl->u.ip4, req);
+	struct dst_entry *dst = inet_csk_route_req(sk, &fl->u.ip4, req);
+	if (dst)
+		tcp_rsk(req)->txhash = get_hash_from_flowi4(&fl->u.ip4);
+	return dst;
 }
 
 struct request_sock_ops tcp_request_sock_ops __read_mostly = {
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a4d214c..94e56cb 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3752,7 +3752,6 @@ int tcp_rtx_synack(const struct sock *sk, struct request_sock *req)
 	struct flowi fl;
 	int res;
 
-	tcp_rsk(req)->txhash = net_tx_rndhash();
 	res = af_ops->send_synack(sk, NULL, &fl, req, NULL, TCP_SYNACK_NORMAL);
 	if (!res) {
 		__TCP_INC_STATS(sock_net(sk), TCP_MIB_RETRANSSEGS);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index a1f9187..c13286c 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -150,6 +150,7 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 	int			addr_type;
 	int			err;
 	__be32			fl6_flowlabel = 0;
+	struct flowi6		fl6;
 
 	if (usin->sin6_family == AF_INET) {
 		if (__ipv6_only_sock(sk))
@@ -260,7 +261,8 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 	}
 
 	sk->sk_state = TCP_ESTABLISHED;
-	sk_set_txhash(sk);
+	ip6_datagram_flow_key_init(&fl6, sk);
+	sk->sk_txhash = get_hash_from_flowi6(&fl6);
 out:
 	return err;
 }
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index e7a3a6b..3ba9401 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -216,7 +216,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	treq->rcv_isn = ntohl(th->seq) - 1;
 	treq->snt_isn = cookie;
 	treq->ts_off = 0;
-	treq->txhash = net_tx_rndhash();
 
 	/*
 	 * We need to lookup the dst_entry to get the correct window size.
@@ -238,6 +237,8 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 		fl6.flowi6_uid = sk->sk_uid;
 		security_req_classify_flow(req, flowi6_to_flowi(&fl6));
 
+		treq->txhash = get_hash_from_flowi6(&fl6);
+
 		dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
 		if (IS_ERR(dst))
 			goto out_free;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6bb98c9..a1a5802 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -286,7 +286,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
 	if (err)
 		goto late_failure;
 
-	sk_set_txhash(sk);
+	sk->sk_txhash = get_hash_from_flowi6(&fl6);
 
 	if (likely(!tp->repair)) {
 		if (!tp->write_seq)
@@ -470,9 +470,13 @@ static int tcp_v6_send_synack(const struct sock *sk, struct dst_entry *dst,
 	int err = -ENOMEM;
 
 	/* First, grab a route. */
-	if (!dst && (dst = inet6_csk_route_req(sk, fl6, req,
+	if (!dst) {
+		if ((dst = inet6_csk_route_req(sk, fl6, req,
 					       IPPROTO_TCP)) == NULL)
-		goto done;
+			goto done;
+
+		tcp_rsk(req)->txhash = get_hash_from_flowi6(fl6);
+	}
 
 	skb = tcp_make_synack(sk, dst, req, foc, synack_type);
 
@@ -743,9 +747,13 @@ static void tcp_v6_init_req(struct request_sock *req,
 
 static struct dst_entry *tcp_v6_route_req(const struct sock *sk,
 					  struct flowi *fl,
-					  const struct request_sock *req)
+					  struct request_sock *req)
 {
-	return inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
+	struct dst_entry *dst;
+	dst = inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
+	if (dst)
+		tcp_rsk(req)->txhash = get_hash_from_flowi6(&fl->u.ip6);
+	return dst;
 }
 
 struct request_sock_ops tcp6_request_sock_ops __read_mostly = {
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next V2 0/2] net: fix flowlabel inconsistency in reset packet
From: Shaohua Li @ 2017-12-01 21:00 UTC (permalink / raw)
  To: netdev, davem; +Cc: kafai, eric.dumazet, flo, xiyou.wangcong, tom, Shaohua Li

From: Shaohua Li <shli@fb.com>

Hi,

Please see below tcpdump output:
21:00:48.109122 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [S], cksum 0x0529 (incorrect -> 0xf56c), seq 3282214508, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 0,nop,wscale 7], length 0
21:00:48.109381 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [S.], cksum 0x0529 (incorrect -> 0x49ad), seq 1923801573, ack 3282214509, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 2500903437,nop,wscale 7], length 0
21:00:48.109548 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1bdf), seq 1, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.109823 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 62) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x053f (incorrect -> 0xb8b1), seq 1:31, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 30
21:00:48.109910 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [.], cksum 0x0521 (incorrect -> 0x1bc1), seq 1, ack 31, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.110043 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [P.], cksum 0x0539 (incorrect -> 0xb726), seq 1:25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 24
21:00:48.110173 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba7), seq 31, ack 25, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:48.110211 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [F.], cksum 0x0521 (incorrect -> 0x1ba7), seq 25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 0
21:00:48.151099 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba6), seq 31, ack 26, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:49.110524 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x0539 (incorrect -> 0xb324), seq 31:55, ack 26, win 342, options [nop,nop,TS val 2500904438 ecr 2500903438], length 24
21:00:49.110637 IP6 (flowlabel 0xb34d5, hlim 64, next-header TCP (6) payload length: 20) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [R], cksum 0x0515 (incorrect -> 0x668c), seq 1923801599, win 0, length 0

The tcp reset packet has a different flowlabel, which causes our router
doesn't correctly close tcp connection. We are using flowlabel to do
load balance. Routers in the path maintain connection state. So if flow
label changes, the packet is routed through a different router. In this
case, the old router doesn't get the reset packet to close the tcp
connection.

The reason is the normal packet gets the skb->hash from sk->sk_txhash,
which is generated randomly. ip6_make_flowlabel then uses the hash to
create a flowlabel. The reset packet doesn't get assigned a hash, so the
flowlabel is calculated with flowi6.

The patches fix the issue.

Thanks,
Shaohua


Shaohua Li (2):
  net-next: use five-tuple hash for sk_txhash
  net-next: copy user configured flowlabel to reset packet

 include/net/sock.h    | 18 ++++--------------
 include/net/tcp.h     |  2 +-
 net/ipv4/datagram.c   |  2 +-
 net/ipv4/syncookies.c |  4 +++-
 net/ipv4/tcp_input.c  |  1 -
 net/ipv4/tcp_ipv4.c   | 17 ++++++++++++-----
 net/ipv4/tcp_output.c |  1 -
 net/ipv6/datagram.c   |  4 +++-
 net/ipv6/syncookies.c |  3 ++-
 net/ipv6/tcp_ipv6.c   | 36 ++++++++++++++++++++++++++++++------
 10 files changed, 56 insertions(+), 32 deletions(-)

-- 
2.9.5

^ permalink raw reply

* Re: [PATCH v4 3/8] MIPS: Octeon: Add a global resource manager.
From: David Daney @ 2017-12-01 20:56 UTC (permalink / raw)
  To: Philippe Ombredanne
  Cc: Mark Rutland, linux-mips, devel, David Daney, Greg Kroah-Hartman,
	LKML, ralf, Carlos Munoz,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Andrew Lunn, Steven J. Hill, netdev,
	Florian Fainelli, James Hogan, David S. Miller
In-Reply-To: <CAOFm3uHSp3ziQ-h1-V9uz07+jiixpgoUB9UZV82fOkrMBk8aZQ@mail.gmail.com>

On 12/01/2017 12:41 PM, Philippe Ombredanne wrote:
> David,
> 
> On Fri, Dec 1, 2017 at 9:01 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>> On 12/01/2017 11:49 AM, Philippe Ombredanne wrote:
>>>
>>> David, Greg,
>>>
>>> On Fri, Dec 1, 2017 at 6:42 PM, David Daney <ddaney@caviumnetworks.com>
>>> wrote:
>>>>
>>>> On 11/30/2017 11:53 PM, Philippe Ombredanne wrote:
>>>
>>> [...]
>>>>>>>
>>>>>>> --- /dev/null
>>>>>>> +++ b/arch/mips/cavium-octeon/resource-mgr.c
>>>>>>> @@ -0,0 +1,371 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>> +/*
>>>>>>> + * Resource manager for Octeon.
>>>>>>> + *
>>>>>>> + * This file is subject to the terms and conditions of the GNU
>>>>>>> General
>>>>>>> Public
>>>>>>> + * License.  See the file "COPYING" in the main directory of this
>>>>>>> archive
>>>>>>> + * for more details.
>>>>>>> + *
>>>>>>> + * Copyright (C) 2017 Cavium, Inc.
>>>>>>> + */
>>>>>
>>>>>
>>>>>
>>>>> Since you nicely included an SPDX id, you would not need the
>>>>> boilerplate anymore. e.g. these can go alright?
>>>>
>>>>
>>>>
>>>> They may not be strictly speaking necessary, but I don't think they hurt
>>>> anything.  Unless there is a requirement to strip out the license text,
>>>> we
>>>> would stick with it as is.
>>>
>>>
>>> I think the requirement is there and that would be much better for
>>> everyone: keeping both is redundant and does not bring any value, does
>>> it? Instead it kinda removes the benefits of having the SPDX id in the
>>> first place IMHO.
>>>
>>> Furthermore, as there have been already ~12K+ files cleaned up and
>>> still over 60K files to go, it would really nice if new files could
>>> adopt the new style: this way we will not have to revisit and repatch
>>> them in the future.
>>>
>>
>> I am happy to follow any style Greg would suggest.  There doesn't seem to be
>> much documentation about how this should be done yet.
> 
> Thomas (tglx) has already submitted a first series of doc patches a
> few weeks ago. And AFAIK he might be working on posting the updates
> soon, whenever his real time clock yields a few cycles away from real
> time coding work ;)
> 
> See also these discussions with Linus [1][2][3], Thomas[4] and Greg[5]
> on this and mostly related topics
> 
> [1] https://lkml.org/lkml/2017/11/2/715
> [2] https://lkml.org/lkml/2017/11/25/125
> [3] https://lkml.org/lkml/2017/11/25/133
> [4] https://lkml.org/lkml/2017/11/2/805
> [5] https://lkml.org/lkml/2017/10/19/165
> 

OK, you convinced me.

Thanks,
David

^ permalink raw reply

* [PATCH v2 net-next 2/4] udp: Move udp[46]_portaddr_hash() to net/ip[v6].h
From: Martin KaFai Lau @ 2017-12-01 20:52 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171201205232.3012584-1-kafai@fb.com>

This patch moves the udp[46]_portaddr_hash()
to net/ip[v6].h.  The function name is renamed to
ipv[46]_portaddr_hash().

It will be used by a later patch which adds a second listener
hashtable hashed by the address and port.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip.h   |  9 +++++++++
 include/net/ipv6.h | 17 +++++++++++++++++
 net/ipv4/udp.c     | 22 ++++++++--------------
 net/ipv6/udp.c     | 32 ++++++++------------------------
 4 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 9896f46cbbf1..fc9bf1b1fe2c 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -26,12 +26,14 @@
 #include <linux/ip.h>
 #include <linux/in.h>
 #include <linux/skbuff.h>
+#include <linux/jhash.h>
 
 #include <net/inet_sock.h>
 #include <net/route.h>
 #include <net/snmp.h>
 #include <net/flow.h>
 #include <net/flow_dissector.h>
+#include <net/netns/hash.h>
 
 #define IPV4_MAX_PMTU		65535U		/* RFC 2675, Section 5.1 */
 
@@ -521,6 +523,13 @@ static inline unsigned int ipv4_addr_hash(__be32 ip)
 	return (__force unsigned int) ip;
 }
 
+static inline u32 ipv4_portaddr_hash(const struct net *net,
+				     __be32 saddr,
+				     unsigned int port)
+{
+	return jhash_1word((__force u32)saddr, net_hash_mix(net)) ^ port;
+}
+
 bool ip_call_ra_chain(struct sk_buff *skb);
 
 /*
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index f73797e2fa60..25be4715578c 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -22,6 +22,7 @@
 #include <net/flow.h>
 #include <net/flow_dissector.h>
 #include <net/snmp.h>
+#include <net/netns/hash.h>
 
 #define SIN6_LEN_RFC2133	24
 
@@ -673,6 +674,22 @@ static inline bool ipv6_addr_v4mapped(const struct in6_addr *a)
 					cpu_to_be32(0x0000ffff))) == 0UL;
 }
 
+static inline u32 ipv6_portaddr_hash(const struct net *net,
+				     const struct in6_addr *addr6,
+				     unsigned int port)
+{
+	unsigned int hash, mix = net_hash_mix(net);
+
+	if (ipv6_addr_any(addr6))
+		hash = jhash_1word(0, mix);
+	else if (ipv6_addr_v4mapped(addr6))
+		hash = jhash_1word((__force u32)addr6->s6_addr32[3], mix);
+	else
+		hash = jhash2((__force u32 *)addr6->s6_addr32, 4, mix);
+
+	return hash ^ port;
+}
+
 /*
  * Check for a RFC 4843 ORCHID address
  * (Overlay Routable Cryptographic Hash Identifiers)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 36f857c87fe2..e9c0d1e1772e 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -357,18 +357,12 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
 }
 EXPORT_SYMBOL(udp_lib_get_port);
 
-static u32 udp4_portaddr_hash(const struct net *net, __be32 saddr,
-			      unsigned int port)
-{
-	return jhash_1word((__force u32)saddr, net_hash_mix(net)) ^ port;
-}
-
 int udp_v4_get_port(struct sock *sk, unsigned short snum)
 {
 	unsigned int hash2_nulladdr =
-		udp4_portaddr_hash(sock_net(sk), htonl(INADDR_ANY), snum);
+		ipv4_portaddr_hash(sock_net(sk), htonl(INADDR_ANY), snum);
 	unsigned int hash2_partial =
-		udp4_portaddr_hash(sock_net(sk), inet_sk(sk)->inet_rcv_saddr, 0);
+		ipv4_portaddr_hash(sock_net(sk), inet_sk(sk)->inet_rcv_saddr, 0);
 
 	/* precompute partial secondary hash */
 	udp_sk(sk)->udp_portaddr_hash = hash2_partial;
@@ -485,7 +479,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 	u32 hash = 0;
 
 	if (hslot->count > 10) {
-		hash2 = udp4_portaddr_hash(net, daddr, hnum);
+		hash2 = ipv4_portaddr_hash(net, daddr, hnum);
 		slot2 = hash2 & udptable->mask;
 		hslot2 = &udptable->hash2[slot2];
 		if (hslot->count < hslot2->count)
@@ -496,7 +490,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 					  exact_dif, hslot2, skb);
 		if (!result) {
 			unsigned int old_slot2 = slot2;
-			hash2 = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum);
+			hash2 = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum);
 			slot2 = hash2 & udptable->mask;
 			/* avoid searching the same slot again. */
 			if (unlikely(slot2 == old_slot2))
@@ -1761,7 +1755,7 @@ EXPORT_SYMBOL(udp_lib_rehash);
 
 static void udp_v4_rehash(struct sock *sk)
 {
-	u16 new_hash = udp4_portaddr_hash(sock_net(sk),
+	u16 new_hash = ipv4_portaddr_hash(sock_net(sk),
 					  inet_sk(sk)->inet_rcv_saddr,
 					  inet_sk(sk)->inet_num);
 	udp_lib_rehash(sk, new_hash);
@@ -1952,9 +1946,9 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	struct sk_buff *nskb;
 
 	if (use_hash2) {
-		hash2_any = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
+		hash2_any = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
 			    udptable->mask;
-		hash2 = udp4_portaddr_hash(net, daddr, hnum) & udptable->mask;
+		hash2 = ipv4_portaddr_hash(net, daddr, hnum) & udptable->mask;
 start_lookup:
 		hslot = &udptable->hash2[hash2];
 		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
@@ -2186,7 +2180,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
 					    int dif, int sdif)
 {
 	unsigned short hnum = ntohs(loc_port);
-	unsigned int hash2 = udp4_portaddr_hash(net, loc_addr, hnum);
+	unsigned int hash2 = ipv4_portaddr_hash(net, loc_addr, hnum);
 	unsigned int slot2 = hash2 & udp_table.mask;
 	struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
 	INET_ADDR_COOKIE(acookie, rmt_addr, loc_addr);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index c9f91c28b81d..eecf9f0faf29 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -89,28 +89,12 @@ static u32 udp6_ehashfn(const struct net *net,
 			       udp_ipv6_hash_secret + net_hash_mix(net));
 }
 
-static u32 udp6_portaddr_hash(const struct net *net,
-			      const struct in6_addr *addr6,
-			      unsigned int port)
-{
-	unsigned int hash, mix = net_hash_mix(net);
-
-	if (ipv6_addr_any(addr6))
-		hash = jhash_1word(0, mix);
-	else if (ipv6_addr_v4mapped(addr6))
-		hash = jhash_1word((__force u32)addr6->s6_addr32[3], mix);
-	else
-		hash = jhash2((__force u32 *)addr6->s6_addr32, 4, mix);
-
-	return hash ^ port;
-}
-
 int udp_v6_get_port(struct sock *sk, unsigned short snum)
 {
 	unsigned int hash2_nulladdr =
-		udp6_portaddr_hash(sock_net(sk), &in6addr_any, snum);
+		ipv6_portaddr_hash(sock_net(sk), &in6addr_any, snum);
 	unsigned int hash2_partial =
-		udp6_portaddr_hash(sock_net(sk), &sk->sk_v6_rcv_saddr, 0);
+		ipv6_portaddr_hash(sock_net(sk), &sk->sk_v6_rcv_saddr, 0);
 
 	/* precompute partial secondary hash */
 	udp_sk(sk)->udp_portaddr_hash = hash2_partial;
@@ -119,7 +103,7 @@ int udp_v6_get_port(struct sock *sk, unsigned short snum)
 
 static void udp_v6_rehash(struct sock *sk)
 {
-	u16 new_hash = udp6_portaddr_hash(sock_net(sk),
+	u16 new_hash = ipv6_portaddr_hash(sock_net(sk),
 					  &sk->sk_v6_rcv_saddr,
 					  inet_sk(sk)->inet_num);
 
@@ -225,7 +209,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
 	u32 hash = 0;
 
 	if (hslot->count > 10) {
-		hash2 = udp6_portaddr_hash(net, daddr, hnum);
+		hash2 = ipv6_portaddr_hash(net, daddr, hnum);
 		slot2 = hash2 & udptable->mask;
 		hslot2 = &udptable->hash2[slot2];
 		if (hslot->count < hslot2->count)
@@ -236,7 +220,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
 					  hslot2, skb);
 		if (!result) {
 			unsigned int old_slot2 = slot2;
-			hash2 = udp6_portaddr_hash(net, &in6addr_any, hnum);
+			hash2 = ipv6_portaddr_hash(net, &in6addr_any, hnum);
 			slot2 = hash2 & udptable->mask;
 			/* avoid searching the same slot again. */
 			if (unlikely(slot2 == old_slot2))
@@ -705,9 +689,9 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	struct sk_buff *nskb;
 
 	if (use_hash2) {
-		hash2_any = udp6_portaddr_hash(net, &in6addr_any, hnum) &
+		hash2_any = ipv6_portaddr_hash(net, &in6addr_any, hnum) &
 			    udptable->mask;
-		hash2 = udp6_portaddr_hash(net, daddr, hnum) & udptable->mask;
+		hash2 = ipv6_portaddr_hash(net, daddr, hnum) & udptable->mask;
 start_lookup:
 		hslot = &udptable->hash2[hash2];
 		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
@@ -895,7 +879,7 @@ static struct sock *__udp6_lib_demux_lookup(struct net *net,
 			int dif, int sdif)
 {
 	unsigned short hnum = ntohs(loc_port);
-	unsigned int hash2 = udp6_portaddr_hash(net, loc_addr, hnum);
+	unsigned int hash2 = ipv6_portaddr_hash(net, loc_addr, hnum);
 	unsigned int slot2 = hash2 & udp_table.mask;
 	struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
 	const __portpair ports = INET_COMBINED_PORTS(rmt_port, hnum);
-- 
2.9.5

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox