diff mbox series

[v2] infiniband: i40iw, nes: don't use wall time for TCP sequence numbers

Message ID 20180627132628.915978-1-arnd@arndb.de
State Superseded
Headers show
Series [v2] infiniband: i40iw, nes: don't use wall time for TCP sequence numbers | expand

Commit Message

Arnd Bergmann June 27, 2018, 1:26 p.m. UTC 
The nes infiniband driver uses current_kernel_time() to get a nanosecond
granunarity timestamp to initialize its tcp sequence counters. This is
one of only a few remaining users of that deprecated function, so we
should try to get rid of it.

Aside from using a deprecated API, there are several problems I see here:

- Using a CLOCK_REALTIME based time source makes it predictable in
  case the time base is synchronized.
- Using a coarse timestamp means it only gets updated once per jiffie,
  making it even more predictable in order to avoid having to access
  the hardware clock source
- The upper 2 bits are always zero because the nanoseconds are at most
  999999999.

For the Linux TCP implementation, we use secure_tcp_seq(), which appears
to be appropriate here as well, and solves all the above problems.

i40iw uses a variant of the same code, so I do that same thing there
for ipv4. Unlike nes, i40e also supports ipv6, which needs to call
secure_tcpv6_seq instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
v2: use secure_tcpv6_seq for IPv6 support as suggested by Shiraz Saleem.
---
 drivers/infiniband/hw/i40iw/i40iw_cm.c | 26 +++++++++++++++++++++-----
 drivers/infiniband/hw/nes/nes_cm.c     |  8 +++++---
 net/core/secure_seq.c                  |  1 +
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.9.0

Comments

Saleem, Shiraz June 29, 2018, 10:10 p.m. UTC | #1 
On Wed, Jun 27, 2018 at 07:26:05AM -0600, Arnd Bergmann wrote:
> The nes infiniband driver uses current_kernel_time() to get a nanosecond

> granunarity timestamp to initialize its tcp sequence counters. This is

> one of only a few remaining users of that deprecated function, so we

> should try to get rid of it.

> 

> Aside from using a deprecated API, there are several problems I see here:

> 

> - Using a CLOCK_REALTIME based time source makes it predictable in

>   case the time base is synchronized.

> - Using a coarse timestamp means it only gets updated once per jiffie,

>   making it even more predictable in order to avoid having to access

>   the hardware clock source

> - The upper 2 bits are always zero because the nanoseconds are at most

>   999999999.

> 

> For the Linux TCP implementation, we use secure_tcp_seq(), which appears

> to be appropriate here as well, and solves all the above problems.

> 

> i40iw uses a variant of the same code, so I do that same thing there

> for ipv4. Unlike nes, i40e also supports ipv6, which needs to call

> secure_tcpv6_seq instead.

> 

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> v2: use secure_tcpv6_seq for IPv6 support as suggested by Shiraz Saleem.

> ---


Looks good.

Acked-by: Shiraz Saleem <shiraz.saleem@intel.com>
kernel test robot June 30, 2018, 7 p.m. UTC | #2 
Hi Arnd,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Arnd-Bergmann/infiniband-i40iw-nes-don-t-use-wall-time-for-TCP-sequence-numbers/20180627-221142
config: x86_64-randconfig-s2-06302231 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/infiniband/hw/i40iw/i40iw_cm.o: In function `i40iw_make_cm_node':
>> drivers/infiniband/hw/i40iw/i40iw_cm.c:2232: undefined reference to `secure_tcpv6_seq'


