Skip to contentSkip to conversation

net: gre: don't pull skb if dealing with icmp message

PATCH In review Duan Jiong 3 participants 5 messages Jan 19, 2014 – Jan 22, 2014

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Duan Jiong

author
#0 lore raw

PATCH 1/1 net: gre: don't pull skb if dealing with icmp message

When dealing with icmp messages, the skb->data points the ip header that triggered the sending of the icmp message.

In gre_cisco_err(), the parse_gre_header() is called, and the iptunnel_pull_header() is called to pull the skb at the end of the parse_gre_header(). Unfortunately, the ipgre_err still needs the skb->data points the ip header following the icmp layer, and those ip addresses in ip header will be used to look up tunnel by ip_tunnel_lookup().

Signed-off-by Duan Jiong <[email protected]>
---
 net/ipv4/gre_demux.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
net/ipv4/gre_demux.c +6 -3
index 5893e99..35bfba4 100644
@@ -116,7 +116,7 @@ static __sum16 check_checksum(struct sk_buff *skb)
 }
 
 static int parse_gre_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
-			    bool *csum_err)
+			    bool *csum_err, bool in_err)
 {
 	unsigned int ip_hlen = ip_hdrlen(skb);
 	const struct gre_base_hdr *greh;

@@ -160,6 +160,9 @@ static int parse_gre_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 	} else
 		tpi->seq = 0;
 
+	if (in_err)
+		return 0;
+
 	/* WCCP version 1 and 2 protocol decoding.
 	 * - Change protocol to IP
 	 * - When dealing with WCCPv2, Skip extra 4 bytes in GRE header

@@ -182,7 +185,7 @@ static int gre_cisco_rcv(struct sk_buff *skb)
 	int i;
 	bool csum_err = false;
 
-	if (parse_gre_header(skb, &tpi, &csum_err) < 0)
+	if (parse_gre_header(skb, &tpi, &csum_err, false) < 0)
 		goto drop;
 
 	rcu_read_lock();

@@ -229,7 +232,7 @@ static void gre_cisco_err(struct sk_buff *skb, u32 info)
 	bool csum_err = false;
 	int i;
 
-	if (parse_gre_header(skb, &tpi, &csum_err)) {
+	if (parse_gre_header(skb, &tpi, &csum_err, true)) {
 		if (!csum_err)		/* ignore csum errors. */
 			return;
 	}
Signature · 1 line
1.8.3.1
#1 lore raw
Quoted 11 lines · Duan Jiong

When dealing with icmp messages, the skb->data points the ip header that triggered the sending of the icmp message.

In gre_cisco_err(), the parse_gre_header() is called, and the iptunnel_pull_header() is called to pull the skb at the end of the parse_gre_header(). Unfortunately, the ipgre_err still needs the skb->data points the ip header following the icmp layer, and those ip addresses in ip header will be used to look up tunnel by ip_tunnel_lookup().

This looks like bug.
Can you use ip_hdr() rather than skb->data in ipgre_err(). Same is done in ipgre_rcv().

author
#2 lore raw
Quoted 12 lines · Pravin Shelar
Quoted 11 lines · Duan Jiong

When dealing with icmp messages, the skb->data points the ip header that triggered the sending of the icmp message.

In gre_cisco_err(), the parse_gre_header() is called, and the iptunnel_pull_header() is called to pull the skb at the end of the parse_gre_header(). Unfortunately, the ipgre_err still needs the skb->data points the ip header following the icmp layer, and those ip addresses in ip header will be used to look up tunnel by ip_tunnel_lookup().

Quoted 3 lines

This looks like bug.
Can you use ip_hdr() rather than skb->data in ipgre_err(). Same is done in ipgre_rcv().

That's maybe not a good idea. The ip_hdr() will the return the outmost ip header, but we need inner ip header following the icmp layer.

Thanks,
Duan

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 13 lines · Duan Jiong
Quoted 12 lines · Pravin Shelar
Quoted 11 lines · Duan Jiong

When dealing with icmp messages, the skb->data points the ip header that triggered the sending of the icmp message.

In gre_cisco_err(), the parse_gre_header() is called, and the iptunnel_pull_header() is called to pull the skb at the end of the parse_gre_header(). Unfortunately, the ipgre_err still needs the skb->data points the ip header following the icmp layer, and those ip addresses in ip header will be used to look up tunnel by ip_tunnel_lookup().

Quoted 7 lines
Quoted 3 lines

This looks like bug.
Can you use ip_hdr() rather than skb->data in ipgre_err(). Same is done in ipgre_rcv().

That's maybe not a good idea. The ip_hdr() will the return the outmost ip header, but we need inner ip header following the icmp layer.

Why do you want to use inner ip header?
If you want to really look at original packet header, you can calculate it from outer iph header rather than changing fast path parse_gre_header() function.

maintainer
#4 lore raw
In reply to Duan Jiong

From: Duan Jiong <[email protected]>
Date: Sun, 19 Jan 2014 16:35:22 +0800

Quoted 4 lines

When dealing with icmp messages, the skb->data points the ip header that triggered the sending of the icmp message.

Quoted 8 lines

In gre_cisco_err(), the parse_gre_header() is called, and the iptunnel_pull_header() is called to pull the skb at the end of the parse_gre_header(). Unfortunately, the ipgre_err still needs the skb->data points the ip header following the icmp layer, and those ip addresses in ip header will be used to look up tunnel by ip_tunnel_lookup().

Signed-off-by Duan Jiong <[email protected]>

Conditional pulling of headers based upon caller context leads to impossible to maintain code.

Please find another way to fix this.