[PATCHv2 net-next 0/5] pktgen IPsec support

[PATCHv2 net-next 0/5] pktgen IPsec support

[Fan Du]

Hi, Dave/Steffen

Current pktgen IPsec supports only transport/ESP combinnation,
and it's buggy before I fixed transformed IP header/length issue
a few days ago.

This patchset enables user to do almost any IPsec transformation,
both transport/tunnel mode, and AH/ESP/IPcomp type.

Below configuration has been tested, and using Wireshark could decrypt
out plain text in good formation without any checksum/auth errors:

Mode/TYPE	AH	ESP
Transport	x	x
Tunnel		x	x

ChangeLog
v2:
  Rebase patchset against newest net-next.
  Patch1: Remove adding rebundant empty line spotted by Sergei.
  Patch2: Use only one dst pointing into itself to save space.

Fan Du (5):
  {pktgen, xfrm} Remove original pktgen ipsec fixed configuration
  {pktgen, xfrm} Using "pgset spi xxx" to spedifiy SA for a given flow
  {pktgen, xfrm} Construct skb dst for tunnel mode transformation
  {pktgen, xfrm} Introduce xfrm_state_lookup_byspi for pktgen
  {pktgen, xfrm} Correct xfrm state lock usage when transforming

 include/net/xfrm.h    |    7 ++-----
 net/core/pktgen.c     |   55 +++++++++++++++++++++++++++++++++----------------
 net/xfrm/xfrm_state.c |   43 +++++++++++++++-----------------------
 3 files changed, 56 insertions(+), 49 deletions(-)

-- 
1.7.9.5

[PATCHv2 net-next 1/5] {pktgen, xfrm} Remove original pktgen ipsec fixed configuration

[Fan Du]

Cleanup original fixed IPsec output mode and encapuslation type,
As following patchset enable user to configure IPsec attribute.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/core/pktgen.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index a797fff..e6820e5 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -387,8 +387,6 @@ struct pktgen_dev {
 	int node;               /* Memory node */
 
 #ifdef CONFIG_XFRM
-	__u8	ipsmode;		/* IPSEC mode (config) */
-	__u8	ipsproto;		/* IPSEC type (config) */
 #endif
 	char result[512];
 };
@@ -2482,10 +2480,6 @@ static int pktgen_output_ipsec(struct sk_buff *skb, struct pktgen_dev *pkt_dev)
 
 	if (!x)
 		return 0;
-	/* XXX: we dont support tunnel mode for now until
-	 * we resolve the dst issue */
-	if (x->props.mode != XFRM_MODE_TRANSPORT)
-		return 0;
 
 	spin_lock(&x->lock);
 
@@ -3540,8 +3534,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 		goto out2;
 	}
 #ifdef CONFIG_XFRM
-	pkt_dev->ipsmode = XFRM_MODE_TRANSPORT;
-	pkt_dev->ipsproto = IPPROTO_ESP;
 #endif
 
 	return add_dev_to_thread(t, pkt_dev);
-- 
1.7.9.5

[PATCHv2 net-next 2/5] {pktgen, xfrm} Using "pgset spi xxx" to spedifiy SA for a given flow

[Fan Du]

User could set specific SPI value to arm pktgen flow with IPsec
transformation, instead of looking up SA by sadr/daddr. The reaseon
to do so is because current state lookup scheme is both slow and, most
important of all, in fact pktgen doesn't need to match any SA state
addresses information, all it needs is the SA transfromation shell to
do the encapuslation.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/core/pktgen.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index e6820e5..717e0b7 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -387,6 +387,7 @@ struct pktgen_dev {
 	int node;               /* Memory node */
 
 #ifdef CONFIG_XFRM
+	__u32 spi;
 #endif
 	char result[512];
 };
@@ -1474,6 +1475,16 @@ static ssize_t pktgen_if_write(struct file *file,
 		sprintf(pg_result, "OK: flows=%u", pkt_dev->cflows);
 		return count;
 	}