vim +2232 drivers/infiniband/hw/i40iw/i40iw_cm.c

  2153	
  2154	/**
  2155	 * i40iw_make_cm_node - create a new instance of a cm node
  2156	 * @cm_core: cm's core
  2157	 * @iwdev: iwarp device structure
  2158	 * @cm_info: quad info for connection
  2159	 * @listener: passive connection's listener
  2160	 */
  2161	static struct i40iw_cm_node *i40iw_make_cm_node(
  2162					   struct i40iw_cm_core *cm_core,
  2163					   struct i40iw_device *iwdev,
  2164					   struct i40iw_cm_info *cm_info,
  2165					   struct i40iw_cm_listener *listener)
  2166	{
  2167		struct i40iw_cm_node *cm_node;
  2168		int oldarpindex;
  2169		int arpindex;
  2170		struct net_device *netdev = iwdev->netdev;
  2171	
  2172		/* create an hte and cm_node for this instance */
  2173		cm_node = kzalloc(sizeof(*cm_node), GFP_ATOMIC);
  2174		if (!cm_node)
  2175			return NULL;
  2176	
  2177		/* set our node specific transport info */
  2178		cm_node->ipv4 = cm_info->ipv4;
  2179		cm_node->vlan_id = cm_info->vlan_id;
  2180		if ((cm_node->vlan_id == I40IW_NO_VLAN) && iwdev->dcb)
  2181			cm_node->vlan_id = 0;
  2182		cm_node->tos = cm_info->tos;
  2183		cm_node->user_pri = cm_info->user_pri;
  2184		if (listener) {
  2185			if (listener->tos != cm_info->tos)
  2186				i40iw_debug(&iwdev->sc_dev, I40IW_DEBUG_DCB,
  2187					    "application TOS[%d] and remote client TOS[%d] mismatch\n",
  2188					     listener->tos, cm_info->tos);
  2189			cm_node->tos = max(listener->tos, cm_info->tos);
  2190			cm_node->user_pri = rt_tos2priority(cm_node->tos);
  2191			i40iw_debug(&iwdev->sc_dev, I40IW_DEBUG_DCB, "listener: TOS:[%d] UP:[%d]\n",
  2192				    cm_node->tos, cm_node->user_pri);
  2193		}
  2194		memcpy(cm_node->loc_addr, cm_info->loc_addr, sizeof(cm_node->loc_addr));
  2195		memcpy(cm_node->rem_addr, cm_info->rem_addr, sizeof(cm_node->rem_addr));
  2196		cm_node->loc_port = cm_info->loc_port;
  2197		cm_node->rem_port = cm_info->rem_port;
  2198	
  2199		cm_node->mpa_frame_rev = iwdev->mpa_version;
  2200		cm_node->send_rdma0_op = SEND_RDMA_READ_ZERO;
  2201		cm_node->ird_size = I40IW_MAX_IRD_SIZE;
  2202		cm_node->ord_size = I40IW_MAX_ORD_SIZE;
  2203	
  2204		cm_node->listener = listener;
  2205		cm_node->cm_id = cm_info->cm_id;
  2206		ether_addr_copy(cm_node->loc_mac, netdev->dev_addr);
  2207		spin_lock_init(&cm_node->retrans_list_lock);
  2208		cm_node->ack_rcvd = false;
  2209	
  2210		atomic_set(&cm_node->ref_count, 1);
  2211		/* associate our parent CM core */
  2212		cm_node->cm_core = cm_core;
  2213		cm_node->tcp_cntxt.loc_id = I40IW_CM_DEF_LOCAL_ID;
  2214		cm_node->tcp_cntxt.rcv_wscale = I40IW_CM_DEFAULT_RCV_WND_SCALE;
  2215		cm_node->tcp_cntxt.rcv_wnd =
  2216				I40IW_CM_DEFAULT_RCV_WND_SCALED >> I40IW_CM_DEFAULT_RCV_WND_SCALE;
  2217		if (cm_node->ipv4) {
  2218			cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr[0]),
  2219								htonl(cm_node->rem_addr[0]),
  2220								htons(cm_node->loc_port),
  2221								htons(cm_node->rem_port));
  2222			cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV4;
  2223		} else {
  2224			__be32 loc[4] = {
  2225				htonl(cm_node->loc_addr[0]), htonl(cm_node->loc_addr[1]),
  2226				htonl(cm_node->loc_addr[2]), htonl(cm_node->loc_addr[3])
  2227			};
  2228			__be32 rem[4] = {
  2229				htonl(cm_node->rem_addr[0]), htonl(cm_node->rem_addr[1]),
  2230				htonl(cm_node->rem_addr[2]), htonl(cm_node->rem_addr[3])
  2231			};
> 2232			cm_node->tcp_cntxt.loc_seq_num = secure_tcpv6_seq(loc, rem,

  2233								htons(cm_node->loc_port),
  2234								htons(cm_node->rem_port));
  2235			cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV6;
  2236		}
  2237	
  2238		cm_node->iwdev = iwdev;
  2239		cm_node->dev = &iwdev->sc_dev;
  2240	
  2241		if ((cm_node->ipv4 &&
  2242		     i40iw_ipv4_is_loopback(cm_node->loc_addr[0], cm_node->rem_addr[0])) ||
  2243		     (!cm_node->ipv4 && i40iw_ipv6_is_loopback(cm_node->loc_addr,
  2244							       cm_node->rem_addr))) {
  2245			arpindex = i40iw_arp_table(iwdev,
  2246						   cm_node->rem_addr,
  2247						   false,
  2248						   NULL,
  2249						   I40IW_ARP_RESOLVE);
  2250		} else {
  2251			oldarpindex = i40iw_arp_table(iwdev,
  2252						      cm_node->rem_addr,
  2253						      false,
  2254						      NULL,
  2255						      I40IW_ARP_RESOLVE);
  2256			if (cm_node->ipv4)
  2257				arpindex = i40iw_addr_resolve_neigh(iwdev,
  2258								    cm_info->loc_addr[0],
  2259								    cm_info->rem_addr[0],
  2260								    oldarpindex);
  2261			else if (IS_ENABLED(CONFIG_IPV6))
  2262				arpindex = i40iw_addr_resolve_neigh_ipv6(iwdev,
  2263									 cm_info->loc_addr,
  2264									 cm_info->rem_addr,
  2265									 oldarpindex);
  2266			else
  2267				arpindex = -EINVAL;
  2268		}
  2269		if (arpindex < 0) {
  2270			i40iw_pr_err("cm_node arpindex\n");
  2271			kfree(cm_node);
  2272			return NULL;
  2273		}
  2274		ether_addr_copy(cm_node->rem_mac, iwdev->arp_table[arpindex].mac_addr);
  2275		i40iw_add_hte_node(cm_core, cm_node);
  2276		cm_core->stats_nodes_created++;
  2277		return cm_node;
  2278	}
  2279	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Arnd Bergmann July 5, 2018, 1:15 p.m. UTC | #3 
