[PATCH net-next 0/4] Updates for ipsec selftests

[PATCH net-next 0/4] Updates for ipsec selftests

[Shannon Nelson]

Fix up the existing ipsec selftest and add tests for
the ipsec offload driver API.

Shannon Nelson (4):
  selftests: rtnetlink: clear the return code at start of ipsec test
  selftests: rtnetlink: use dummydev as a test device
  netdevsim: add ipsec offload testing
  selftests: rtnetlink: add ipsec offload API test

 drivers/net/netdevsim/Makefile           |   4 +
 drivers/net/netdevsim/ipsec.c            | 345 +++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c           |   7 +
 drivers/net/netdevsim/netdevsim.h        |  37 ++++
 tools/testing/selftests/net/rtnetlink.sh | 132 +++++++++++-
 5 files changed, 518 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/netdevsim/ipsec.c

-- 
2.7.4

[PATCH net-next 1/4] selftests: rtnetlink: clear the return code at start of ipsec test

[Shannon Nelson]

Following the custom from the other functions, clear the global
ret code before starting the test so as to not have previously
failed tests cause us to think this test has failed.

Reported-by: Anders Roxell <anders.roxell@linaro.org>
Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 tools/testing/selftests/net/rtnetlink.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index b33a371..261a981 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -522,6 +522,8 @@ kci_test_macsec()
 #-------------------------------------------------------------------
 kci_test_ipsec()
 {
+	ret=0
+
 	# find an ip address on this machine and make up a destination
 	srcip=`ip -o addr | awk '/inet / { print $4; }' | grep -v "^127" | head -1 | cut -f1 -d/`
 	net=`echo $srcip | cut -f1-3 -d.`
-- 
2.7.4

[PATCH net-next 2/4] selftests: rtnetlink: use dummydev as a test device

[Shannon Nelson]

We really shouldn't mess with local system settings, so let's
use the already created dummy device instead for ipsec testing.
Oh, and let's put the temp file into a proper directory.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 tools/testing/selftests/net/rtnetlink.sh | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 261a981..15948cf 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -523,21 +523,19 @@ kci_test_macsec()
 kci_test_ipsec()
 {
 	ret=0
-
-	# find an ip address on this machine and make up a destination
-	srcip=`ip -o addr | awk '/inet / { print $4; }' | grep -v "^127" | head -1 | cut -f1 -d/`
-	net=`echo $srcip | cut -f1-3 -d.`
-	base=`echo $srcip | cut -f4 -d.`
-	dstip="$net."`expr $base + 1`
-
 	algo="aead rfc4106(gcm(aes)) 0x3132333435363738393031323334353664636261 128"
+	srcip=192.168.123.1
+	dstip=192.168.123.2
+	spi=7
+
+	ip addr add $srcip dev $devdummy
 
 	# flush to be sure there's nothing configured
 	ip x s flush ; ip x p flush
 	check_err $?
 
 	# start the monitor in the background
-	tmpfile=`mktemp ipsectestXXX`
+	tmpfile=`mktemp /var/run/ipsectestXXX`
 	mpid=`(ip x m > $tmpfile & echo $!) 2>/dev/null`
 	sleep 0.2
 
@@ -601,6 +599,7 @@ kci_test_ipsec()
 	check_err $?
 	ip x p flush
 	check_err $?
+	ip addr del $srcip/32 dev $devdummy
 
 	if [ $ret -ne 0 ]; then
 		echo "FAIL: ipsec"
-- 
2.7.4

[PATCH net-next 3/4] netdevsim: add ipsec offload testing

[Shannon Nelson]

Implement the IPsec/XFRM offload API for testing.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 drivers/net/netdevsim/Makefile    |   4 +
 drivers/net/netdevsim/ipsec.c     | 345 ++++++++++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c    |   7 +
 drivers/net/netdevsim/netdevsim.h |  37 ++++
 4 files changed, 393 insertions(+)
 create mode 100644 drivers/net/netdevsim/ipsec.c

diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
index 449b2a1..0fee1d0 100644
--- a/drivers/net/netdevsim/Makefile
+++ b/drivers/net/netdevsim/Makefile
@@ -13,3 +13,7 @@ endif
 ifneq ($(CONFIG_NET_DEVLINK),)
 netdevsim-objs += devlink.o fib.o
 endif
+
+ifneq ($(CONFIG_XFRM_OFFLOAD),)
+netdevsim-objs += ipsec.o
+endif
diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
new file mode 100644
index 0000000..ad64266
--- /dev/null
+++ b/drivers/net/netdevsim/ipsec.c
@@ -0,0 +1,345 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2018 Oracle and/or its affiliates. All rights reserved. */
+
+#include <net/xfrm.h>
+#include <crypto/aead.h>
+#include <linux/debugfs.h>
+#include "netdevsim.h"
+
+#define NSIM_IPSEC_AUTH_BITS	128
+
+/**
+ * nsim_ipsec_dbg_read - read for ipsec data
+ * @filp: the opened file
+ * @buffer: where to write the data for the user to read
+ * @count: the size of the user's buffer
+ * @ppos: file position offset
+ **/
+static ssize_t nsim_dbg_netdev_ops_read(struct file *filp,
+					char __user *buffer,
+					size_t count, loff_t *ppos)
+{
+	struct netdevsim *ns = filp->private_data;
+	struct nsim_ipsec *ipsec = &ns->ipsec;
+	size_t bufsize;
+	char *buf, *p;
+	int len;
+	int i;
+
+	/* don't allow partial reads */
+	if (*ppos != 0)
+		return 0;
+
+	/* the buffer needed is
+	 * (num SAs * 3 lines each * ~60 bytes per line) + one more line
+	 */
+	bufsize = (ipsec->count * 4 * 60) + 60;
+	buf = kzalloc(bufsize, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	p = buf;
+	p += snprintf(p, bufsize - (p - buf),
+		      "SA count=%u tx=%u\n",
+		      ipsec->count, ipsec->tx);
+
+	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
+		struct nsim_sa *sap = &ipsec->sa[i];
+
+		if (!sap->used)
+			continue;
+
+		p += snprintf(p, bufsize - (p - buf),
+			      "sa[%i] %cx ipaddr=0x%08x %08x %08x %08x\n",
+			      i, (sap->rx ? 'r' : 't'), sap->ipaddr[0],
+			      sap->ipaddr[1], sap->ipaddr[2], sap->ipaddr[3]);
+		p += snprintf(p, bufsize - (p - buf),
+			      "sa[%i]    spi=0x%08x proto=0x%x salt=0x%08x crypt=%d\n",
+			      i, be32_to_cpu(sap->xs->id.spi),
+			      sap->xs->id.proto, sap->salt, sap->crypt);
+		p += snprintf(p, bufsize - (p - buf),
+			      "sa[%i]    key=0x%08x %08x %08x %08x\n",
+			      i, sap->key[0], sap->key[1],
+			      sap->key[2], sap->key[3]);
+	}
+
+	len = simple_read_from_buffer(buffer, count, ppos, buf, p - buf);
+
+	kfree(buf);
+	return len;
+}
+
+static const struct file_operations ipsec_dbg_fops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.read = nsim_dbg_netdev_ops_read,
+};
+
+/**
+ * nsim_ipsec_find_empty_idx - find the first unused security parameter index
+ * @ipsec: pointer to ipsec struct
+ **/
+static int nsim_ipsec_find_empty_idx(struct nsim_ipsec *ipsec)
+{
+	u32 i;
+
+	if (ipsec->count == NSIM_IPSEC_MAX_SA_COUNT)
+		return -ENOSPC;
+
+	/* search sa table */
+	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
+		if (!ipsec->sa[i].used)
+			return i;
+	}
+
+	return -ENOSPC;
+}
+
+/**
+ * nsim_ipsec_parse_proto_keys - find the key and salt based on the protocol
+ * @xs: pointer to xfrm_state struct
+ * @mykey: pointer to key array to populate
+ * @mysalt: pointer to salt value to populate
+ *
+ * This copies the protocol keys and salt to our own data tables.  The
+ * 82599 family only supports the one algorithm.
+ **/
+static int nsim_ipsec_parse_proto_keys(struct xfrm_state *xs,
+				       u32 *mykey, u32 *mysalt)
+{
+	struct net_device *dev = xs->xso.dev;
+	unsigned char *key_data;
+	char *alg_name = NULL;
+	const char aes_gcm_name[] = "rfc4106(gcm(aes))";
+	int key_len;
+
+	if (!xs->aead) {
+		netdev_err(dev, "Unsupported IPsec algorithm\n");
+		return -EINVAL;
+	}
+
+	if (xs->aead->alg_icv_len != NSIM_IPSEC_AUTH_BITS) {
+		netdev_err(dev, "IPsec offload requires %d bit authentication\n",
+			   NSIM_IPSEC_AUTH_BITS);
+		return -EINVAL;
+	}
+
+	key_data = &xs->aead->alg_key[0];
+	key_len = xs->aead->alg_key_len;
+	alg_name = xs->aead->alg_name;
+
+	if (strcmp(alg_name, aes_gcm_name)) {
+		netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
+			   aes_gcm_name);
+		return -EINVAL;
+	}
+
+	/* The key bytes come down in a bigendian array of bytes, so
+	 * we don't need to do any byteswapping.
+	 * 160 accounts for 16 byte key and 4 byte salt
+	 */
+	if (key_len > 128) {
+		*mysalt = ((u32 *)key_data)[4];
+	} else if (key_len == 128) {
+		*mysalt = 0;
+	} else {
+		netdev_err(dev, "IPsec hw offload only supports 128 bit keys with optional 32 bit salt\n");
+		return -EINVAL;
+	}
+	memcpy(mykey, key_data, 16);
+
+	return 0;
+}
+
+/**
+ * nsim_ipsec_add_sa - program device with a security association
+ * @xs: pointer to transformer state struct
+ **/
+static int nsim_ipsec_add_sa(struct xfrm_state *xs)
+{
+	struct net_device *dev = xs->xso.dev;
+	struct netdevsim *ns = netdev_priv(dev);
+	struct nsim_ipsec *ipsec = &ns->ipsec;
+	struct nsim_sa sa;
+	u16 sa_idx;
+	int ret;
+
+	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
+		netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
+			   xs->id.proto);
+		return -EINVAL;
+	}
+
+	if (xs->calg) {
+		netdev_err(dev, "Compression offload not supported\n");
+		return -EINVAL;
+	}
+
+	/* find the first unused index */
+	ret = nsim_ipsec_find_empty_idx(ipsec);
+	if (ret < 0) {
+		netdev_err(dev, "No space for SA in Rx table!\n");
+		return ret;
+	}
+	sa_idx = (u16)ret;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.used = true;
+	sa.xs = xs;
+
+	if (sa.xs->id.proto & IPPROTO_ESP)
+		sa.crypt = xs->ealg || xs->aead;
+
+	/* get the key and salt */
+	ret = nsim_ipsec_parse_proto_keys(xs, sa.key, &sa.salt);
+	if (ret) {
+		netdev_err(dev, "Failed to get key data for SA table\n");
+		return ret;
+	}
+
+	if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
+		sa.rx = true;
+
+		if (xs->props.family == AF_INET6)
+			memcpy(sa.ipaddr, &xs->id.daddr.a6, 16);
+		else
+			memcpy(&sa.ipaddr[3], &xs->id.daddr.a4, 4);
+	}
+
+	/* the preparations worked, so save the info */
+	memcpy(&ipsec->sa[sa_idx], &sa, sizeof(sa));
+
+	/* the XFRM stack doesn't like offload_handle == 0,
+	 * so add a bitflag in case our array index is 0
+	 */
+	xs->xso.offload_handle = sa_idx | NSIM_IPSEC_VALID;
+	ipsec->count++;
+
+	return 0;
+}
+
+/**
+ * nsim_ipsec_del_sa - clear out this specific SA
+ * @xs: pointer to transformer state struct
+ **/
+static void nsim_ipsec_del_sa(struct xfrm_state *xs)
+{
+	struct net_device *dev = xs->xso.dev;
+	struct netdevsim *ns = netdev_priv(dev);
+	struct nsim_ipsec *ipsec = &ns->ipsec;
+	u16 sa_idx;
+
+	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
+	if (!ipsec->sa[sa_idx].used) {
+		netdev_err(dev, "Invalid SA for delete sa_idx=%d\n", sa_idx);
+		return;
+	}
+
+	memset(&ipsec->sa[sa_idx], 0, sizeof(struct nsim_sa));
+	ipsec->count--;
+}
+
+/**
+ * nsim_ipsec_offload_ok - can this packet use the xfrm hw offload
+ * @skb: current data packet
+ * @xs: pointer to transformer state struct
+ **/
+static bool nsim_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
+{
+	struct net_device *dev = xs->xso.dev;
+	struct netdevsim *ns = netdev_priv(dev);
+	struct nsim_ipsec *ipsec = &ns->ipsec;
+
+	ipsec->ok++;
+
+	return true;
+}
+
+static const struct xfrmdev_ops nsim_xfrmdev_ops = {
+	.xdo_dev_state_add = nsim_ipsec_add_sa,
+	.xdo_dev_state_delete = nsim_ipsec_del_sa,
+	.xdo_dev_offload_ok = nsim_ipsec_offload_ok,
+};
+
+/**
+ * nsim_ipsec_tx - check Tx packet for ipsec offload
+ * @ns: pointer to ns structure
+ * @skb: current data packet
+ **/
+int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
+{
+	struct nsim_ipsec *ipsec = &ns->ipsec;
+	struct xfrm_state *xs;
+	struct nsim_sa *tsa;
+	u32 sa_idx;
+
+	/* do we even need to check this packet? */
+	if (!skb->sp)
+		return 1;
+
+	if (unlikely(!skb->sp->len)) {
+		netdev_err(ns->netdev, "%s: no xfrm state len = %d\n",
+			   __func__, skb->sp->len);
+		return 0;
+	}
+
+	xs = xfrm_input_state(skb);
+	if (unlikely(!xs)) {
+		netdev_err(ns->netdev, "%s: no xfrm_input_state() xs = %p\n",
+			   __func__, xs);
+		return 0;
+	}
+
+	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
+	if (unlikely(sa_idx > NSIM_IPSEC_MAX_SA_COUNT)) {
+		netdev_err(ns->netdev, "%s: bad sa_idx=%d max=%d\n",
+			   __func__, sa_idx, NSIM_IPSEC_MAX_SA_COUNT);
+		return 0;
+	}
+
+	tsa = &ipsec->sa[sa_idx];
+	if (unlikely(!tsa->used)) {
+		netdev_err(ns->netdev, "%s: unused sa_idx=%d\n",
+			   __func__, sa_idx);
+		return 0;
+	}
+
+	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
+		netdev_err(ns->netdev, "%s: unexpected proto=%d\n",
+			   __func__, xs->id.proto);
+		return 0;
+	}
+
+	ipsec->tx++;
+
+	return 1;
+}
+
+/**
+ * nsim_ipsec_init - initialize security registers for IPSec operation
+ * @ns: board private structure
+ **/
+void nsim_ipsec_init(struct netdevsim *ns)
+{
+	ns->netdev->xfrmdev_ops = &nsim_xfrmdev_ops;
+
+#define NSIM_ESP_FEATURES	(NETIF_F_HW_ESP | \
+				 NETIF_F_HW_ESP_TX_CSUM | \
+				 NETIF_F_GSO_ESP)
+
+	ns->netdev->features |= NSIM_ESP_FEATURES;
+	ns->netdev->hw_enc_features |= NSIM_ESP_FEATURES;
+
+	ns->ipsec.pfile = debugfs_create_file("ipsec", 0400, ns->ddir, ns,
+					      &ipsec_dbg_fops);
+}
+
+void nsim_ipsec_teardown(struct netdevsim *ns)
+{
+	struct nsim_ipsec *ipsec = &ns->ipsec;
+
+	if (ipsec->count)
+		netdev_err(ns->netdev, "%s: tearing down IPsec offload with %d SAs left\n",
+			   __func__, ipsec->count);
+	debugfs_remove_recursive(ipsec->pfile);
+}
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index ec68f38..6ce8604d 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -171,6 +171,8 @@ static int nsim_init(struct net_device *dev)
 	if (err)
 		goto err_unreg_dev;
 
