ipv4: Invalidate the socket cached route on pmtu events if possible

The route lookup in ipv4_sk_update_pmtu() might return a route different from the route we cached at the socket. This is because standart routes are per cpu, so each cpu has it's own struct rtable. This means that we do not invalidate the socket cached route if the NET_RX_SOFTIRQ is not served by the same cpu that the sending socket uses. As a result, the cached route reused until we disconnect.

With this patch we invalidate the socket cached route if possible. If the socket is owened by the user, we can't update the cached route directly. A followup patch will implement socket release callback functions for datagram sockets to handle this case.

Reported-by Yurij M. Plotnikov <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

This implements a socket release callback function to check if the socket cached route got invalid during the time we owned the socket. The function is used from udp, raw and ping sockets.

Signed-off-by Steffen Klassert <[email protected]>

The route lookup in ipv4_sk_update_pmtu() might return a route different from the route we cached at the socket. This is because standart routes are per cpu, so each cpu has it's own struct rtable. This means that we do not invalidate the socket cached route if the NET_RX_SOFTIRQ is not served by the same cpu that the sending socket uses. As a result, the cached route reused until we disconnect.

With this patch we invalidate the socket cached route if possible. If the socket is owened by the user, we can't update the cached route directly. A followup patch will implement socket release callback functions for datagram sockets to handle this case.

Reported-by Yurij M. Plotnikov <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

---
 net/ipv4/route.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

net/ipv4/route.c +1 -1

index 259cbee..132737a 100644
@@ -965,7 +965,7 @@ void ipv4_update_pmtu(struct sk_buff *skb, struct net *net, u32 mtu,
 }
 EXPORT_SYMBOL_GPL(ipv4_update_pmtu);
 
-void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu)
+static void __ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu)
 {
 	const struct iphdr *iph = (const struct iphdr *) skb->data;
 	struct flowi4 fl4;

@@ -978,6 +978,46 @@ void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu)
 		ip_rt_put(rt);
 	}
 }

net/ipv4/route.c +4 -0

+
+out:
+	bh_unlock_sock(sk);
+}
 EXPORT_SYMBOL_GPL(ipv4_sk_update_pmtu);
 
 void ipv4_redirect(struct sk_buff *skb, struct net *net,

Signature · 1 line

1.7.9.5

Hello,

On Mon, 21 Jan 2013, Steffen Klassert wrote:

net/ipv4/route.c void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu) · line 990

Quoted 8 lines

net/ipv4/route.c +8 -0

+{
+	const struct iphdr *iph = (const struct iphdr *) skb->data;
+	struct flowi4 fl4;
+	struct rtable *rt;
+	struct dst_entry *dst;
+
+	bh_lock_sock(sk);
+	rt = (struct rtable *) __sk_dst_get(sk);

I just saw another problem, sorry that
I missed it the first time. Here __sk_dst_get does not get reference...

net/ipv4/route.c void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu) · line 1002

Quoted 12 lines

net/ipv4/route.c +12 -0

+
+	if (sock_owned_by_user(sk) || !rt) {
+		__ipv4_sk_update_pmtu(skb, sk, mtu);
+		goto out;
+	}
+
+	__build_flow_key(&fl4, sk, iph, 0, 0, 0, 0, 0);
+
+	if (!__sk_dst_check(sk, 0)) {
+		rt = ip_route_output_flow(sock_net(sk), &fl4, sk);
+		if (IS_ERR(rt))
+			goto out;

		but here rt->dst comes with reference.

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

	May be here we can use 'else dst_hold(&rt->dst);' ?

It is needed for __sk_dst_set.

net/ipv4/route.c void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu) · line 1008

Quoted 5 lines

net/ipv4/route.c +5 -0

+
+	__ip_rt_update_pmtu((struct rtable *) rt->dst.path, &fl4, mtu);
+
+	dst = dst_check(&rt->dst, 0);
+	if (!dst) {

		dst_release(&rt->dst);

net/ipv4/route.c void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu) · line 1013

Quoted 5 lines

net/ipv4/route.c +5 -0

+		rt = ip_route_output_flow(sock_net(sk), &fl4, sk);
+		if (IS_ERR(rt))
+			goto out;
+
+		dst = &rt->dst;

Remove above line...

Review

Quoted 2 lines

(quoted diff) +2 -0

+	}
+

	and use here __sk_dst_set(sk, &rt->dst) instead:

net/ipv4/route.c void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu) · line 1016

Quoted 1 line

net/ipv4/route.c +1 -0

+	 __sk_dst_set(sk, dst);

Another variant is to remember with flag 'new_rt'
that we should call __sk_dst_set, eg. when rt comes from ip_route_output_flow. By this way we can avoid some of the dst_hold/dst_release calls if sk_dst_cache is not changed. IIRC, according to sk_dst_check, dst_check can not return different dst from the ->check method.

Quoted 7 lines

Another variant is to remember with flag 'new_rt'
that we should call __sk_dst_set, eg. when rt comes from ip_route_output_flow. By this way we can avoid some of the dst_hold/dst_release calls if sk_dst_cache is not changed. IIRC, according to sk_dst_check, dst_check can not return different dst from the ->check method.

With this variant, we also need to check the new_rt flag before we call the dst_release() function you mentioned above, right?

I think we should avoid refcounts whenever we can, so I prefer this variant. I'll do a patch.