Skip to contentSkip to conversation

possible bug in handling of ipv4 route caching

RFC PATCH In review Chris Friesen 3 participants 11 messages Apr 7, 2016 – Apr 14, 2016

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Chris Friesen

author
#0 lore raw

RFC PATCH 1/1 possible bug in handling of ipv4 route caching

Hi,

We think we may have found a bug in the handling of ipv4 route caching, and are curious what you think.

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

For what it's worth, here's a patch that we've applied locally to deal with the issue.

Chris

Signed-off-by Allain Legacy <[email protected]>
Signed-off-by Chris Friesen <[email protected]>
net/ipv4/route.c +11 -0
index 02c6229..e965d4b 100644
@@ -2045,6 +2045,17 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0)) {
+		/* For local routes that require a particular output interface
+                 * we do not want to cache the result.  Caching the result
+                 * causes incorrect behaviour when there are multiple source
+                 * addresses on the interface, the end result being that if the
+                 * intended recipient is waiting on that interface for the
+                 * packet he won't receive it because it will be delivered on
+                 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+                 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;
#1 lore raw

Hello,

Quoted 5 lines · Chris Friesen

Hi,

We think we may have found a bug in the handling of ipv4 route caching, and are curious what you think.

Quoted 7 lines

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

So, if we store some orig_oif in rt_iif that
is specific (cached in nexthop) to some device, this orig_oif should match the device. The only case when this is violated is when we redirect traffic to lo. Any attempts to store eth1 in rt_iif for nexthop cache for lo should be avoided.

Quoted 23 lines

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

For what it's worth, here's a patch that we've applied locally to deal with the issue.

Chris

Signed-off-by Allain Legacy <[email protected]>
Signed-off-by Chris Friesen <[email protected]>
net/ipv4/route.c +0 -0
index 02c6229..e965d4b 100644

net/ipv4/route.c static struct rtable *__mkroute_output(const struct fib_result *res, · line 2048

Quoted 4 lines
net/ipv4/route.c +1 -0
@@ -2045,6 +2045,17 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0)) {

	So, we can be more specific. Can this work?:

	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
		   (orig_oif != dev_out->ifindex)) {

	I.e. we should allow to cache orig_oif=LOOPBACK_IFINDEX

but eth1 should not be cached.

net/ipv4/route.c static struct rtable *__mkroute_output(const struct fib_result *res, · line 2061

Quoted 13 lines
net/ipv4/route.c +10 -0
+		/* For local routes that require a particular output interface
+                 * we do not want to cache the result.  Caching the result
+                 * causes incorrect behaviour when there are multiple source
+                 * addresses on the interface, the end result being that if the
+                 * intended recipient is waiting on that interface for the
+                 * packet he won't receive it because it will be delivered on
+                 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+                 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;

Regards

author
#2 lore raw
Quoted 31 lines · Julian Anastasov
Quoted 18 lines · Chris Friesen

Hi,

We think we may have found a bug in the handling of ipv4 route caching, and are curious what you think.

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

net/ipv4/route.c +0 -0
index 02c6229..e965d4b 100644

net/ipv4/route.c static struct rtable *__mkroute_output(const struct fib_result *res, · line 2048

Quoted 4 lines
net/ipv4/route.c +1 -0
@@ -2045,6 +2045,17 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
  		 */
  		if (fi && res->prefixlen < 4)
  			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0)) {

	So, we can be more specific. Can this work?:

	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
		   (orig_oif != dev_out->ifindex)) {

	I.e. we should allow to cache orig_oif=LOOPBACK_IFINDEX
Quoted 1 line

but eth1 should not be cached.

Yes, we think that will work. New patch to follow.

Chris

author
#3 lore raw

PATCH v2 1/1 route: do not cache fib route info on local routes with oif

In reply to Julian Anastasov

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Sample dhcp_release command line:

dhcp_release eth1 192.168.204.222 02:11:33:22:44:66

Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

---
 net/ipv4/route.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
net/ipv4/route.c +12 -0
index 02c6229..437a377 100644
@@ -2045,6 +2045,18 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
+		   (orig_oif != dev_out->ifindex)) {
+		/* For local routes that require a particular output interface
+		 * we do not want to cache the result.  Caching the result
+		 * causes incorrect behaviour when there are multiple source
+		 * addresses on the interface, the end result being that if the
+		 * intended recipient is waiting on that interface for the
+		 * packet he won't receive it because it will be delivered on
+		 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+		 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;
#4 lore raw

Re: [PATCH v2] route: do not cache fib route info on local routes with oif

Hello,

Quoted 31 lines · Chris Friesen

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are
multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive
it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Sample dhcp_release command line:

   dhcp_release eth1 192.168.204.222 02:11:33:22:44:66
Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

---
 net/ipv4/route.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
net/ipv4/route.c +0 -0
index 02c6229..437a377 100644

net/ipv4/route.c static struct rtable *__mkroute_output(const struct · line 2045

Quoted 1 line
net/ipv4/route.c +0 -0
@@ -2045,6 +2045,18 @@ static struct rtable *__mkroute_output(const struct
 fib_result *res,

Your patch is corrupted. I was in the same trap
some time ago but with different client:

Quoted 1 line

From Documentation/email-clients.txt:

Don't send patches with "format=flowed". This can cause unexpected and unwanted line breaks.

Anyways, the change looks good to me and I'll add my
Reviewed-by tag the next time.

Quoted 10 lines
net/ipv4/route.c +6 -0
 		*/
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
+		   (orig_oif != dev_out->ifindex)) {
+		/* For local routes that require a particular output interface
+		 * we do not want to cache the result.  Caching the result
+		 * causes incorrect behaviour when there are multiple source
+		 * addresses on the interface, the end result being that if

the

net/ipv4/route.c static struct rtable *__mkroute_output(const struct fib_result *res, · line 2062

Quoted 9 lines
net/ipv4/route.c +6 -0
+		 * intended recipient is waiting on that interface for the
+		 * packet he won't receive it because it will be delivered on
+		 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+		 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;

Regards

author
#5 lore raw

Re: [PATCH v2] route: do not cache fib route info on local routes with oif

Quoted 10 lines · Julian Anastasov

Your patch is corrupted. I was in the same trap
some time ago but with different client:

From Documentation/email-clients.txt:

Don't send patches with "format=flowed". This can cause unexpected and unwanted line breaks.

Anyways, the change looks good to me and I'll add my
Reviewed-by tag the next time.

Doh...forgot to turn off word wrapping. New patch coming.

Chris

author
#6 lore raw

PATCH v3 1/1 route: do not cache fib route info on local routes with oif

In reply to Julian Anastasov

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Sample dhcp_release command line:

dhcp_release eth1 192.168.204.222 02:11:33:22:44:66

Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

---
 net/ipv4/route.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
net/ipv4/route.c +12 -0
index 02c6229..437a377 100644
@@ -2045,6 +2045,18 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
+		   (orig_oif != dev_out->ifindex)) {
+		/* For local routes that require a particular output interface
+                 * we do not want to cache the result.  Caching the result
+                 * causes incorrect behaviour when there are multiple source
+                 * addresses on the interface, the end result being that if the
+                 * intended recipient is waiting on that interface for the
+                 * packet he won't receive it because it will be delivered on
+                 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+                 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;
#7 lore raw

Re: [PATCH v3] route: do not cache fib route info on local routes with oif

Hello,

Quoted 14 lines · Chris Friesen

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Quoted 6 lines

Sample dhcp_release command line:

   dhcp_release eth1 192.168.204.222 02:11:33:22:44:66
Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

Sorry, forgot to mention that patch has other errors:

scripts/checkpatch.pl --strict /tmp/file3.patch

Its recommendations:

- prefer a maximum 75 chars per line
- code indent should use tabs where possible

Quoted 17 lines
---
 net/ipv4/route.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
net/ipv4/route.c +5 -0
index 02c6229..437a377 100644
@@ -2045,6 +2045,18 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
+		   (orig_oif != dev_out->ifindex)) {
+		/* For local routes that require a particular output interface
+                 * we do not want to cache the result.  Caching the result
+                 * causes incorrect behaviour when there are multiple source

net/ipv4/route.c static struct rtable *__mkroute_output(const struct fib_result *res, · line 2062

Quoted 10 lines
net/ipv4/route.c +7 -0
+                 * addresses on the interface, the end result being that if the
+                 * intended recipient is waiting on that interface for the
+                 * packet he won't receive it because it will be delivered on
+                 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+                 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;

Regards

author
#8 lore raw

PATCH v4 1/1 route: do not cache fib route info on local routes with oif

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Sample dhcp_release command line:

dhcp_release eth1 192.168.204.222 02:11:33:22:44:66

Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

---
 net/ipv4/route.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
net/ipv4/route.c +12 -0
index 02c6229..b050cf9 100644
@@ -2045,6 +2045,18 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
+		   (orig_oif != dev_out->ifindex)) {
+		/* For local routes that require a particular output interface
+		 * we do not want to cache the result.  Caching the result
+		 * causes incorrect behaviour when there are multiple source
+		 * addresses on the interface, the end result being that if the
+		 * intended recipient is waiting on that interface for the
+		 * packet he won't receive it because it will be delivered on
+		 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+		 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;
Reviewed-by on v4 1/1
#9 lore raw

Re: [PATCH v4] route: do not cache fib route info on local routes with oif

Hello,

Quoted 16 lines · Chris Friesen

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Quoted 6 lines

Sample dhcp_release command line:

   dhcp_release eth1 192.168.204.222 02:11:33:22:44:66
Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

Looks good to me.

Reviewed-by Julian Anastasov <[email protected]>
Quoted 17 lines
---
 net/ipv4/route.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
net/ipv4/route.c +5 -0
index 02c6229..b050cf9 100644
@@ -2045,6 +2045,18 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
 		 */
 		if (fi && res->prefixlen < 4)
 			fi = NULL;
+	} else if ((type == RTN_LOCAL) && (orig_oif != 0) &&
+		   (orig_oif != dev_out->ifindex)) {
+		/* For local routes that require a particular output interface
+		 * we do not want to cache the result.  Caching the result
+		 * causes incorrect behaviour when there are multiple source

net/ipv4/route.c static struct rtable *__mkroute_output(const struct fib_result *res, · line 2062

Quoted 10 lines
net/ipv4/route.c +7 -0
+		 * addresses on the interface, the end result being that if the
+		 * intended recipient is waiting on that interface for the
+		 * packet he won't receive it because it will be delivered on
+		 * the loopback interface and the IP_PKTINFO ipi_ifindex will
+		 * be set to the loopback interface as well.
+		 */
+		fi = NULL;
 	}
 
 	fnhe = NULL;

Regards

maintainer
#10 lore raw

Re: [PATCH v4] route: do not cache fib route info on local routes with oif

In reply to Chris Friesen

From: Chris Friesen <[email protected]>
Date: Fri, 8 Apr 2016 15:21:30 -0600

Quoted 16 lines

For local routes that require a particular output interface we do not want to cache the result. Caching the result causes incorrect behaviour when there are multiple source addresses on the interface. The end result being that if the intended recipient is waiting on that interface for the packet he won't receive it because it will be delivered on the loopback interface and the IP_PKTINFO ipi_ifindex will be set to the loopback interface as well.

This can be tested by running a program such as "dhcp_release" which attempts to inject a packet on a particular interface so that it is received by another program on the same board. The receiving process should see an IP_PKTINFO ipi_ifndex value of the source interface (e.g., eth1) instead of the loopback interface (e.g., lo). The packet will still appear on the loopback interface in tcpdump but the important aspect is that the CMSG info is correct.

Quoted 6 lines

Sample dhcp_release command line:

   dhcp_release eth1 192.168.204.222 02:11:33:22:44:66
Signed-off-by Allain Legacy <[email protected]>

Signed off-by: Chris Friesen <[email protected]>

Applied and queued up for -stable, thanks.