+	nsim_ipsec_init(ns);
+
 	return 0;
 
 err_unreg_dev:
@@ -186,6 +188,7 @@ static void nsim_uninit(struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
 
+	nsim_ipsec_teardown(ns);
 	nsim_devlink_teardown(ns);
 	debugfs_remove_recursive(ns->ddir);
 	nsim_bpf_uninit(ns);
@@ -203,11 +206,15 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
 
+	if (!nsim_ipsec_tx(ns, skb))
+		goto out;
+
 	u64_stats_update_begin(&ns->syncp);
 	ns->tx_packets++;
 	ns->tx_bytes += skb->len;
 	u64_stats_update_end(&ns->syncp);
 
+out:
 	dev_kfree_skb(skb);
 
 	return NETDEV_TX_OK;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 3a8581a..1708dee 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -29,6 +29,29 @@ struct bpf_prog;
 struct dentry;
 struct nsim_vf_config;
 
+#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
+#define NSIM_IPSEC_MAX_SA_COUNT		33
+#define NSIM_IPSEC_VALID		BIT(31)
+
+struct nsim_sa {
+	struct xfrm_state *xs;
+	__be32 ipaddr[4];
+	u32 key[4];
+	u32 salt;
+	bool used;
+	bool crypt;
+	bool rx;
+};
+
+struct nsim_ipsec {
+	struct nsim_sa sa[NSIM_IPSEC_MAX_SA_COUNT];
+	struct dentry *pfile;
+	u32 count;
+	u32 tx;
+	u32 ok;
+};
+#endif
+
 struct netdevsim {
 	struct net_device *netdev;
 
@@ -67,6 +90,9 @@ struct netdevsim {
 #if IS_ENABLED(CONFIG_NET_DEVLINK)
 	struct devlink *devlink;
 #endif
+#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
+	struct nsim_ipsec ipsec;
+#endif
 };
 
 extern struct dentry *nsim_ddir;
@@ -147,6 +173,17 @@ static inline void nsim_devlink_exit(void)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
+void nsim_ipsec_init(struct netdevsim *ns);
+void nsim_ipsec_teardown(struct netdevsim *ns);
+int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb);
+#else
+static inline void nsim_ipsec_init(struct netdevsim *ns) {};
+static inline void nsim_ipsec_teardown(struct netdevsim *ns) {};
+static inline int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
+								{ return 1; };
+#endif
+
 static inline struct netdevsim *to_nsim(struct device *ptr)
 {
 	return container_of(ptr, struct netdevsim, dev);
-- 
2.7.4

Re: [PATCH net-next 3/4] netdevsim: add ipsec offload testing

[Jakub Kicinski]

On Fri, 22 Jun 2018 17:31:37 -0700, Shannon Nelson wrote:
> Implement the IPsec/XFRM offload API for testing.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>

Thanks for the patch!  Just a number of stylistic nit picks.

> diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> new file mode 100644
> index 0000000..ad64266
> --- /dev/null
> +++ b/drivers/net/netdevsim/ipsec.c
> @@ -0,0 +1,345 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2018 Oracle and/or its affiliates. All rights reserved. */
> +
> +#include <net/xfrm.h>
> +#include <crypto/aead.h>
> +#include <linux/debugfs.h>
> +#include "netdevsim.h"

Other files in the driver sort headers alphabetically and put an empty
line between global and local headers.

> +#define NSIM_IPSEC_AUTH_BITS	128
> +
> +/**
> + * nsim_ipsec_dbg_read - read for ipsec data
> + * @filp: the opened file
> + * @buffer: where to write the data for the user to read
> + * @count: the size of the user's buffer
> + * @ppos: file position offset
> + **/
> +static ssize_t nsim_dbg_netdev_ops_read(struct file *filp,

Doesn't match the kdoc.  Please run 

./scripts/kernel-doc -none $file

if you want kdoc.  Although IMHO you may as well drop the kdoc, your
code is quite self explanatory and local.

> +					char __user *buffer,
> +					size_t count, loff_t *ppos)
> +{
> +	struct netdevsim *ns = filp->private_data;
> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> +	size_t bufsize;
> +	char *buf, *p;
> +	int len;
> +	int i;
> +
> +	/* don't allow partial reads */
> +	if (*ppos != 0)
> +		return 0;
> +
> +	/* the buffer needed is
> +	 * (num SAs * 3 lines each * ~60 bytes per line) + one more line
> +	 */
> +	bufsize = (ipsec->count * 4 * 60) + 60;
> +	buf = kzalloc(bufsize, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	p = buf;
> +	p += snprintf(p, bufsize - (p - buf),
> +		      "SA count=%u tx=%u\n",
> +		      ipsec->count, ipsec->tx);
> +
> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
> +		struct nsim_sa *sap = &ipsec->sa[i];
> +
> +		if (!sap->used)
> +			continue;
> +
> +		p += snprintf(p, bufsize - (p - buf),
> +			      "sa[%i] %cx ipaddr=0x%08x %08x %08x %08x\n",
> +			      i, (sap->rx ? 'r' : 't'), sap->ipaddr[0],
> +			      sap->ipaddr[1], sap->ipaddr[2], sap->ipaddr[3]);
> +		p += snprintf(p, bufsize - (p - buf),
> +			      "sa[%i]    spi=0x%08x proto=0x%x salt=0x%08x crypt=%d\n",
> +			      i, be32_to_cpu(sap->xs->id.spi),
> +			      sap->xs->id.proto, sap->salt, sap->crypt);
> +		p += snprintf(p, bufsize - (p - buf),
> +			      "sa[%i]    key=0x%08x %08x %08x %08x\n",
> +			      i, sap->key[0], sap->key[1],
> +			      sap->key[2], sap->key[3]);
> +	}
> +
> +	len = simple_read_from_buffer(buffer, count, ppos, buf, p - buf);

Why not seq_file for this?

> +	kfree(buf);
> +	return len;
> +}
> +
> +static const struct file_operations ipsec_dbg_fops = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.read = nsim_dbg_netdev_ops_read,
> +};
> +
> +/**
> + * nsim_ipsec_find_empty_idx - find the first unused security parameter index
> + * @ipsec: pointer to ipsec struct
> + **/
> +static int nsim_ipsec_find_empty_idx(struct nsim_ipsec *ipsec)
> +{
> +	u32 i;
> +
> +	if (ipsec->count == NSIM_IPSEC_MAX_SA_COUNT)
> +		return -ENOSPC;
> +
> +	/* search sa table */
> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
> +		if (!ipsec->sa[i].used)
> +			return i;
> +	}
> +
> +	return -ENOSPC;

FWIW I personally find bitmaps and find_first_zero_bit() etc. nice and
concise for a small ID allocator, but no objection to open coding.

> +}
> +
> +/**
> + * nsim_ipsec_parse_proto_keys - find the key and salt based on the protocol
> + * @xs: pointer to xfrm_state struct
> + * @mykey: pointer to key array to populate
> + * @mysalt: pointer to salt value to populate
> + *
> + * This copies the protocol keys and salt to our own data tables.  The
> + * 82599 family only supports the one algorithm.

82599 is a fine chip, it's not netdevsim tho? ;)

> + **/
> +static int nsim_ipsec_parse_proto_keys(struct xfrm_state *xs,
> +				       u32 *mykey, u32 *mysalt)
> +{
> +	struct net_device *dev = xs->xso.dev;
> +	unsigned char *key_data;
> +	char *alg_name = NULL;
> +	const char aes_gcm_name[] = "rfc4106(gcm(aes))";
> +	int key_len;

reverse xmas tree please

> +
> +	if (!xs->aead) {
> +		netdev_err(dev, "Unsupported IPsec algorithm\n");
> +		return -EINVAL;
> +	}
> +
> +	if (xs->aead->alg_icv_len != NSIM_IPSEC_AUTH_BITS) {
> +		netdev_err(dev, "IPsec offload requires %d bit authentication\n",
> +			   NSIM_IPSEC_AUTH_BITS);
> +		return -EINVAL;
> +	}
> +
> +	key_data = &xs->aead->alg_key[0];
> +	key_len = xs->aead->alg_key_len;
> +	alg_name = xs->aead->alg_name;
> +
> +	if (strcmp(alg_name, aes_gcm_name)) {
> +		netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
> +			   aes_gcm_name);
> +		return -EINVAL;
> +	}
> +
> +	/* The key bytes come down in a bigendian array of bytes, so
> +	 * we don't need to do any byteswapping.

Why the mention of bigendian?  82599 needs big endian? -.^

> +	 * 160 accounts for 16 byte key and 4 byte salt
> +	 */
> +	if (key_len > 128) {

s/128/NSIM_IPSEC_AUTH_BITS/ ?

> +		*mysalt = ((u32 *)key_data)[4];

Is alignment guaranteed?  There are the unaligned helpers if you need
them..

> +	} else if (key_len == 128) {
> +		*mysalt = 0;
> +	} else {
> +		netdev_err(dev, "IPsec hw offload only supports 128 bit keys with optional 32 bit salt\n");
> +		return -EINVAL;
> +	}
> +	memcpy(mykey, key_data, 16);
> +
> +	return 0;
> +}
> +
> +/**
> + * nsim_ipsec_add_sa - program device with a security association
> + * @xs: pointer to transformer state struct
> + **/
> +static int nsim_ipsec_add_sa(struct xfrm_state *xs)
> +{
> +	struct net_device *dev = xs->xso.dev;
> +	struct netdevsim *ns = netdev_priv(dev);
> +	struct nsim_ipsec *ipsec = &ns->ipsec;

xmas tree again (initialize out of line if you have to)

> +	struct nsim_sa sa;
> +	u16 sa_idx;
> +	int ret;
> +
> +	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
> +		netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
> +			   xs->id.proto);
> +		return -EINVAL;
> +	}
> +
> +	if (xs->calg) {
> +		netdev_err(dev, "Compression offload not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	/* find the first unused index */
> +	ret = nsim_ipsec_find_empty_idx(ipsec);
> +	if (ret < 0) {
> +		netdev_err(dev, "No space for SA in Rx table!\n");
> +		return ret;
> +	}
> +	sa_idx = (u16)ret;
> +
> +	memset(&sa, 0, sizeof(sa));
> +	sa.used = true;
> +	sa.xs = xs;
> +
> +	if (sa.xs->id.proto & IPPROTO_ESP)
> +		sa.crypt = xs->ealg || xs->aead;
> +
> +	/* get the key and salt */
> +	ret = nsim_ipsec_parse_proto_keys(xs, sa.key, &sa.salt);
> +	if (ret) {
> +		netdev_err(dev, "Failed to get key data for SA table\n");
> +		return ret;
> +	}
> +
> +	if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
> +		sa.rx = true;
> +
> +		if (xs->props.family == AF_INET6)
> +			memcpy(sa.ipaddr, &xs->id.daddr.a6, 16);
> +		else
> +			memcpy(&sa.ipaddr[3], &xs->id.daddr.a4, 4);
> +	}
> +
> +	/* the preparations worked, so save the info */
> +	memcpy(&ipsec->sa[sa_idx], &sa, sizeof(sa));
> +
> +	/* the XFRM stack doesn't like offload_handle == 0,
> +	 * so add a bitflag in case our array index is 0
> +	 */
> +	xs->xso.offload_handle = sa_idx | NSIM_IPSEC_VALID;
> +	ipsec->count++;
> +
> +	return 0;
> +}
> +
> +/**
> + * nsim_ipsec_del_sa - clear out this specific SA
> + * @xs: pointer to transformer state struct
> + **/
> +static void nsim_ipsec_del_sa(struct xfrm_state *xs)
> +{
> +	struct net_device *dev = xs->xso.dev;
> +	struct netdevsim *ns = netdev_priv(dev);
> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> +	u16 sa_idx;
> +
> +	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
> +	if (!ipsec->sa[sa_idx].used) {
> +		netdev_err(dev, "Invalid SA for delete sa_idx=%d\n", sa_idx);
> +		return;
> +	}
> +
> +	memset(&ipsec->sa[sa_idx], 0, sizeof(struct nsim_sa));
> +	ipsec->count--;
> +}
> +
> +/**
> + * nsim_ipsec_offload_ok - can this packet use the xfrm hw offload
> + * @skb: current data packet
> + * @xs: pointer to transformer state struct
> + **/
> +static bool nsim_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
> +{
> +	struct net_device *dev = xs->xso.dev;
> +	struct netdevsim *ns = netdev_priv(dev);
> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> +
> +	ipsec->ok++;
> +
> +	return true;
> +}
> +
> +static const struct xfrmdev_ops nsim_xfrmdev_ops = {
> +	.xdo_dev_state_add = nsim_ipsec_add_sa,
> +	.xdo_dev_state_delete = nsim_ipsec_del_sa,
> +	.xdo_dev_offload_ok = nsim_ipsec_offload_ok,

Please align the initializers by adding tabs before '='.

> +};
> +
> +/**
> + * nsim_ipsec_tx - check Tx packet for ipsec offload
> + * @ns: pointer to ns structure
> + * @skb: current data packet
> + **/
> +int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> +{
> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> +	struct xfrm_state *xs;
> +	struct nsim_sa *tsa;
> +	u32 sa_idx;
> +
> +	/* do we even need to check this packet? */
> +	if (!skb->sp)
> +		return 1;
> +
> +	if (unlikely(!skb->sp->len)) {
> +		netdev_err(ns->netdev, "%s: no xfrm state len = %d\n",
> +			   __func__, skb->sp->len);

Hmm..  __func__ started appearing in errors?  Perhaps either always or
never add it?

Also, I know this is not a real device, but please always use rate
limited print functions on the data path.

> +		return 0;
> +	}
> +
> +	xs = xfrm_input_state(skb);
> +	if (unlikely(!xs)) {
> +		netdev_err(ns->netdev, "%s: no xfrm_input_state() xs = %p\n",
> +			   __func__, xs);
> +		return 0;
> +	}
> +
> +	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
> +	if (unlikely(sa_idx > NSIM_IPSEC_MAX_SA_COUNT)) {
> +		netdev_err(ns->netdev, "%s: bad sa_idx=%d max=%d\n",
> +			   __func__, sa_idx, NSIM_IPSEC_MAX_SA_COUNT);
> +		return 0;
> +	}
> +
> +	tsa = &ipsec->sa[sa_idx];
> +	if (unlikely(!tsa->used)) {
> +		netdev_err(ns->netdev, "%s: unused sa_idx=%d\n",
> +			   __func__, sa_idx);
> +		return 0;
> +	}
> +
> +	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
> +		netdev_err(ns->netdev, "%s: unexpected proto=%d\n",
> +			   __func__, xs->id.proto);
> +		return 0;
> +	}
> +
> +	ipsec->tx++;
> +
> +	return 1;
> +}