On Sat, Jun 30, 2018 at 9:00 PM, kbuild test robot <lkp@intel.com> wrote:
> Hi Arnd,

>

> I love your patch! Yet something to improve:

>

> [auto build test ERROR on linus/master]

> [also build test ERROR on v4.18-rc2 next-20180629]

> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

>

> url:    https://github.com/0day-ci/linux/commits/Arnd-Bergmann/infiniband-i40iw-nes-don-t-use-wall-time-for-TCP-sequence-numbers/20180627-221142

> config: x86_64-randconfig-s2-06302231 (attached as .config)

> compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026

> reproduce:

>         # save the attached .config to linux build tree

>         make ARCH=x86_64

>

> All errors (new ones prefixed by >>):

>

>    drivers/infiniband/hw/i40iw/i40iw_cm.o: In function `i40iw_make_cm_node':

>>> drivers/infiniband/hw/i40iw/i40iw_cm.c:2232: undefined reference to `secure_tcpv6_seq'

>


Fixed this now by added a

        depends on IPV6 || !IPV6

dependency in Kconfig. This ensures that with IPV6=m, i40iw cannot be built-in.

Will send v3 after a little more build testing.

      Arnd
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c b/drivers/infiniband/hw/i40iw/i40iw_cm.c
index 7b2655128b9f..e2590784b857 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
@@ -57,6 +57,7 @@ 
 #include <net/addrconf.h>
 #include <net/ip6_route.h>
 #include <net/ip_fib.h>
+#include <net/secure_seq.h>
 #include <net/tcp.h>
 #include <asm/checksum.h>
 