+	if (!strcmp(name, "spi")) {
+		len = num_arg(&user_buffer[i], 10, &value);
+		if (len < 0)
+			return len;
+
+		i += len;
+		pkt_dev->spi = value;
+		sprintf(pg_result, "OK: spi=%u", pkt_dev->spi);
+		return count;
+	}
 
 	if (!strcmp(name, "flowlen")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-- 
1.7.9.5

[PATCHv2 net-next 3/5] {pktgen,xfrm} Construct skb dst for tunnel mode transformation

[Fan Du]

IPsec tunnel mode encapuslation needs to set outter ip header
with right protocol/ttl/id value with regard to skb->dst->child.

Looking up a rt in a standard way is absolutely wrong for every
packet transmission. In a simple way, construct a dst by setting
neccessary information to make tunnel mode encapuslation working.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
v2:
 -Point dst->child into dst itself to save space.
---
 net/core/pktgen.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 717e0b7..2f4f90c 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -388,6 +388,8 @@ struct pktgen_dev {
 
 #ifdef CONFIG_XFRM
 	__u32 spi;
+	struct dst_entry dst;
+	struct dst_ops dstops;
 #endif
 	char result[512];
 };
@@ -2484,6 +2486,11 @@ static void mod_cur_headers(struct pktgen_dev *pkt_dev)
 
 
 #ifdef CONFIG_XFRM
+u32 pktgen_dst_metrics[RTAX_MAX + 1] = {
+
+	[RTAX_HOPLIMIT] = 0x5, /* Set a static hoplimit */
+};
+
 static int pktgen_output_ipsec(struct sk_buff *skb, struct pktgen_dev *pkt_dev)
 {
 	struct xfrm_state *x = pkt_dev->flows[pkt_dev->curfl].x;
@@ -2492,6 +2499,9 @@ static int pktgen_output_ipsec(struct sk_buff *skb, struct pktgen_dev *pkt_dev)
 	if (!x)
 		return 0;
 
+	if (x->props.mode == XFRM_MODE_TUNNEL)
+		__skb_dst_set_noref(skb, &pkt_dev->dst, true);
+
 	spin_lock(&x->lock);
 
 	err = x->outer_mode->output(x, skb);
@@ -3545,6 +3555,16 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 		goto out2;
 	}
 #ifdef CONFIG_XFRM
+	/* xfrm tunnel mode needs additional dst to extract outter
+	 * ip header protocol/ttl/id field, here creat a phony one.
+	 * instead of looking for a valid rt, which definitely hurting
+	 * performance under such circumstance.
+	 */
+	pkt_dev->dstops.family = AF_INET;
+	pkt_dev->dst.dev = pkt_dev->odev;
+	dst_init_metrics(&pkt_dev->dst, pktgen_dst_metrics, false);
+	pkt_dev->dst.child = &pkt_dev->dst;
+	pkt_dev->dst.ops = &pkt_dev->dstops;
 #endif
 
 	return add_dev_to_thread(t, pkt_dev);
-- 
1.7.9.5

[PATCHv2 net-next 4/5] {pktgen, xfrm} Introduce xfrm_state_lookup_byspi for pktgen

[Fan Du]

Introduce xfrm_state_lookup_byspi to find user specified by custom
from "pgset spi xxx". Using this scheme, any flow regardless its
saddr/daddr could be transform by SA specified with configurable
spi.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/net/xfrm.h    |    7 ++-----
 net/core/pktgen.c     |   13 +++++--------
 net/xfrm/xfrm_state.c |   43 +++++++++++++++++--------------------------
 3 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 6b82fdf..68ad3f8 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1417,11 +1417,8 @@ struct xfrm_state *xfrm_state_find(const xfrm_address_t *daddr,
 				   struct xfrm_tmpl *tmpl,
 				   struct xfrm_policy *pol, int *err,
 				   unsigned short family);
-struct xfrm_state *xfrm_stateonly_find(struct net *net, u32 mark,
-				       xfrm_address_t *daddr,
-				       xfrm_address_t *saddr,
-				       unsigned short family,
-				       u8 mode, u8 proto, u32 reqid);
+struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
+					      unsigned short family);
 int xfrm_state_check_expire(struct xfrm_state *x);
 void xfrm_state_insert(struct xfrm_state *x);
 int xfrm_state_add(struct xfrm_state *x);
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 2f4f90c..f2a2a98 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -2238,19 +2238,16 @@ static inline int f_pick(struct pktgen_dev *pkt_dev)
 /* If there was already an IPSEC SA, we keep it as is, else
  * we go look for it ...
 */