Looks like the function should return bool?

> +
> +/**
> + * nsim_ipsec_init - initialize security registers for IPSec operation
> + * @ns: board private structure

"board"?  Yes, the kdoc may be best removed ;)

> + **/
> +void nsim_ipsec_init(struct netdevsim *ns)
> +{
> +	ns->netdev->xfrmdev_ops = &nsim_xfrmdev_ops;
> +
> +#define NSIM_ESP_FEATURES	(NETIF_F_HW_ESP | \
> +				 NETIF_F_HW_ESP_TX_CSUM | \
> +				 NETIF_F_GSO_ESP)
> +
> +	ns->netdev->features |= NSIM_ESP_FEATURES;
> +	ns->netdev->hw_enc_features |= NSIM_ESP_FEATURES;
> +
> +	ns->ipsec.pfile = debugfs_create_file("ipsec", 0400, ns->ddir, ns,
> +					      &ipsec_dbg_fops);
> +}
> +
> +void nsim_ipsec_teardown(struct netdevsim *ns)
> +{
> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> +
> +	if (ipsec->count)
> +		netdev_err(ns->netdev, "%s: tearing down IPsec offload with %d SAs left\n",
> +			   __func__, ipsec->count);
> +	debugfs_remove_recursive(ipsec->pfile);
> +}
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index ec68f38..6ce8604d 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -171,6 +171,8 @@ static int nsim_init(struct net_device *dev)
>  	if (err)
>  		goto err_unreg_dev;
>  
> +	nsim_ipsec_init(ns);
> +
>  	return 0;
>  
>  err_unreg_dev:
> @@ -186,6 +188,7 @@ static void nsim_uninit(struct net_device *dev)
>  {
>  	struct netdevsim *ns = netdev_priv(dev);
>  
> +	nsim_ipsec_teardown(ns);
>  	nsim_devlink_teardown(ns);
>  	debugfs_remove_recursive(ns->ddir);
>  	nsim_bpf_uninit(ns);
> @@ -203,11 +206,15 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct netdevsim *ns = netdev_priv(dev);
>  
> +	if (!nsim_ipsec_tx(ns, skb))
> +		goto out;
> +
>  	u64_stats_update_begin(&ns->syncp);
>  	ns->tx_packets++;
>  	ns->tx_bytes += skb->len;
>  	u64_stats_update_end(&ns->syncp);
>  
> +out:
>  	dev_kfree_skb(skb);
>  
>  	return NETDEV_TX_OK;
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 3a8581a..1708dee 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -29,6 +29,29 @@ struct bpf_prog;
>  struct dentry;
>  struct nsim_vf_config;
>  
> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
> +#define NSIM_IPSEC_MAX_SA_COUNT		33

33 caught my eye - out of curiosity is it 2^5 + 1 to catch some type of
bug or failure mode?

> +#define NSIM_IPSEC_VALID		BIT(31)
> +
> +struct nsim_sa {
> +	struct xfrm_state *xs;
> +	__be32 ipaddr[4];
> +	u32 key[4];
> +	u32 salt;
> +	bool used;
> +	bool crypt;
> +	bool rx;
> +};
> +
> +struct nsim_ipsec {
> +	struct nsim_sa sa[NSIM_IPSEC_MAX_SA_COUNT];
> +	struct dentry *pfile;
> +	u32 count;
> +	u32 tx;
> +	u32 ok;
> +};
> +#endif

No need to wrap struct definitions in #if/#endif.

>  struct netdevsim {
>  	struct net_device *netdev;
>  
> @@ -67,6 +90,9 @@ struct netdevsim {
>  #if IS_ENABLED(CONFIG_NET_DEVLINK)
>  	struct devlink *devlink;
>  #endif
> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
> +	struct nsim_ipsec ipsec;
> +#endif
>  };
>  
>  extern struct dentry *nsim_ddir;
> @@ -147,6 +173,17 @@ static inline void nsim_devlink_exit(void)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
> +void nsim_ipsec_init(struct netdevsim *ns);
> +void nsim_ipsec_teardown(struct netdevsim *ns);
> +int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb);
> +#else
> +static inline void nsim_ipsec_init(struct netdevsim *ns) {};
> +static inline void nsim_ipsec_teardown(struct netdevsim *ns) {};
> +static inline int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> +								{ return 1; };

Please use the same formatting for static inlines as the rest of the
file.  The ';' are also unnecessary.

Other than those formatting nit picks looks good to me :)