@@ -2164,7 +2165,6 @@  static struct i40iw_cm_node *i40iw_make_cm_node(
 				   struct i40iw_cm_listener *listener)
 {
 	struct i40iw_cm_node *cm_node;
-	struct timespec ts;
 	int oldarpindex;
 	int arpindex;
 	struct net_device *netdev = iwdev->netdev;
@@ -2214,10 +2214,26 @@  static struct i40iw_cm_node *i40iw_make_cm_node(
 	cm_node->tcp_cntxt.rcv_wscale = I40IW_CM_DEFAULT_RCV_WND_SCALE;
 	cm_node->tcp_cntxt.rcv_wnd =
 			I40IW_CM_DEFAULT_RCV_WND_SCALED >> I40IW_CM_DEFAULT_RCV_WND_SCALE;
-	ts = current_kernel_time();
-	cm_node->tcp_cntxt.loc_seq_num = ts.tv_nsec;
-	cm_node->tcp_cntxt.mss = (cm_node->ipv4) ? (iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV4) :
-				 (iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV6);
+	if (cm_node->ipv4) {
+		cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr[0]),
+							htonl(cm_node->rem_addr[0]),
+							htons(cm_node->loc_port),
+							htons(cm_node->rem_port));
+		cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV4;
+	} else {
+		__be32 loc[4] = {
+			htonl(cm_node->loc_addr[0]), htonl(cm_node->loc_addr[1]),
+			htonl(cm_node->loc_addr[2]), htonl(cm_node->loc_addr[3])
+		};
+		__be32 rem[4] = {
+			htonl(cm_node->rem_addr[0]), htonl(cm_node->rem_addr[1]),
+			htonl(cm_node->rem_addr[2]), htonl(cm_node->rem_addr[3])
+		};
+		cm_node->tcp_cntxt.loc_seq_num = secure_tcpv6_seq(loc, rem,
+							htons(cm_node->loc_port),
+							htons(cm_node->rem_port));
+		cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV6;
+	}
 
 	cm_node->iwdev = iwdev;
 	cm_node->dev = &iwdev->sc_dev;
diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index 6cdfbf8c5674..2b67ace5b614 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -58,6 +58,7 @@ 
 #include <net/neighbour.h>
 #include <net/route.h>
 #include <net/ip_fib.h>
+#include <net/secure_seq.h>
 #include <net/tcp.h>
 #include <linux/fcntl.h>
 
@@ -1445,7 +1446,6 @@  static struct nes_cm_node *make_cm_node(struct nes_cm_core *cm_core,
 					struct nes_cm_listener *listener)
 {
 	struct nes_cm_node *cm_node;
-	struct timespec ts;
 	int oldarpindex = 0;
 	int arpindex = 0;
 	struct nes_device *nesdev;
@@ -1496,8 +1496,10 @@  static struct nes_cm_node *make_cm_node(struct nes_cm_core *cm_core,
 	cm_node->tcp_cntxt.rcv_wscale = NES_CM_DEFAULT_RCV_WND_SCALE;
 	cm_node->tcp_cntxt.rcv_wnd = NES_CM_DEFAULT_RCV_WND_SCALED >>
 				     NES_CM_DEFAULT_RCV_WND_SCALE;
-	ts = current_kernel_time();
-	cm_node->tcp_cntxt.loc_seq_num = htonl(ts.tv_nsec);
+	cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr),
+							htonl(cm_node->rem_addr),
+							htons(cm_node->loc_port),
+							htons(cm_node->rem_port));
 	cm_node->tcp_cntxt.mss = nesvnic->max_frame_size - sizeof(struct iphdr) -
 				 sizeof(struct tcphdr) - ETH_HLEN - VLAN_HLEN;
 	cm_node->tcp_cntxt.rcv_nxt = 0;
diff --git a/net/core/secure_seq.c b/net/core/secure_seq.c
index 7232274de334..af6ad467ed61 100644
--- a/net/core/secure_seq.c
+++ b/net/core/secure_seq.c
@@ -140,6 +140,7 @@  u32 secure_tcp_seq(__be32 saddr, __be32 daddr,
 			    &net_secret);
 	return seq_scale(hash);
 }
+EXPORT_SYMBOL_GPL(secure_tcp_seq);
 
 u32 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport)
 {