SAD info TLV aggregation

PATCH In review jamal 3 participants 6 messages May 2, 2007 – May 3, 2007

Subsystems (3)

Networking Network drivers Networking IPsec

lore

Share & tools

lore

jamal

author

2007-05-02 22:18 UTC #0 lore raw

PATCH 1/1 SAD info TLV aggregation

Dave,
against net-2.6.22.

I am keeping the thin 32 bit header, but other than that everything is in one TLV. SPD info one to follow..

cheers,
jamal

[XFRM] SAD info TLV aggregationx

Aggregate the SAD info TLVs.

Signed-off-by Jamal Hadi Salim <[email protected]>

---
commit 48d8642f8e49b2c4e249858a65bf760ac03a4be7
tree 0041377be49d157d18de8b5abff13f93222f14cc
parent f0b0a76736fa63b21b1658ed514ecd67d39e485e
author Jamal Hadi Salim <[email protected]> Wed, 02 May 2007 14:43:05 -0400 committer Jamal Hadi Salim <[email protected]> Wed, 02 May 2007 14:43:05 -0400

 include/linux/xfrm.h  |   21 +++++++--------------
 include/net/xfrm.h    |   10 +---------
 net/xfrm/xfrm_state.c |    2 +-
 net/xfrm/xfrm_user.c  |   16 +++-------------
 4 files changed, 12 insertions(+), 37 deletions(-)

include/linux/xfrm.h +7 ~~-14~~

index a5d53e0..31e2a2a 100644
@@ -243,17 +243,6 @@ enum xfrm_ae_ftype_t {
 #define XFRM_AE_MAX (__XFRM_AE_MAX - 1)
 };
 