Re: [PATCH net-next 3/4] netdevsim: add ipsec offload testing

[Shannon Nelson]

On 6/22/2018 9:07 PM, Jakub Kicinski wrote:
> On Fri, 22 Jun 2018 17:31:37 -0700, Shannon Nelson wrote:
>> Implement the IPsec/XFRM offload API for testing.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> 
> Thanks for the patch!  Just a number of stylistic nit picks.

Thanks for the comments, I'll do a v2 in a couple of days.
sln

> 
>> diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
>> new file mode 100644
>> index 0000000..ad64266
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/ipsec.c
>> @@ -0,0 +1,345 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2018 Oracle and/or its affiliates. All rights reserved. */
>> +
>> +#include <net/xfrm.h>
>> +#include <crypto/aead.h>
>> +#include <linux/debugfs.h>
>> +#include "netdevsim.h"
> 
> Other files in the driver sort headers alphabetically and put an empty
> line between global and local headers.
> 
>> +#define NSIM_IPSEC_AUTH_BITS	128
>> +
>> +/**
>> + * nsim_ipsec_dbg_read - read for ipsec data
>> + * @filp: the opened file
>> + * @buffer: where to write the data for the user to read
>> + * @count: the size of the user's buffer
>> + * @ppos: file position offset
>> + **/
>> +static ssize_t nsim_dbg_netdev_ops_read(struct file *filp,
> 
> Doesn't match the kdoc.  Please run
> 
> ./scripts/kernel-doc -none $file
> 
> if you want kdoc.  Although IMHO you may as well drop the kdoc, your
> code is quite self explanatory and local.
> 
>> +					char __user *buffer,
>> +					size_t count, loff_t *ppos)
>> +{
>> +	struct netdevsim *ns = filp->private_data;
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +	size_t bufsize;
>> +	char *buf, *p;
>> +	int len;
>> +	int i;
>> +
>> +	/* don't allow partial reads */
>> +	if (*ppos != 0)
>> +		return 0;
>> +
>> +	/* the buffer needed is
>> +	 * (num SAs * 3 lines each * ~60 bytes per line) + one more line
>> +	 */
>> +	bufsize = (ipsec->count * 4 * 60) + 60;
>> +	buf = kzalloc(bufsize, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	p = buf;
>> +	p += snprintf(p, bufsize - (p - buf),
>> +		      "SA count=%u tx=%u\n",
>> +		      ipsec->count, ipsec->tx);
>> +
>> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
>> +		struct nsim_sa *sap = &ipsec->sa[i];
>> +
>> +		if (!sap->used)
>> +			continue;
>> +
>> +		p += snprintf(p, bufsize - (p - buf),
>> +			      "sa[%i] %cx ipaddr=0x%08x %08x %08x %08x\n",
>> +			      i, (sap->rx ? 'r' : 't'), sap->ipaddr[0],
>> +			      sap->ipaddr[1], sap->ipaddr[2], sap->ipaddr[3]);
>> +		p += snprintf(p, bufsize - (p - buf),
>> +			      "sa[%i]    spi=0x%08x proto=0x%x salt=0x%08x crypt=%d\n",
>> +			      i, be32_to_cpu(sap->xs->id.spi),
>> +			      sap->xs->id.proto, sap->salt, sap->crypt);
>> +		p += snprintf(p, bufsize - (p - buf),
>> +			      "sa[%i]    key=0x%08x %08x %08x %08x\n",
>> +			      i, sap->key[0], sap->key[1],
>> +			      sap->key[2], sap->key[3]);
>> +	}
>> +
>> +	len = simple_read_from_buffer(buffer, count, ppos, buf, p - buf);
> 
> Why not seq_file for this?
> 
>> +	kfree(buf);
>> +	return len;
>> +}
>> +
>> +static const struct file_operations ipsec_dbg_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = simple_open,
>> +	.read = nsim_dbg_netdev_ops_read,
>> +};
>> +
>> +/**
>> + * nsim_ipsec_find_empty_idx - find the first unused security parameter index
>> + * @ipsec: pointer to ipsec struct
>> + **/
>> +static int nsim_ipsec_find_empty_idx(struct nsim_ipsec *ipsec)
>> +{
>> +	u32 i;
>> +
>> +	if (ipsec->count == NSIM_IPSEC_MAX_SA_COUNT)
>> +		return -ENOSPC;
>> +
>> +	/* search sa table */
>> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
>> +		if (!ipsec->sa[i].used)
>> +			return i;
>> +	}
>> +
>> +	return -ENOSPC;
> 
> FWIW I personally find bitmaps and find_first_zero_bit() etc. nice and
> concise for a small ID allocator, but no objection to open coding.
> 
>> +}
>> +
>> +/**
>> + * nsim_ipsec_parse_proto_keys - find the key and salt based on the protocol
>> + * @xs: pointer to xfrm_state struct
>> + * @mykey: pointer to key array to populate
>> + * @mysalt: pointer to salt value to populate
>> + *
>> + * This copies the protocol keys and salt to our own data tables.  The
>> + * 82599 family only supports the one algorithm.
> 
> 82599 is a fine chip, it's not netdevsim tho? ;)
> 
>> + **/
>> +static int nsim_ipsec_parse_proto_keys(struct xfrm_state *xs,
>> +				       u32 *mykey, u32 *mysalt)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	unsigned char *key_data;
>> +	char *alg_name = NULL;
>> +	const char aes_gcm_name[] = "rfc4106(gcm(aes))";
>> +	int key_len;
> 
> reverse xmas tree please
> 
>> +
>> +	if (!xs->aead) {
>> +		netdev_err(dev, "Unsupported IPsec algorithm\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (xs->aead->alg_icv_len != NSIM_IPSEC_AUTH_BITS) {
>> +		netdev_err(dev, "IPsec offload requires %d bit authentication\n",
>> +			   NSIM_IPSEC_AUTH_BITS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	key_data = &xs->aead->alg_key[0];
>> +	key_len = xs->aead->alg_key_len;
>> +	alg_name = xs->aead->alg_name;
>> +
>> +	if (strcmp(alg_name, aes_gcm_name)) {
>> +		netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
>> +			   aes_gcm_name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* The key bytes come down in a bigendian array of bytes, so
>> +	 * we don't need to do any byteswapping.
> 
> Why the mention of bigendian?  82599 needs big endian? -.^
> 
>> +	 * 160 accounts for 16 byte key and 4 byte salt
>> +	 */
>> +	if (key_len > 128) {
> 
> s/128/NSIM_IPSEC_AUTH_BITS/ ?
> 
>> +		*mysalt = ((u32 *)key_data)[4];
> 
> Is alignment guaranteed?  There are the unaligned helpers if you need
> them..
> 
>> +	} else if (key_len == 128) {
>> +		*mysalt = 0;
>> +	} else {
>> +		netdev_err(dev, "IPsec hw offload only supports 128 bit keys with optional 32 bit salt\n");
>> +		return -EINVAL;
>> +	}
>> +	memcpy(mykey, key_data, 16);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * nsim_ipsec_add_sa - program device with a security association
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static int nsim_ipsec_add_sa(struct xfrm_state *xs)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	struct netdevsim *ns = netdev_priv(dev);
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> 
> xmas tree again (initialize out of line if you have to)
> 
>> +	struct nsim_sa sa;
>> +	u16 sa_idx;
>> +	int ret;
>> +
>> +	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
>> +		netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
>> +			   xs->id.proto);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (xs->calg) {
>> +		netdev_err(dev, "Compression offload not supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* find the first unused index */
>> +	ret = nsim_ipsec_find_empty_idx(ipsec);
>> +	if (ret < 0) {
>> +		netdev_err(dev, "No space for SA in Rx table!\n");
>> +		return ret;
>> +	}
>> +	sa_idx = (u16)ret;
>> +
>> +	memset(&sa, 0, sizeof(sa));
>> +	sa.used = true;
>> +	sa.xs = xs;
>> +
>> +	if (sa.xs->id.proto & IPPROTO_ESP)
>> +		sa.crypt = xs->ealg || xs->aead;
>> +
>> +	/* get the key and salt */
>> +	ret = nsim_ipsec_parse_proto_keys(xs, sa.key, &sa.salt);
>> +	if (ret) {
>> +		netdev_err(dev, "Failed to get key data for SA table\n");
>> +		return ret;
>> +	}
>> +
>> +	if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
>> +		sa.rx = true;
>> +
>> +		if (xs->props.family == AF_INET6)
>> +			memcpy(sa.ipaddr, &xs->id.daddr.a6, 16);
>> +		else
>> +			memcpy(&sa.ipaddr[3], &xs->id.daddr.a4, 4);
>> +	}
>> +
>> +	/* the preparations worked, so save the info */
>> +	memcpy(&ipsec->sa[sa_idx], &sa, sizeof(sa));
>> +
>> +	/* the XFRM stack doesn't like offload_handle == 0,
>> +	 * so add a bitflag in case our array index is 0
>> +	 */
>> +	xs->xso.offload_handle = sa_idx | NSIM_IPSEC_VALID;
>> +	ipsec->count++;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * nsim_ipsec_del_sa - clear out this specific SA
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static void nsim_ipsec_del_sa(struct xfrm_state *xs)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	struct netdevsim *ns = netdev_priv(dev);
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +	u16 sa_idx;
>> +
>> +	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
>> +	if (!ipsec->sa[sa_idx].used) {
>> +		netdev_err(dev, "Invalid SA for delete sa_idx=%d\n", sa_idx);
>> +		return;
>> +	}
>> +
>> +	memset(&ipsec->sa[sa_idx], 0, sizeof(struct nsim_sa));
>> +	ipsec->count--;
>> +}
>> +
>> +/**
>> + * nsim_ipsec_offload_ok - can this packet use the xfrm hw offload
>> + * @skb: current data packet
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static bool nsim_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	struct netdevsim *ns = netdev_priv(dev);
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +
>> +	ipsec->ok++;
>> +
>> +	return true;
>> +}
>> +
>> +static const struct xfrmdev_ops nsim_xfrmdev_ops = {
>> +	.xdo_dev_state_add = nsim_ipsec_add_sa,
>> +	.xdo_dev_state_delete = nsim_ipsec_del_sa,
>> +	.xdo_dev_offload_ok = nsim_ipsec_offload_ok,
> 
> Please align the initializers by adding tabs before '='.
> 
>> +};
>> +
>> +/**
>> + * nsim_ipsec_tx - check Tx packet for ipsec offload
>> + * @ns: pointer to ns structure
>> + * @skb: current data packet
>> + **/
>> +int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>> +{
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +	struct xfrm_state *xs;
>> +	struct nsim_sa *tsa;
>> +	u32 sa_idx;
>> +
>> +	/* do we even need to check this packet? */
>> +	if (!skb->sp)
>> +		return 1;
>> +
>> +	if (unlikely(!skb->sp->len)) {
>> +		netdev_err(ns->netdev, "%s: no xfrm state len = %d\n",
>> +			   __func__, skb->sp->len);
> 
> Hmm..  __func__ started appearing in errors?  Perhaps either always or
> never add it?
> 
> Also, I know this is not a real device, but please always use rate
> limited print functions on the data path.
> 
>> +		return 0;
>> +	}
>> +
>> +	xs = xfrm_input_state(skb);
>> +	if (unlikely(!xs)) {
>> +		netdev_err(ns->netdev, "%s: no xfrm_input_state() xs = %p\n",
>> +			   __func__, xs);
>> +		return 0;
>> +	}
>> +
>> +	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
>> +	if (unlikely(sa_idx > NSIM_IPSEC_MAX_SA_COUNT)) {
>> +		netdev_err(ns->netdev, "%s: bad sa_idx=%d max=%d\n",
>> +			   __func__, sa_idx, NSIM_IPSEC_MAX_SA_COUNT);
>> +		return 0;
>> +	}
>> +
>> +	tsa = &ipsec->sa[sa_idx];
>> +	if (unlikely(!tsa->used)) {
>> +		netdev_err(ns->netdev, "%s: unused sa_idx=%d\n",
>> +			   __func__, sa_idx);
>> +		return 0;
>> +	}
>> +
>> +	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
>> +		netdev_err(ns->netdev, "%s: unexpected proto=%d\n",
>> +			   __func__, xs->id.proto);
>> +		return 0;
>> +	}
>> +
>> +	ipsec->tx++;
>> +
>> +	return 1;
>> +}
> 
> Looks like the function should return bool?
> 
>> +
>> +/**
>> + * nsim_ipsec_init - initialize security registers for IPSec operation
>> + * @ns: board private structure
> 
> "board"?  Yes, the kdoc may be best removed ;)
> 
>> + **/
>> +void nsim_ipsec_init(struct netdevsim *ns)
>> +{
>> +	ns->netdev->xfrmdev_ops = &nsim_xfrmdev_ops;
>> +
>> +#define NSIM_ESP_FEATURES	(NETIF_F_HW_ESP | \
>> +				 NETIF_F_HW_ESP_TX_CSUM | \
>> +				 NETIF_F_GSO_ESP)
>> +
>> +	ns->netdev->features |= NSIM_ESP_FEATURES;
>> +	ns->netdev->hw_enc_features |= NSIM_ESP_FEATURES;
>> +
>> +	ns->ipsec.pfile = debugfs_create_file("ipsec", 0400, ns->ddir, ns,
>> +					      &ipsec_dbg_fops);
>> +}
>> +
>> +void nsim_ipsec_teardown(struct netdevsim *ns)
>> +{
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +
>> +	if (ipsec->count)
>> +		netdev_err(ns->netdev, "%s: tearing down IPsec offload with %d SAs left\n",
>> +			   __func__, ipsec->count);
>> +	debugfs_remove_recursive(ipsec->pfile);
>> +}
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index ec68f38..6ce8604d 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -171,6 +171,8 @@ static int nsim_init(struct net_device *dev)
>>   	if (err)
>>   		goto err_unreg_dev;
>>   
>> +	nsim_ipsec_init(ns);
>> +
>>   	return 0;
>>   
>>   err_unreg_dev:
>> @@ -186,6 +188,7 @@ static void nsim_uninit(struct net_device *dev)
>>   {
>>   	struct netdevsim *ns = netdev_priv(dev);
>>   
>> +	nsim_ipsec_teardown(ns);
>>   	nsim_devlink_teardown(ns);
>>   	debugfs_remove_recursive(ns->ddir);
>>   	nsim_bpf_uninit(ns);
>> @@ -203,11 +206,15 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>>   	struct netdevsim *ns = netdev_priv(dev);
>>   
>> +	if (!nsim_ipsec_tx(ns, skb))
>> +		goto out;
>> +
>>   	u64_stats_update_begin(&ns->syncp);
>>   	ns->tx_packets++;
>>   	ns->tx_bytes += skb->len;
>>   	u64_stats_update_end(&ns->syncp);
>>   
>> +out:
>>   	dev_kfree_skb(skb);
>>   
>>   	return NETDEV_TX_OK;
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index 3a8581a..1708dee 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -29,6 +29,29 @@ struct bpf_prog;
>>   struct dentry;
>>   struct nsim_vf_config;
>>   
>> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
>> +#define NSIM_IPSEC_MAX_SA_COUNT		33
> 
> 33 caught my eye - out of curiosity is it 2^5 + 1 to catch some type of
> bug or failure mode?
> 
>> +#define NSIM_IPSEC_VALID		BIT(31)
>> +
>> +struct nsim_sa {
>> +	struct xfrm_state *xs;
>> +	__be32 ipaddr[4];
>> +	u32 key[4];
>> +	u32 salt;
>> +	bool used;
>> +	bool crypt;
>> +	bool rx;
>> +};
>> +
>> +struct nsim_ipsec {
>> +	struct nsim_sa sa[NSIM_IPSEC_MAX_SA_COUNT];
>> +	struct dentry *pfile;
>> +	u32 count;
>> +	u32 tx;
>> +	u32 ok;
>> +};
>> +#endif
> 
> No need to wrap struct definitions in #if/#endif.
> 
>>   struct netdevsim {
>>   	struct net_device *netdev;
>>   
>> @@ -67,6 +90,9 @@ struct netdevsim {
>>   #if IS_ENABLED(CONFIG_NET_DEVLINK)
>>   	struct devlink *devlink;
>>   #endif
>> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
>> +	struct nsim_ipsec ipsec;
>> +#endif
>>   };
>>   
>>   extern struct dentry *nsim_ddir;
>> @@ -147,6 +173,17 @@ static inline void nsim_devlink_exit(void)
>>   }
>>   #endif
>>   
>> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
>> +void nsim_ipsec_init(struct netdevsim *ns);
>> +void nsim_ipsec_teardown(struct netdevsim *ns);
>> +int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb);
>> +#else
>> +static inline void nsim_ipsec_init(struct netdevsim *ns) {};
>> +static inline void nsim_ipsec_teardown(struct netdevsim *ns) {};
>> +static inline int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>> +								{ return 1; };
> 
> Please use the same formatting for static inlines as the rest of the
> file.  The ';' are also unnecessary.
> 
> Other than those formatting nit picks looks good to me :)
> 