-#define DUMMY_MARK 0
 static void get_ipsec_sa(struct pktgen_dev *pkt_dev, int flow)
 {
 	struct xfrm_state *x = pkt_dev->flows[flow].x;
 	struct pktgen_net *pn = net_generic(dev_net(pkt_dev->odev), pg_net_id);
 	if (!x) {
-		/*slow path: we dont already have xfrm_state*/
-		x = xfrm_stateonly_find(pn->net, DUMMY_MARK,
-					(xfrm_address_t *)&pkt_dev->cur_daddr,
-					(xfrm_address_t *)&pkt_dev->cur_saddr,
-					AF_INET,
-					pkt_dev->ipsmode,
-					pkt_dev->ipsproto, 0);
+		/* We need as quick as possible to find the right SA
+		 * Searching with minimum criteria to archieve this.
+		 */
+		x = xfrm_state_lookup_byspi(pn->net, htonl(pkt_dev->spi),
+						AF_INET);
 		if (x) {
 			pkt_dev->flows[flow].x = x;
 			set_pkt_overhead(pkt_dev);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 68c2f35..6a9c402 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -892,38 +892,29 @@ out:
 	return x;
 }
 
-struct xfrm_state *
-xfrm_stateonly_find(struct net *net, u32 mark,
-		    xfrm_address_t *daddr, xfrm_address_t *saddr,
-		    unsigned short family, u8 mode, u8 proto, u32 reqid)
+struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
+					      unsigned short family)
 {
-	unsigned int h;
-	struct xfrm_state *rx = NULL, *x = NULL;
+	struct xfrm_state *x;
+	struct xfrm_state_walk *w;
 
-	spin_lock(&xfrm_state_lock);
-	h = xfrm_dst_hash(net, daddr, saddr, reqid, family);
-	hlist_for_each_entry(x, net->xfrm.state_bydst+h, bydst) {
-		if (x->props.family == family &&
-		    x->props.reqid == reqid &&
-		    (mark & x->mark.m) == x->mark.v &&
-		    !(x->props.flags & XFRM_STATE_WILDRECV) &&
-		    xfrm_state_addr_check(x, daddr, saddr, family) &&
-		    mode == x->props.mode &&
-		    proto == x->id.proto &&
-		    x->km.state == XFRM_STATE_VALID) {
-			rx = x;
-			break;
-		}
-	}
+	spin_lock_bh(&xfrm_state_lock);
+	list_for_each_entry(w, &net->xfrm.state_all, all) {
 
-	if (rx)
-		xfrm_state_hold(rx);
-	spin_unlock(&xfrm_state_lock);
+		x = container_of(w, struct xfrm_state, km);
+		if (x->props.family != family ||
+			x->id.spi != spi)
+			continue;
 
+		spin_unlock_bh(&xfrm_state_lock);
+		xfrm_state_hold(x);
+		return x;
+	}
+	spin_unlock_bh(&xfrm_state_lock);
 
-	return rx;
+	return NULL;
 }
-EXPORT_SYMBOL(xfrm_stateonly_find);
+EXPORT_SYMBOL(xfrm_state_lookup_byspi);
 
 static void __xfrm_state_insert(struct xfrm_state *x)
 {
-- 
1.7.9.5

[PATCHv2 net-next 5/5] {pktgen, xfrm} Correct xfrm state lock usage when transforming

[Fan Du]

xfrm_state lock protects its state, i.e., VALID/DEAD and statistics,
not the transforming procedure, as both mode/type output functions
are reentrant.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/core/pktgen.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f2a2a98..10237a3 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -2499,8 +2499,6 @@ static int pktgen_output_ipsec(struct sk_buff *skb, struct pktgen_dev *pkt_dev)
 	if (x->props.mode == XFRM_MODE_TUNNEL)
 		__skb_dst_set_noref(skb, &pkt_dev->dst, true);
 
-	spin_lock(&x->lock);
-
 	err = x->outer_mode->output(x, skb);
 	if (err)
 		goto error;
@@ -2508,10 +2506,11 @@ static int pktgen_output_ipsec(struct sk_buff *skb, struct pktgen_dev *pkt_dev)
 	if (err)
 		goto error;
 
+	spin_lock(&x->lock);
 	x->curlft.bytes += skb->len;
 	x->curlft.packets++;
-error:
 	spin_unlock(&x->lock);
+error:
 	return err;
 }
 
-- 
1.7.9.5

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Fri, 6 Dec 2013 17:53:29 +0800

> Hi, Dave/Steffen
> 
> Current pktgen IPsec supports only transport/ESP combinnation,
> and it's buggy before I fixed transformed IP header/length issue
> a few days ago.
> 
> This patchset enables user to do almost any IPsec transformation,
> both transport/tunnel mode, and AH/ESP/IPcomp type.
> 
> Below configuration has been tested, and using Wireshark could decrypt
> out plain text in good formation without any checksum/auth errors:
> 
> Mode/TYPE	AH	ESP
> Transport	x	x
> Tunnel		x	x
> 
> ChangeLog
> v2:
>   Rebase patchset against newest net-next.
>   Patch1: Remove adding rebundant empty line spotted by Sergei.
>   Patch2: Use only one dst pointing into itself to save space.

I don't like how you've arranged this.

The existing mechanism might be used by someone, and in patch #1
you just remove it.

It's fixed, but so what?  It might useful for someone.

You must make your changes such that existing setups still
work, you can't just break things on people.

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Fan Du]