-/* SAD Table filter flags  */
-enum xfrm_sad_ftype_t {
-	XFRM_SAD_UNSPEC,
-	XFRM_SAD_HMASK=1,
-	XFRM_SAD_HMAX=2,
-	XFRM_SAD_CNT=4,
-	__XFRM_SAD_MAX
-
-#define XFRM_SAD_MAX (__XFRM_SAD_MAX - 1)
-};
-
 struct xfrm_userpolicy_type {
 	__u8		type;
 	__u16		reserved1;

@@ -287,14 +276,18 @@ enum xfrm_attr_type_t {
 
 enum xfrm_sadattr_type_t {
 	XFRMA_SAD_UNSPEC,
-	XFRMA_SADHMASK,
-	XFRMA_SADHMAX,
-	XFRMA_SADCNT,
+	XFRMA_SADINFO,
 	__XFRMA_SAD_MAX
 
 #define XFRMA_SAD_MAX (__XFRMA_SAD_MAX - 1)
 };
 
+struct xfrmu_sadinfo {
+	__u32 sadhcnt; /* current hash bkts */
+	__u32 sadhmcnt; /* max allowed hash bkts */
+	__u32 sadcnt; /* current running count */
+};
+
 /* SPD Table filter flags  */
 enum xfrm_spd_ftype_t {
 	XFRM_SPD_UNSPEC,

include/net/xfrm.h +1 -9

index 66c2d3e..d3551b2 100644
@@ -416,14 +416,6 @@ struct xfrm_audit
 	u32	secid;
 };
 
-/* SAD metadata, add more later */
-struct xfrm_sadinfo
-{
-	u32 sadhcnt; /* current hash bkts */
-	u32 sadhmcnt; /* max allowed hash bkts */
-	u32 sadcnt; /* current running count */
-};
-
 struct xfrm_spdinfo
 {
 	u32 incnt;

@@ -967,7 +959,7 @@ static inline int xfrm_state_sort(struct xfrm_state **dst, struct xfrm_state **s
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
 extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
-extern void xfrm_sad_getinfo(struct xfrm_sadinfo *si);
+extern void xfrm_sad_getinfo(struct xfrmu_sadinfo *si);
 extern void xfrm_spd_getinfo(struct xfrm_spdinfo *si);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, __be32 seq);

net/xfrm/xfrm_state.c +1 -1

index f3a61eb..e5e7b0f 100644
@@ -421,7 +421,7 @@ restart:
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
-void xfrm_sad_getinfo(struct xfrm_sadinfo *si)
+void xfrm_sad_getinfo(struct xfrmu_sadinfo *si)
 {
 	spin_lock_bh(&xfrm_state_lock);
 	si->sadcnt = xfrm_state_num;

net/xfrm/xfrm_user.c +3 ~~-13~~

index 4210d91..0d98955 100644
@@ -749,7 +749,7 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 static int build_sadinfo(struct sk_buff *skb, u32 pid, u32 seq, u32 flags)
 {
-	struct xfrm_sadinfo si;
+	struct xfrmu_sadinfo si;
 	struct nlmsghdr *nlh;
 	u32 *f;

@@ -761,12 +761,7 @@ static int build_sadinfo(struct sk_buff *skb, u32 pid, u32 seq, u32 flags)
 	*f = flags;
 	xfrm_sad_getinfo(&si);
 
-	if (flags & XFRM_SAD_HMASK)
-		NLA_PUT_U32(skb, XFRMA_SADHMASK, si.sadhcnt);
-	if (flags & XFRM_SAD_HMAX)
-		NLA_PUT_U32(skb, XFRMA_SADHMAX, si.sadhmcnt);
-	if (flags & XFRM_SAD_CNT)
-		NLA_PUT_U32(skb, XFRMA_SADCNT, si.sadcnt);
+	NLA_PUT(skb, XFRMA_SADINFO, sizeof(si), &si);
 
 	return nlmsg_end(skb, nlh);

@@ -784,12 +779,7 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	u32 seq = nlh->nlmsg_seq;
 	int len = NLMSG_LENGTH(sizeof(u32));
 
-	if (*flags & XFRM_SAD_HMASK)
-		len += RTA_SPACE(sizeof(u32));
-	if (*flags & XFRM_SAD_HMAX)
-		len += RTA_SPACE(sizeof(u32));
-	if (*flags & XFRM_SAD_CNT)
-		len += RTA_SPACE(sizeof(u32));
+	len += RTA_SPACE(sizeof(struct xfrmu_sadinfo));
 
 	r_skb = alloc_skb(len, GFP_ATOMIC);

Patrick McHardy

2007-05-03 00:15 UTC #1 lore raw

Quoted 2 lines · jamal

I am keeping the thin 32 bit header, but other than that everything is in one TLV. SPD info one to follow..

I think using attributes here wasn't a bad idea since this exports things that are implementation details and might need to be changed or extended at some point.

include/linux/xfrm.h enum xfrm_attr_type_t { · line 283

Quoted 10 lines

enum xfrm_sadattr_type_t {

include/linux/xfrm.h +1 -3

 	XFRMA_SAD_UNSPEC,
-	XFRMA_SADHMASK,
-	XFRMA_SADHMAX,
-	XFRMA_SADCNT,
+	XFRMA_SADINFO,
 	__XFRMA_SAD_MAX
 
 #define XFRMA_SAD_MAX (__XFRMA_SAD_MAX - 1)
 };

In any case consistent naming here would be nice (SAD_ vs. SADXXX).

jamal

author

2007-05-03 13:02 UTC #2 lore raw

Quoted 2 lines · Patrick McHardy

I think using attributes here

"here" as in SAD or SPD as well?

Quoted 3 lines

wasn't a bad idea since this exports
things that are implementation details and might need to be changed or extended at some point.

I think it may be reasonable to group the Hash info in one TLV and the IPSEC specific info in a separate TLV - is this what you are saying? Be explicit so i dont have to redo the patch over and over. The other way to look at it is in the future, a new TLV could be added that replaces the current one.

Quoted 1 line

In any case consistent naming here would be nice (SAD_ vs. SADXXX).

yes, i need to make this change on the SPD as well. Dave hold onto both patches - i will resend.

cheers,
jamal

Patrick McHardy

2007-05-03 13:17 UTC #3 lore raw

Quoted 7 lines · jamal

Quoted 1 line · Patrick McHardy
I think using attributes here
"here" as in SAD or SPD as well?

Both.

Quoted 8 lines

Quoted 3 lines
wasn't a bad idea since this exports
things that are implementation details and might need to be changed or extended at some point.
I think it may be reasonable to group the Hash info in one TLV and the IPSEC specific info in a separate TLV - is this what you are saying? Be explicit so i dont have to redo the patch over and over.

Yes, thats what I imagined too.

Quoted 2 lines

The other way to look at it is in the future, a new TLV could be added that replaces the current one.

It would still make sense to seperate hash related things from the SA count in my opinion.

Quoted 5 lines

Quoted 1 line
In any case consistent naming here would be nice (SAD_ vs. SADXXX).
yes, i need to make this change on the SPD as well. Dave hold onto both patches - i will resend.

Thanks.

jamal

author

2007-05-03 13:26 UTC #4 lore raw

Quoted 6 lines · Patrick McHardy

Quoted 4 lines
I think it may be reasonable to group the Hash info in one TLV and the IPSEC specific info in a separate TLV - is this what you are saying? Be explicit so i dont have to redo the patch over and over.
Yes, thats what I imagined too.

Ok, thanks Patrick - indeed it is a wise thing to do to improve longevity of user space.

I will make those two changes sometime tonight or tommorow morn and resend.

cheers,
jamal

David Miller

maintainer

2007-05-03 20:15 UTC #5 lore raw

In reply to jamal

From: jamal <[email protected]>
Date: Thu, 03 May 2007 09:01:56 -0400

Quoted 4 lines

Quoted 1 line
In any case consistent naming here would be nice (SAD_ vs. SADXXX).
yes, i need to make this change on the SPD as well. Dave hold onto both patches - i will resend.

Okie dokie.