Re: [PATCH net-next 3/4] netdevsim: add ipsec offload testing

[Shannon Nelson]

On 6/22/2018 9:07 PM, Jakub Kicinski wrote:
> On Fri, 22 Jun 2018 17:31:37 -0700, Shannon Nelson wrote:
>> Implement the IPsec/XFRM offload API for testing.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> 
> Thanks for the patch!  Just a number of stylistic nit picks.
> 
>> diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
>> new file mode 100644
>> index 0000000..ad64266
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/ipsec.c
>> @@ -0,0 +1,345 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2018 Oracle and/or its affiliates. All rights reserved. */
>> +
>> +#include <net/xfrm.h>
>> +#include <crypto/aead.h>
>> +#include <linux/debugfs.h>
>> +#include "netdevsim.h"
> 
> Other files in the driver sort headers alphabetically and put an empty
> line between global and local headers.

Sure.

> 
>> +#define NSIM_IPSEC_AUTH_BITS	128
>> +
>> +/**
>> + * nsim_ipsec_dbg_read - read for ipsec data
>> + * @filp: the opened file
>> + * @buffer: where to write the data for the user to read
>> + * @count: the size of the user's buffer
>> + * @ppos: file position offset
>> + **/
>> +static ssize_t nsim_dbg_netdev_ops_read(struct file *filp,
> 
> Doesn't match the kdoc.  Please run
> 
> ./scripts/kernel-doc -none $file
> 
> if you want kdoc.  Although IMHO you may as well drop the kdoc, your
> code is quite self explanatory and local.

By adding -v to that I got a couple of warnings that I didn't include 
the Return information - is that what you were commenting on?  The rest 
seems acceptable to the script I'm using from the net-next tree.

> 
>> +					char __user *buffer,
>> +					size_t count, loff_t *ppos)
>> +{
>> +	struct netdevsim *ns = filp->private_data;
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +	size_t bufsize;
>> +	char *buf, *p;
>> +	int len;
>> +	int i;
>> +
>> +	/* don't allow partial reads */
>> +	if (*ppos != 0)
>> +		return 0;
>> +
>> +	/* the buffer needed is
>> +	 * (num SAs * 3 lines each * ~60 bytes per line) + one more line
>> +	 */
>> +	bufsize = (ipsec->count * 4 * 60) + 60;
>> +	buf = kzalloc(bufsize, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	p = buf;
>> +	p += snprintf(p, bufsize - (p - buf),
>> +		      "SA count=%u tx=%u\n",
>> +		      ipsec->count, ipsec->tx);
>> +
>> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
>> +		struct nsim_sa *sap = &ipsec->sa[i];
>> +
>> +		if (!sap->used)
>> +			continue;
>> +
>> +		p += snprintf(p, bufsize - (p - buf),
>> +			      "sa[%i] %cx ipaddr=0x%08x %08x %08x %08x\n",
>> +			      i, (sap->rx ? 'r' : 't'), sap->ipaddr[0],
>> +			      sap->ipaddr[1], sap->ipaddr[2], sap->ipaddr[3]);
>> +		p += snprintf(p, bufsize - (p - buf),
>> +			      "sa[%i]    spi=0x%08x proto=0x%x salt=0x%08x crypt=%d\n",
>> +			      i, be32_to_cpu(sap->xs->id.spi),
>> +			      sap->xs->id.proto, sap->salt, sap->crypt);
>> +		p += snprintf(p, bufsize - (p - buf),
>> +			      "sa[%i]    key=0x%08x %08x %08x %08x\n",
>> +			      i, sap->key[0], sap->key[1],
>> +			      sap->key[2], sap->key[3]);
>> +	}
>> +
>> +	len = simple_read_from_buffer(buffer, count, ppos, buf, p - buf);
> 
> Why not seq_file for this?

Why bother with more interface code?  This is useful enough to support 
the API testing needed.

> 
>> +	kfree(buf);
>> +	return len;
>> +}
>> +
>> +static const struct file_operations ipsec_dbg_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = simple_open,
>> +	.read = nsim_dbg_netdev_ops_read,
>> +};
>> +
>> +/**
>> + * nsim_ipsec_find_empty_idx - find the first unused security parameter index
>> + * @ipsec: pointer to ipsec struct
>> + **/
>> +static int nsim_ipsec_find_empty_idx(struct nsim_ipsec *ipsec)
>> +{
>> +	u32 i;
>> +
>> +	if (ipsec->count == NSIM_IPSEC_MAX_SA_COUNT)
>> +		return -ENOSPC;
>> +
>> +	/* search sa table */
>> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
>> +		if (!ipsec->sa[i].used)
>> +			return i;
>> +	}
>> +
>> +	return -ENOSPC;
> 
> FWIW I personally find bitmaps and find_first_zero_bit() etc. nice and
> concise for a small ID allocator, but no objection to open coding.