On 2013年12月10日 08:58, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Fri, 6 Dec 2013 17:53:29 +0800
>
>> Hi, Dave/Steffen
>>
>> Current pktgen IPsec supports only transport/ESP combinnation,
>> and it's buggy before I fixed transformed IP header/length issue
>> a few days ago.
>>
>> This patchset enables user to do almost any IPsec transformation,
>> both transport/tunnel mode, and AH/ESP/IPcomp type.
>>
>> Below configuration has been tested, and using Wireshark could decrypt
>> out plain text in good formation without any checksum/auth errors:
>>
>> Mode/TYPE	AH	ESP
>> Transport	x	x
>> Tunnel		x	x
>>
>> ChangeLog
>> v2:
>>    Rebase patchset against newest net-next.
>>    Patch1: Remove adding rebundant empty line spotted by Sergei.
>>    Patch2: Use only one dst pointing into itself to save space.
>
> I don't like how you've arranged this.
>
> The existing mechanism might be used by someone, and in patch #1
> you just remove it.
>
> It's fixed, but so what?  It might useful for someone.


How it could be usable first and then useful for someone before my fix
9e921193884dd85b4cd68aa18598d8c2f9ad85b9
({pktgen, xfrm} Correct xfrm state lock usage when transforming)???

Do you really know what this fix does?

> You must make your changes such that existing setups still
> work, you can't just break things on people.
>

The broken things is the original patch, not this patchset,
any people try to use pktgen with IPsec enabled could spot it's not working.
and any up comping make-working patch won't break anything!


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Tue, 10 Dec 2013 09:19:09 +0800

> How it could be usable first and then useful for someone before my fix
> 9e921193884dd85b4cd68aa18598d8c2f9ad85b9
> ({pktgen, xfrm} Correct xfrm state lock usage when transforming)???
> 
> Do you really know what this fix does?

Maybe they didn't care about the checksum being correct in the
testbed they were using.

I know Jamal used it when he added the feature.

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Fan Du]

On 2013年12月10日 09:23, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Tue, 10 Dec 2013 09:19:09 +0800
>
>> How it could be usable first and then useful for someone before my fix
>> 9e921193884dd85b4cd68aa18598d8c2f9ad85b9
>> ({pktgen, xfrm} Correct xfrm state lock usage when transforming)???

slip of pen here, should be commit:
3868204d6b89ea373a273e760609cb08020beb1a
("{pktgen, xfrm} Update IPv4 header total len and checksum after transformation")

>> Do you really know what this fix does?
>
> Maybe they didn't care about the checksum being correct in the
> testbed they were using.

Can I just reply to custom "Yes, wrong checksum value and non-decrypted packet
is not big deal when your peer host receiving them"?

