Skip to contentSkip to conversation

STATE: Add a hook to find where to be inserted header in outbound.

PATCH In review Masahide NAKAMURA 3 participants 6 messages Jul 29, 2006 – Aug 2, 2006

Subsystems (3)
Networking Network drivers Networking IPsec
lore
Share & tools
lore

Masahide NAKAMURA

author
#0 lore raw

PATCH 7/23 STATE: Add a hook to find where to be inserted header in outbound.

On current kernel, ip6_find_1stfragopt() is used by IPv6 IPsec to find where to be inserted header in outbound. (BTW, no usage may be needed for IPv4 case.) Mobile IPv6 requires other logic for routing header and destination options header respectively.
Based on MIPL2 kernel patch.

---
 include/net/xfrm.h      |    1 +
 net/ipv6/xfrm6_output.c |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)
include/net/xfrm.h +1 -0
index aaef1c4..bcda8c0 100644
@@ -270,6 +270,7 @@ struct xfrm_type
 	void			(*destructor)(struct xfrm_state *);
 	int			(*input)(struct xfrm_state *, struct sk_buff *skb);
 	int			(*output)(struct xfrm_state *, struct sk_buff *pskb);
+	int			(*place_find)(struct xfrm_state *, struct sk_buff *, u8 **);
 	/* Estimate maximal size of result of transformation of a dgram */
 	u32			(*get_max_size)(struct xfrm_state *, int size);
 };
net/ipv6/xfrm6_output.c +4 -1
index 2736a54..8792b5c 100644
@@ -48,7 +48,10 @@ static void xfrm6_encap(struct sk_buff *
 		u8 *prevhdr;
 		int hdr_len;
 
-		hdr_len = ip6_find_1stfragopt(skb, &prevhdr);
+		if (x->type->place_find)
+			hdr_len = x->type->place_find(x, skb, &prevhdr);
+		else
+			hdr_len = ip6_find_1stfragopt(skb, &prevhdr);
 		skb->nh.raw = prevhdr - x->props.header_len;
 		skb->h.raw = skb->data + hdr_len;
 		memmove(skb->data, iph, hdr_len);
Signature · 1 line
1.4.1
#1 lore raw

net/ipv6/xfrm6_output.c static void xfrm6_encap(struct sk_buff * · line 54

Quoted 5 lines · Masahide NAKAMURA
net/ipv6/xfrm6_output.c +4 -1
-		hdr_len = ip6_find_1stfragopt(skb, &prevhdr);
+		if (x->type->place_find)
+			hdr_len = x->type->place_find(x, skb, &prevhdr);
+		else
+			hdr_len = ip6_find_1stfragopt(skb, &prevhdr);

What about encapsulating this like:

static inline int hdr_offset(...)

{
	if (x->type->place_find)
		return x->type->place_find(x, skb, &prevhdr);
	else
		return ip6_find_1stfragopt(skb, &prevhdr);
}

Maybe also change the naming of ->place_find to ->hdr_offset

- James

Signature · 2 lines
James Morris
<[email protected]>
maintainer
#2 lore raw
In reply to Masahide NAKAMURA

From: Masahide NAKAMURA <[email protected]>
Date: Sat, 29 Jul 2006 18:30:23 +0900

include/net/xfrm.h struct xfrm_type · line 276

Quoted 8 lines
include/net/xfrm.h +1 -0
@@ -270,6 +270,7 @@ struct xfrm_type
 	void			(*destructor)(struct xfrm_state *);
 	int			(*input)(struct xfrm_state *, struct sk_buff *skb);
 	int			(*output)(struct xfrm_state *, struct sk_buff *pskb);
+	int			(*place_find)(struct xfrm_state *, struct sk_buff *, u8 **);
 	/* Estimate maximal size of result of transformation of a dgram */
 	u32			(*get_max_size)(struct xfrm_state *, int size);
 };

I see a dangerous pattern of adding many, many, many methods to the xfrm_type structure which are only used by ipv6. But I cannot suggest another method.

There are frequent calls of the form:

	if (x->type->op != NULL)
		x->type->op(x, y, z);
	else
		foo(y, z);

it might be nicer to hide all of this behind carefully crafted inline functions.

author
#3 lore raw
Quoted 15 lines · David Miller

From: Masahide NAKAMURA <[email protected]>
Date: Sat, 29 Jul 2006 18:30:23 +0900

include/net/xfrm.h struct xfrm_type · line 276

Quoted 8 lines
include/net/xfrm.h +1 -0
@@ -270,6 +270,7 @@ struct xfrm_type
 	void			(*destructor)(struct xfrm_state *);
 	int			(*input)(struct xfrm_state *, struct sk_buff *skb);
 	int			(*output)(struct xfrm_state *, struct sk_buff *pskb);
+	int			(*place_find)(struct xfrm_state *, struct sk_buff *, u8 **);
 	/* Estimate maximal size of result of transformation of a dgram */
 	u32			(*get_max_size)(struct xfrm_state *, int size);
 };

I see a dangerous pattern of adding many, many, many methods to the xfrm_type structure which are only used by ipv6. But I cannot suggest another method.

Sometimes this is a difficult point for me to design.

Quoted 9 lines

There are frequent calls of the form:

	if (x->type->op != NULL)
		x->type->op(x, y, z);
	else
		foo(y, z);

it might be nicer to hide all of this behind carefully crafted inline functions.

I'll fix it. James gave me this comment, too.

BTW he also gave me another point:

Quoted 1 line

Maybe also change the naming of ->place_find to ->hdr_offset

I'll use this idea, too.

Thank you.

Signature · 1 line
Masahide NAKAMURA
maintainer
#4 lore raw

From: Masahide NAKAMURA <[email protected]>
Date: Wed, 02 Aug 2006 11:20:30 +0900

Quoted 6 lines
Quoted 3 lines · David Miller

I see a dangerous pattern of adding many, many, many methods to the xfrm_type structure which are only used by ipv6. But I cannot suggest another method.

Sometimes this is a difficult point for me to design.

Do not worry so much about it right now, it is not a barrier for code integration. We can try to refine this later on.

author
#5 lore raw
Quoted 11 lines · David Miller

From: Masahide NAKAMURA <[email protected]>
Date: Wed, 02 Aug 2006 11:20:30 +0900

Quoted 5 lines
Quoted 3 lines · David Miller

I see a dangerous pattern of adding many, many, many methods to the xfrm_type structure which are only used by ipv6. But I cannot suggest another method.

Sometimes this is a difficult point for me to design.

Do not worry so much about it right now, it is not a barrier for code integration. We can try to refine this later on.

OK, I improve my code for current framework at first. Thanks :-)

Signature · 1 line
Masahide NAKAMURA