Sure, we could add a parallel bitmap data structure to track usage of 
our array elements, and probably would for a much larger array so as to 
lessen the impact of a serial search.  But, since this is a short array 
for simple testing purposes, the search time is minimal so I think the 
simple code is fine.

> 
>> +}
>> +
>> +/**
>> + * nsim_ipsec_parse_proto_keys - find the key and salt based on the protocol
>> + * @xs: pointer to xfrm_state struct
>> + * @mykey: pointer to key array to populate
>> + * @mysalt: pointer to salt value to populate
>> + *
>> + * This copies the protocol keys and salt to our own data tables.  The
>> + * 82599 family only supports the one algorithm.
> 
> 82599 is a fine chip, it's not netdevsim tho? ;)

Yeah, guess where I hacked the code from...  Thanks, I missed this 
reference.

> 
>> + **/
>> +static int nsim_ipsec_parse_proto_keys(struct xfrm_state *xs,
>> +				       u32 *mykey, u32 *mysalt)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	unsigned char *key_data;
>> +	char *alg_name = NULL;
>> +	const char aes_gcm_name[] = "rfc4106(gcm(aes))";
>> +	int key_len;
> 
> reverse xmas tree please

Yep, missed it here.

> 
>> +
>> +	if (!xs->aead) {
>> +		netdev_err(dev, "Unsupported IPsec algorithm\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (xs->aead->alg_icv_len != NSIM_IPSEC_AUTH_BITS) {
>> +		netdev_err(dev, "IPsec offload requires %d bit authentication\n",
>> +			   NSIM_IPSEC_AUTH_BITS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	key_data = &xs->aead->alg_key[0];
>> +	key_len = xs->aead->alg_key_len;
>> +	alg_name = xs->aead->alg_name;
>> +
>> +	if (strcmp(alg_name, aes_gcm_name)) {
>> +		netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
>> +			   aes_gcm_name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* The key bytes come down in a bigendian array of bytes, so
>> +	 * we don't need to do any byteswapping.
> 
> Why the mention of bigendian?  82599 needs big endian? -.^

Yep, another useless reference left over from the hack-n-slash - I'll 
remove it.

> 
>> +	 * 160 accounts for 16 byte key and 4 byte salt
>> +	 */
>> +	if (key_len > 128) {
> 
> s/128/NSIM_IPSEC_AUTH_BITS/ ?

Sure.

> 
>> +		*mysalt = ((u32 *)key_data)[4];
> 
> Is alignment guaranteed?  There are the unaligned helpers if you need
> them..

Since the key_data must be at least 128 bits in this implementation, and 
the key itself is 128 bits, anything after is salt, so we can assume 
that the salt data is aligned.

> 
>> +	} else if (key_len == 128) {
>> +		*mysalt = 0;
>> +	} else {
>> +		netdev_err(dev, "IPsec hw offload only supports 128 bit keys with optional 32 bit salt\n");
>> +		return -EINVAL;
>> +	}
>> +	memcpy(mykey, key_data, 16);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * nsim_ipsec_add_sa - program device with a security association
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static int nsim_ipsec_add_sa(struct xfrm_state *xs)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	struct netdevsim *ns = netdev_priv(dev);
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> 
> xmas tree again (initialize out of line if you have to)

This one is pretty much the way I've done in the past with no complaints 
and seems common enough in other net drivers, specifically when dealing 
with netdev and netdevpriv elements.  Only the first line is out of 
place, with the next lines dependent on it.

> 
>> +	struct nsim_sa sa;
>> +	u16 sa_idx;
>> +	int ret;
>> +
>> +	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
>> +		netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
>> +			   xs->id.proto);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (xs->calg) {
>> +		netdev_err(dev, "Compression offload not supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* find the first unused index */
>> +	ret = nsim_ipsec_find_empty_idx(ipsec);
>> +	if (ret < 0) {
>> +		netdev_err(dev, "No space for SA in Rx table!\n");
>> +		return ret;
>> +	}
>> +	sa_idx = (u16)ret;
>> +
>> +	memset(&sa, 0, sizeof(sa));
>> +	sa.used = true;
>> +	sa.xs = xs;
>> +
>> +	if (sa.xs->id.proto & IPPROTO_ESP)
>> +		sa.crypt = xs->ealg || xs->aead;
>> +
>> +	/* get the key and salt */
>> +	ret = nsim_ipsec_parse_proto_keys(xs, sa.key, &sa.salt);
>> +	if (ret) {
>> +		netdev_err(dev, "Failed to get key data for SA table\n");
>> +		return ret;
>> +	}
>> +
>> +	if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
>> +		sa.rx = true;
>> +
>> +		if (xs->props.family == AF_INET6)
>> +			memcpy(sa.ipaddr, &xs->id.daddr.a6, 16);
>> +		else
>> +			memcpy(&sa.ipaddr[3], &xs->id.daddr.a4, 4);
>> +	}
>> +
>> +	/* the preparations worked, so save the info */
>> +	memcpy(&ipsec->sa[sa_idx], &sa, sizeof(sa));
>> +
>> +	/* the XFRM stack doesn't like offload_handle == 0,
>> +	 * so add a bitflag in case our array index is 0
>> +	 */
>> +	xs->xso.offload_handle = sa_idx | NSIM_IPSEC_VALID;
>> +	ipsec->count++;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * nsim_ipsec_del_sa - clear out this specific SA
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static void nsim_ipsec_del_sa(struct xfrm_state *xs)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	struct netdevsim *ns = netdev_priv(dev);
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +	u16 sa_idx;
>> +
>> +	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
>> +	if (!ipsec->sa[sa_idx].used) {
>> +		netdev_err(dev, "Invalid SA for delete sa_idx=%d\n", sa_idx);
>> +		return;
>> +	}
>> +
>> +	memset(&ipsec->sa[sa_idx], 0, sizeof(struct nsim_sa));
>> +	ipsec->count--;
>> +}
>> +
>> +/**
>> + * nsim_ipsec_offload_ok - can this packet use the xfrm hw offload
>> + * @skb: current data packet
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static bool nsim_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>> +{
>> +	struct net_device *dev = xs->xso.dev;
>> +	struct netdevsim *ns = netdev_priv(dev);
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +
>> +	ipsec->ok++;
>> +
>> +	return true;
>> +}
>> +
>> +static const struct xfrmdev_ops nsim_xfrmdev_ops = {
>> +	.xdo_dev_state_add = nsim_ipsec_add_sa,
>> +	.xdo_dev_state_delete = nsim_ipsec_del_sa,
>> +	.xdo_dev_offload_ok = nsim_ipsec_offload_ok,
> 
> Please align the initializers by adding tabs before '='.

Sure.

> 
>> +};
>> +
>> +/**
>> + * nsim_ipsec_tx - check Tx packet for ipsec offload
>> + * @ns: pointer to ns structure
>> + * @skb: current data packet
>> + **/
>> +int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>> +{
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +	struct xfrm_state *xs;
>> +	struct nsim_sa *tsa;
>> +	u32 sa_idx;
>> +
>> +	/* do we even need to check this packet? */
>> +	if (!skb->sp)
>> +		return 1;
>> +
>> +	if (unlikely(!skb->sp->len)) {
>> +		netdev_err(ns->netdev, "%s: no xfrm state len = %d\n",
>> +			   __func__, skb->sp->len);
> 
> Hmm..  __func__ started appearing in errors?  Perhaps either always or
> never add it?
> 
> Also, I know this is not a real device, but please always use rate
> limited print functions on the data path.
> 
>> +		return 0;
>> +	}
>> +
>> +	xs = xfrm_input_state(skb);
>> +	if (unlikely(!xs)) {
>> +		netdev_err(ns->netdev, "%s: no xfrm_input_state() xs = %p\n",
>> +			   __func__, xs);
>> +		return 0;
>> +	}
>> +
>> +	sa_idx = xs->xso.offload_handle & ~NSIM_IPSEC_VALID;
>> +	if (unlikely(sa_idx > NSIM_IPSEC_MAX_SA_COUNT)) {
>> +		netdev_err(ns->netdev, "%s: bad sa_idx=%d max=%d\n",
>> +			   __func__, sa_idx, NSIM_IPSEC_MAX_SA_COUNT);
>> +		return 0;
>> +	}
>> +
>> +	tsa = &ipsec->sa[sa_idx];
>> +	if (unlikely(!tsa->used)) {
>> +		netdev_err(ns->netdev, "%s: unused sa_idx=%d\n",
>> +			   __func__, sa_idx);
>> +		return 0;
>> +	}
>> +
>> +	if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
>> +		netdev_err(ns->netdev, "%s: unexpected proto=%d\n",
>> +			   __func__, xs->id.proto);
>> +		return 0;
>> +	}
>> +
>> +	ipsec->tx++;
>> +
>> +	return 1;
>> +}
> 
> Looks like the function should return bool?

Sure.

> 
>> +
>> +/**
>> + * nsim_ipsec_init - initialize security registers for IPSec operation
>> + * @ns: board private structure
> 
> "board"?  Yes, the kdoc may be best removed ;)
> 
>> + **/
>> +void nsim_ipsec_init(struct netdevsim *ns)
>> +{
>> +	ns->netdev->xfrmdev_ops = &nsim_xfrmdev_ops;
>> +
>> +#define NSIM_ESP_FEATURES	(NETIF_F_HW_ESP | \
>> +				 NETIF_F_HW_ESP_TX_CSUM | \
>> +				 NETIF_F_GSO_ESP)
>> +
>> +	ns->netdev->features |= NSIM_ESP_FEATURES;
>> +	ns->netdev->hw_enc_features |= NSIM_ESP_FEATURES;
>> +
>> +	ns->ipsec.pfile = debugfs_create_file("ipsec", 0400, ns->ddir, ns,
>> +					      &ipsec_dbg_fops);
>> +}
>> +
>> +void nsim_ipsec_teardown(struct netdevsim *ns)
>> +{
>> +	struct nsim_ipsec *ipsec = &ns->ipsec;
>> +
>> +	if (ipsec->count)
>> +		netdev_err(ns->netdev, "%s: tearing down IPsec offload with %d SAs left\n",
>> +			   __func__, ipsec->count);
>> +	debugfs_remove_recursive(ipsec->pfile);
>> +}
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index ec68f38..6ce8604d 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -171,6 +171,8 @@ static int nsim_init(struct net_device *dev)
>>   	if (err)
>>   		goto err_unreg_dev;
>>   
>> +	nsim_ipsec_init(ns);
>> +
>>   	return 0;
>>   
>>   err_unreg_dev:
>> @@ -186,6 +188,7 @@ static void nsim_uninit(struct net_device *dev)
>>   {
>>   	struct netdevsim *ns = netdev_priv(dev);
>>   
>> +	nsim_ipsec_teardown(ns);
>>   	nsim_devlink_teardown(ns);
>>   	debugfs_remove_recursive(ns->ddir);
>>   	nsim_bpf_uninit(ns);
>> @@ -203,11 +206,15 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>>   	struct netdevsim *ns = netdev_priv(dev);
>>   
>> +	if (!nsim_ipsec_tx(ns, skb))
>> +		goto out;
>> +
>>   	u64_stats_update_begin(&ns->syncp);
>>   	ns->tx_packets++;
>>   	ns->tx_bytes += skb->len;
>>   	u64_stats_update_end(&ns->syncp);
>>   
>> +out:
>>   	dev_kfree_skb(skb);
>>   
>>   	return NETDEV_TX_OK;
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index 3a8581a..1708dee 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -29,6 +29,29 @@ struct bpf_prog;
>>   struct dentry;
>>   struct nsim_vf_config;
>>   
>> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
>> +#define NSIM_IPSEC_MAX_SA_COUNT		33
> 
> 33 caught my eye - out of curiosity is it 2^5 + 1 to catch some type of
> bug or failure mode?

For test rigs, I often use something like this to help flush out any 
interesting power-of-two or alignment assumptions in the code.  I don't 
expect anything here, but it doesn't hurt.

> 
>> +#define NSIM_IPSEC_VALID		BIT(31)
>> +
>> +struct nsim_sa {
>> +	struct xfrm_state *xs;
>> +	__be32 ipaddr[4];
>> +	u32 key[4];
>> +	u32 salt;
>> +	bool used;
>> +	bool crypt;
>> +	bool rx;
>> +};
>> +
>> +struct nsim_ipsec {
>> +	struct nsim_sa sa[NSIM_IPSEC_MAX_SA_COUNT];
>> +	struct dentry *pfile;
>> +	u32 count;
>> +	u32 tx;
>> +	u32 ok;
>> +};
>> +#endif
> 
> No need to wrap struct definitions in #if/#endif.

I suppose this is a philosophical point... Since CONFIG_XFRM_OFFLOAD is 
not yet a common config setting, I'd like to keep it here to not break 
other folks' builds or dirty them up with unused struct definitions when 
they aren't playing with IPsec offload anyway.

> 
>>   struct netdevsim {
>>   	struct net_device *netdev;
>>   
>> @@ -67,6 +90,9 @@ struct netdevsim {
>>   #if IS_ENABLED(CONFIG_NET_DEVLINK)
>>   	struct devlink *devlink;
>>   #endif
>> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
>> +	struct nsim_ipsec ipsec;
>> +#endif
>>   };
>>   
>>   extern struct dentry *nsim_ddir;
>> @@ -147,6 +173,17 @@ static inline void nsim_devlink_exit(void)
>>   }
>>   #endif
>>   
>> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
>> +void nsim_ipsec_init(struct netdevsim *ns);
>> +void nsim_ipsec_teardown(struct netdevsim *ns);
>> +int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb);
>> +#else
>> +static inline void nsim_ipsec_init(struct netdevsim *ns) {};
>> +static inline void nsim_ipsec_teardown(struct netdevsim *ns) {};
>> +static inline int nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>> +								{ return 1; };
> 
> Please use the same formatting for static inlines as the rest of the
> file.  The ';' are also unnecessary.

Sure

> 
> Other than those formatting nit picks looks good to me :)
> 