> I know Jamal used it when he added the feature.

Please re-consider your approach for this patch set, really.

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Tue, 10 Dec 2013 10:02:36 +0800

> Can I just reply to custom "Yes, wrong checksum value and
> non-decrypted packet
> is not big deal when your peer host receiving them"?

It's a packet spamming tool, take that into consideration.

Regardless of what you may think, it is simply the case that you can't
just remove something because you perceive it as not useful to anyone.

This goes doubly for something exposed to users for a decade.

Please change your patch series to retain the feature.

Thanks.

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Fan Du]

On 2013年12月10日 10:09, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Tue, 10 Dec 2013 10:02:36 +0800
>
>> Can I just reply to custom "Yes, wrong checksum value and
>> non-decrypted packet
>> is not big deal when your peer host receiving them"?
>
> It's a packet spamming tool, take that into consideration.
> Regardless of what you may think, it is simply the case that you can't
> just remove something because you perceive it as not useful to anyone.
>
> This goes doubly for something exposed to users for a decade.
>
> Please change your patch series to retain the feature.

I will restore "wrong checksum/len packet" behavior as a default behavior,
As a matter of fact this has nothing to do with Patch1/5 which removes codes.
And then making enhanced feature as optional for user who expects good format packet

Is this fine for you, finally?


> Thanks.
>
>

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Tue, 10 Dec 2013 10:27:55 +0800

> 
> 
> On 2013年12月10日 10:09, David Miller wrote:
>> From: Fan Du<fan.du@windriver.com>
>> Date: Tue, 10 Dec 2013 10:02:36 +0800
>>
>>> Can I just reply to custom "Yes, wrong checksum value and
>>> non-decrypted packet
>>> is not big deal when your peer host receiving them"?
>>
>> It's a packet spamming tool, take that into consideration.
>> Regardless of what you may think, it is simply the case that you can't
>> just remove something because you perceive it as not useful to anyone.
>>
>> This goes doubly for something exposed to users for a decade.
>>
>> Please change your patch series to retain the feature.
> 
> I will restore "wrong checksum/len packet" behavior as a default
> behavior,
> As a matter of fact this has nothing to do with Patch1/5 which removes
> codes.
> And then making enhanced feature as optional for user who expects good
> format packet
> 
> Is this fine for you, finally?

Sure.

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Jamal Hadi Salim]

On 12/09/13 20:23, David Miller wrote:

> Maybe they didn't care about the checksum being correct in the
> testbed they were using.

Thats correct. The point is to exercise the crypto code.
Please dont change this.

cheers,
jamal

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Fan Du]

FWIW, I will follow Dave's suggestion not to touch your original
support at the first place no matter how below answer looks like.

On 2013年12月11日 21:42, Jamal Hadi Salim wrote:
> On 12/09/13 20:23, David Miller wrote:
>
>> Maybe they didn't care about the checksum being correct in the
>> testbed they were using.
>
> Thats correct. The point is to exercise the crypto code.
> Please dont change this.

However exercising crypto engine on host running pktgen does *NOT* necessary
requiring bad IP checksum value and wrong total len value, because encryption
has been done.

I'm confused here with your test rationale, and what's wrong to use
the right checksum/total length value? Please clear my puzzles.


Thanks
-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Jamal Hadi Salim]

On 12/13/13 03:56, Fan Du wrote:
> FWIW, I will follow Dave's suggestion not to touch your original
> support at the first place no matter how below answer looks like.
>

I think you missed the message. No one said dont add that feature,
just dont make it default or remove the existing one.

cheers,
jamal

Re: [PATCHv2 net-next 0/5] pktgen IPsec support

[Fan Du]

On 2013年12月13日 20:13, Jamal Hadi Salim wrote:
> On 12/13/13 03:56, Fan Du wrote:
>> FWIW, I will follow Dave's suggestion not to touch your original
>> support at the first place no matter how below answer looks like.
>>
>
> I think you missed the message. No one said dont add that feature,
> just dont make it default or remove the existing one.

I got it, v2 will carry out this discussion point.

Thanks
-- 
浮沉随浪只记今朝笑

--fan