Cheers,
sln

Re: [PATCH net-next 3/4] netdevsim: add ipsec offload testing

[Jakub Kicinski]

On Mon, 25 Jun 2018 15:37:10 -0700, Shannon Nelson wrote:
> On 6/22/2018 9:07 PM, Jakub Kicinski wrote:
> > On Fri, 22 Jun 2018 17:31:37 -0700, Shannon Nelson wrote:  
> >> Implement the IPsec/XFRM offload API for testing.
> >>
> >> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>  
> >> +#define NSIM_IPSEC_AUTH_BITS	128
> >> +
> >> +/**
> >> + * nsim_ipsec_dbg_read - read for ipsec data
> >> + * @filp: the opened file
> >> + * @buffer: where to write the data for the user to read
> >> + * @count: the size of the user's buffer
> >> + * @ppos: file position offset
> >> + **/
> >> +static ssize_t nsim_dbg_netdev_ops_read(struct file *filp,  
> > 
> > Doesn't match the kdoc.  Please run
> > 
> > ./scripts/kernel-doc -none $file
> > 
> > if you want kdoc.  Although IMHO you may as well drop the kdoc, your
> > code is quite self explanatory and local.  
> 
> By adding -v to that I got a couple of warnings that I didn't include 
> the Return information - is that what you were commenting on?  The rest 
> seems acceptable to the script I'm using from the net-next tree.

Hm, strange.  Two things: first kdoc requires () after function name,
second the function is called nsim_dbg_netdev_ops_read() while the doc
refers to nsim_ipsec_dbg_read().  Perhaps the combination of the two
makes the script miss the problem. 

> >> +					char __user *buffer,
> >> +					size_t count, loff_t *ppos)
> >> +{
> >> +	struct netdevsim *ns = filp->private_data;
> >> +	struct nsim_ipsec *ipsec = &ns->ipsec;
> >> +	size_t bufsize;
> >> +	char *buf, *p;
> >> +	int len;
> >> +	int i;
> >> +
> >> +	/* don't allow partial reads */
> >> +	if (*ppos != 0)
> >> +		return 0;
> >> +
> >> +	/* the buffer needed is
> >> +	 * (num SAs * 3 lines each * ~60 bytes per line) + one more line
> >> +	 */
> >> +	bufsize = (ipsec->count * 4 * 60) + 60;
> >> +	buf = kzalloc(bufsize, GFP_KERNEL);
> >> +	if (!buf)
> >> +		return -ENOMEM;
> >> +
> >> +	p = buf;
> >> +	p += snprintf(p, bufsize - (p - buf),
> >> +		      "SA count=%u tx=%u\n",
> >> +		      ipsec->count, ipsec->tx);
> >> +
> >> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
> >> +		struct nsim_sa *sap = &ipsec->sa[i];
> >> +
> >> +		if (!sap->used)
> >> +			continue;
> >> +
> >> +		p += snprintf(p, bufsize - (p - buf),
> >> +			      "sa[%i] %cx ipaddr=0x%08x %08x %08x %08x\n",
> >> +			      i, (sap->rx ? 'r' : 't'), sap->ipaddr[0],
> >> +			      sap->ipaddr[1], sap->ipaddr[2], sap->ipaddr[3]);
> >> +		p += snprintf(p, bufsize - (p - buf),
> >> +			      "sa[%i]    spi=0x%08x proto=0x%x salt=0x%08x crypt=%d\n",
> >> +			      i, be32_to_cpu(sap->xs->id.spi),
> >> +			      sap->xs->id.proto, sap->salt, sap->crypt);
> >> +		p += snprintf(p, bufsize - (p - buf),
> >> +			      "sa[%i]    key=0x%08x %08x %08x %08x\n",
> >> +			      i, sap->key[0], sap->key[1],
> >> +			      sap->key[2], sap->key[3]);
> >> +	}
> >> +
> >> +	len = simple_read_from_buffer(buffer, count, ppos, buf, p - buf);  
> > 
> > Why not seq_file for this?  
> 
> Why bother with more interface code?  This is useful enough to support 
> the API testing needed.

No objection on this, seq_file is less error prone, but I don't mind.
FWIW you can drop the *ppos == 0 requirement, simple_read_from_buffer()
will handle other cases just fine.

> >> +	kfree(buf);
> >> +	return len;
> >> +}
> >> +
> >> +static const struct file_operations ipsec_dbg_fops = {
> >> +	.owner = THIS_MODULE,
> >> +	.open = simple_open,
> >> +	.read = nsim_dbg_netdev_ops_read,
> >> +};
> >> +
> >> +/**
> >> + * nsim_ipsec_find_empty_idx - find the first unused security parameter index
> >> + * @ipsec: pointer to ipsec struct
> >> + **/
> >> +static int nsim_ipsec_find_empty_idx(struct nsim_ipsec *ipsec)
> >> +{
> >> +	u32 i;
> >> +
> >> +	if (ipsec->count == NSIM_IPSEC_MAX_SA_COUNT)
> >> +		return -ENOSPC;
> >> +
> >> +	/* search sa table */
> >> +	for (i = 0; i < NSIM_IPSEC_MAX_SA_COUNT; i++) {
> >> +		if (!ipsec->sa[i].used)
> >> +			return i;
> >> +	}
> >> +
> >> +	return -ENOSPC;  
> > 
> > FWIW I personally find bitmaps and find_first_zero_bit() etc. nice and
> > concise for a small ID allocator, but no objection to open coding.  
> 
> Sure, we could add a parallel bitmap data structure to track usage of 
> our array elements, and probably would for a much larger array so as to 
> lessen the impact of a serial search.  But, since this is a short array 
> for simple testing purposes, the search time is minimal so I think the 
> simple code is fine.

Ack, no objection.

> >   
> >> +	} else if (key_len == 128) {
> >> +		*mysalt = 0;
> >> +	} else {
> >> +		netdev_err(dev, "IPsec hw offload only supports 128 bit keys with optional 32 bit salt\n");
> >> +		return -EINVAL;
> >> +	}
> >> +	memcpy(mykey, key_data, 16);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * nsim_ipsec_add_sa - program device with a security association
> >> + * @xs: pointer to transformer state struct
> >> + **/
> >> +static int nsim_ipsec_add_sa(struct xfrm_state *xs)
> >> +{
> >> +	struct net_device *dev = xs->xso.dev;
> >> +	struct netdevsim *ns = netdev_priv(dev);
> >> +	struct nsim_ipsec *ipsec = &ns->ipsec;  
> > 
> > xmas tree again (initialize out of line if you have to)  
> 
> This one is pretty much the way I've done in the past with no complaints 
> and seems common enough in other net drivers, specifically when dealing 
> with netdev and netdevpriv elements.  Only the first line is out of 
> place, with the next lines dependent on it.

I know, but I'd really prefer you just followed the rule here.

> >> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> >> index 3a8581a..1708dee 100644
> >> --- a/drivers/net/netdevsim/netdevsim.h
> >> +++ b/drivers/net/netdevsim/netdevsim.h
> >> @@ -29,6 +29,29 @@ struct bpf_prog;
> >>   struct dentry;
> >>   struct nsim_vf_config;
> >>   
> >> +#if IS_ENABLED(CONFIG_XFRM_OFFLOAD)
> >> +#define NSIM_IPSEC_MAX_SA_COUNT		33  
> > 
> > 33 caught my eye - out of curiosity is it 2^5 + 1 to catch some type of
> > bug or failure mode?  
> 
> For test rigs, I often use something like this to help flush out any 
> interesting power-of-two or alignment assumptions in the code.  I don't 
> expect anything here, but it doesn't hurt.

Cool, seems like a good idea anyway!

> >> +#define NSIM_IPSEC_VALID		BIT(31)
> >> +
> >> +struct nsim_sa {
> >> +	struct xfrm_state *xs;
> >> +	__be32 ipaddr[4];
> >> +	u32 key[4];
> >> +	u32 salt;
> >> +	bool used;
> >> +	bool crypt;
> >> +	bool rx;
> >> +};
> >> +
> >> +struct nsim_ipsec {
> >> +	struct nsim_sa sa[NSIM_IPSEC_MAX_SA_COUNT];
> >> +	struct dentry *pfile;
> >> +	u32 count;
> >> +	u32 tx;
> >> +	u32 ok;
> >> +};
> >> +#endif  
> > 
> > No need to wrap struct definitions in #if/#endif.  
> 
> I suppose this is a philosophical point... Since CONFIG_XFRM_OFFLOAD is 
> not yet a common config setting, I'd like to keep it here to not break 
> other folks' builds or dirty them up with unused struct definitions when 
> they aren't playing with IPsec offload anyway.

The fewer ifdefs in the code the better.  Both from pure LoC stand
point but also because someone may actually change things around
without having CONFIG_XFRM_OFFLOAD enabled and break *your*
configuration.  E.g. you are depending on indirect include of
net/xfrm.h, what if someone was to change headers around and broke that
implicit dependency?  The fewer ifdefs the better.

[PATCH net-next 4/4] selftests: rtnetlink: add ipsec offload API test

[Shannon Nelson]

Using the netdevsim as a device for testing, try out the XFRM commands
for setting up IPsec hardware offloads.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 tools/testing/selftests/net/rtnetlink.sh | 114 +++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 15948cf..9e1a82e 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -608,6 +608,119 @@ kci_test_ipsec()
 	echo "PASS: ipsec"
 }
 
+#-------------------------------------------------------------------
+# Example commands
+#   ip x s add proto esp src 14.0.0.52 dst 14.0.0.70 \
+#            spi 0x07 mode transport reqid 0x07 replay-window 32 \
+#            aead 'rfc4106(gcm(aes))' 1234567890123456dcba 128 \
+#            sel src 14.0.0.52/24 dst 14.0.0.70/24
+#            offload dev sim1 dir out
+#   ip x p add dir out src 14.0.0.52/24 dst 14.0.0.70/24 \
+#            tmpl proto esp src 14.0.0.52 dst 14.0.0.70 \
+#            spi 0x07 mode transport reqid 0x07
+#
+#-------------------------------------------------------------------
+kci_test_ipsec_offload()
+{
+	ret=0
+	algo="aead rfc4106(gcm(aes)) 0x3132333435363738393031323334353664636261 128"
+	srcip=192.168.123.3
+	dstip=192.168.123.4
+	dev=simx1
+	sysfsd=/sys/kernel/debug/netdevsim/$dev
+	sysfsf=$sysfsd/ipsec
+
+	# setup netdevsim since dummydev doesn't have offload support
+	modprobe netdevsim
+	check_err $?
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: ipsec_offload can't load netdevsim"
+		return 1
+	fi
+
+	ip link add $dev type netdevsim
+	ip addr add $srcip dev $dev
+	ip link set $dev up
+	if [ ! -d $sysfsd ] ; then
+		echo "FAIL: ipsec_offload can't create device $dev"
+		return 1
+	fi
+	if [ ! -f $sysfsf ] ; then
+		echo "FAIL: ipsec_offload netdevsim doesn't support IPsec offload"
+		return 1
+	fi
+
+	# flush to be sure there's nothing configured
+	ip x s flush ; ip x p flush
+
+	# create offloaded SAs, both in and out
+	ip x p add dir out src $srcip/24 dst $dstip/24 \
+	    tmpl proto esp src $srcip dst $dstip spi 9 \
+	    mode transport reqid 42
+	check_err $?
+	ip x p add dir out src $dstip/24 dst $srcip/24 \
+	    tmpl proto esp src $dstip dst $srcip spi 9 \
+	    mode transport reqid 42
+	check_err $?
+
+	ip x s add proto esp src $srcip dst $dstip spi 9 \
+	    mode transport reqid 42 $algo sel src $srcip/24 dst $dstip/24 \
+	    offload dev $dev dir out
+	check_err $?
+	ip x s add proto esp src $dstip dst $srcip spi 9 \
+	    mode transport reqid 42 $algo sel src $dstip/24 dst $srcip/24 \
+	    offload dev $dev dir in
+	check_err $?
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: ipsec_offload can't create SA"
+		return 1
+	fi
+
+	# does offload show up in ip output
+	lines=`ip x s list | grep -c "crypto offload parameters: dev $dev dir"`
+	if [ $lines -ne 2 ] ; then
+		echo "FAIL: ipsec_offload SA offload missing from list output"
+		check_err 1
+	fi
+
+	# use ping to exercise the Tx path
+	ping -I $dev -c 3 -W 1 -i 0 $dstip >/dev/null
+
+	# does driver have correct offload info
+	diff $sysfsf - << EOF
+SA count=2 tx=3
+sa[0] tx ipaddr=0x00000000 00000000 00000000 00000000
+sa[0]    spi=0x00000009 proto=0x32 salt=0x61626364 crypt=1
+sa[0]    key=0x34333231 38373635 32313039 36353433
+sa[1] rx ipaddr=0x00000000 00000000 00000000 037ba8c0
+sa[1]    spi=0x00000009 proto=0x32 salt=0x61626364 crypt=1
+sa[1]    key=0x34333231 38373635 32313039 36353433
+EOF
+	if [ $? -ne 0 ] ; then
+		echo "FAIL: ipsec_offload incorrect driver data"
+		check_err 1
+	fi
+
+	# does offload get removed from driver
+	ip x s flush
+	ip x p flush
+	lines=`grep -c "SA count=0" $sysfsf`
+	if [ $lines -ne 1 ] ; then
+		echo "FAIL: ipsec_offload SA not removed from driver"
+		check_err 1
+	fi
+
+	# clean up any leftovers
+	ip link del $dev
+	rmmod netdevsim
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: ipsec_offload"
+		return 1
+	fi
+	echo "PASS: ipsec_offload"
+}
+
 kci_test_gretap()
 {
 	testns="testns"
@@ -862,6 +975,7 @@ kci_test_rtnl()
 	kci_test_encap
 	kci_test_macsec
 	kci_test_ipsec
+	kci_test_ipsec_offload
 
 	kci_del_dummy
 }
-- 
2.7.4