From: David Miller <davem@davemloft.net> Date: 2012-06-11 09:29:12
There is zero point to this function.
It's only real substance is to perform an extremely outdated BSD4.2
ICMP check, which we can safely remove. If you really have a MTU
limited link being routed by a BSD4.2 derived system, here's a nickel
go buy yourself a real router.
The other actions of ip_rt_frag_needed(), checking and conditionally
updating the peer, are done by the per-protocol handlers of the ICMP
event.
TCP, UDP, et al. have a handler which will receive this event and
transmit it back into the associated route via dst_ops->update_pmtu().
This simplification is important, because it eliminates the one place
where we do not have a proper route context in which to make an
inetpeer lookup.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/route.h | 2 --
net/ipv4/icmp.c | 4 +---
net/ipv4/route.c | 61 --------------------------------------------------
net/rxrpc/ar-error.c | 4 ----
4 files changed, 1 insertion(+), 70 deletions(-)
@@ -81,10 +81,6 @@ void rxrpc_UDP_error_report(struct sock *sk)_net("I/F MTU %u",mtu);}-/* ip_rt_frag_needed() may have eaten the info */-if(mtu==0)-mtu=ntohs(icmp_hdr(skb)->un.frag.mtu);-if(mtu==0){/* they didn't give us a size, estimate one */if(mtu>1500){
On Mon, Jun 11, 2012 at 02:29:11AM -0700, David Miller wrote:
-unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
- unsigned short new_mtu,
- struct net_device *dev)
-{
- unsigned short old_mtu = ntohs(iph->tot_len);
- unsigned short est_mtu = 0;
- struct inet_peer *peer;
-
- peer = inet_getpeer_v4(net->ipv4.peers, iph->daddr, 1);
- if (peer) {
- unsigned short mtu = new_mtu;
-
- if (new_mtu < 68 || new_mtu >= old_mtu) {
- /* BSD 4.2 derived systems incorrectly adjust
- * tot_len by the IP header length, and report
- * a zero MTU in the ICMP message.
- */
- if (mtu == 0 &&
- old_mtu >= 68 + (iph->ihl << 2))
- old_mtu -= iph->ihl << 2;
- mtu = guess_mtu(old_mtu);
- }
-
- if (mtu < ip_rt_min_pmtu)
- mtu = ip_rt_min_pmtu;
- if (!peer->pmtu_expires || mtu < peer->pmtu_learned) {
- unsigned long pmtu_expires;
-
- pmtu_expires = jiffies + ip_rt_mtu_expires;
- if (!pmtu_expires)
- pmtu_expires = 1UL;
-
- est_mtu = mtu;
- peer->pmtu_learned = mtu;
- peer->pmtu_expires = pmtu_expires;
- atomic_inc(&__rt_peer_genid);
- }
-
- inet_putpeer(peer);
- }
- return est_mtu ? : new_mtu;
-}
-
It seems that we don't cache the learned pmtu informations
in some cases with ip_rt_frag_needed() removed.
At least when doing a simple ping test on a network that has
a router with mtu 1300 along the path, the following happens:
bash-3.00# ping -c 4 -s 1400 192.168.40.2
PING 192.168.40.2 (192.168.40.2) 1400(1428) bytes of data.
From 10.2.2.2 icmp_seq=1 Frag needed and DF set (mtu = 1300)
From 10.2.2.2 icmp_seq=2 Frag needed and DF set (mtu = 1300)
From 10.2.2.2 icmp_seq=3 Frag needed and DF set (mtu = 1300)
From 10.2.2.2 icmp_seq=4 Frag needed and DF set (mtu = 1300)
--- 192.168.40.2 ping statistics ---
4 packets transmitted, 0 received, +4 errors, 100% packet loss, time 3005ms
We should learn the pmtu information with the first packet,
all further packets should get fragmented according to
the learned informations. Unfortunately we don't cache
these informations:
bash-3.00# ip r g 192.168.40.2
192.168.40.2 via 192.168.20.1 dev eth0 src 192.168.20.2
cache
It seems that we don't cache the learned pmtu informations
in some cases with ip_rt_frag_needed() removed.
We need to find a way to implement this then, in such a way
that we have the route context used to send the ping packet
out.
Otherwise, it's impossible to record the information properly.
From: David Miller <davem@davemloft.net> Date: 2012-06-11 11:28:14
From: David Miller <davem@davemloft.net>
Date: Mon, 11 Jun 2012 04:20:24 -0700 (PDT)
We need to find a way to implement this then, in such a way
that we have the route context used to send the ping packet
out.
The problem is RAW sockets right? If so, then this is where the
fix belongs.
There is nothing preventing the RAW socket code from remembering the
last route used, as well as the flow4 key used to look it up, and
processing the PMTU message appropriately in raw_err() using that
remembered information if the flow4 key matches.
On Mon, Jun 11, 2012 at 04:28:13AM -0700, David Miller wrote:
From: David Miller <davem@davemloft.net>
Date: Mon, 11 Jun 2012 04:20:24 -0700 (PDT)
quoted
We need to find a way to implement this then, in such a way
that we have the route context used to send the ping packet
out.
The problem is RAW sockets right? If so, then this is where the
fix belongs.
Hm, I've just tried with tracepath (udp) and I also don't see the
pmtu informations cached.
I still had no time to look deeper into the new inetpeer code,
I've just gave it a quick try. I'll try to find out what's going on.
On Mon, Jun 11, 2012 at 04:28:13AM -0700, David Miller wrote:
quoted
From: David Miller <davem@davemloft.net>
Date: Mon, 11 Jun 2012 04:20:24 -0700 (PDT)
quoted
We need to find a way to implement this then, in such a way
that we have the route context used to send the ping packet
out.
The problem is RAW sockets right? If so, then this is where the
fix belongs.
Hm, I've just tried with tracepath (udp) and I also don't see the
pmtu informations cached.
I still had no time to look deeper into the new inetpeer code,
I've just gave it a quick try. I'll try to find out what's going on.
Here below is the kind of patch I was suggesting we make. I did a
simple test to make sure the update MTU code path is taken in
raw_err().
But I'm having second thoughts about whether any of this is a good
idea.
UDP works by notifying userspace of PMTU events. And this is
mandatory, if we're setting DF we have to get the user to decrease the
size of it's datagram writes below the reported PMTU value.
As a consequence I believe RAW sockets should also work via
notifications.
And therefore it can be argued that in neither case should we update
the routing cache PMTU information. This is in line with the fact
that TCP goes to great lengths to validate that the PMTU it's getting
is really legitimate and not forged, and UDP and RAW have no way of
making such strict checks.
And it also shows that those TCP strict checks were for nothing
beforehand. That PMTU update guard done by TCP was (before my changes
this past weekend) pointless because we were doing the PMTU update
unconditionally earlier in the ICMP handling.
On Mon, Jun 11, 2012 at 04:02:58PM -0700, David Miller wrote:
Here below is the kind of patch I was suggesting we make. I did a
simple test to make sure the update MTU code path is taken in
raw_err().
I can confirm that your patch restores the old behaviour of ping.
But I'm having second thoughts about whether any of this is a good
idea.
UDP works by notifying userspace of PMTU events. And this is
mandatory, if we're setting DF we have to get the user to decrease the
size of it's datagram writes below the reported PMTU value.
As a consequence I believe RAW sockets should also work via
notifications.
And therefore it can be argued that in neither case should we update
the routing cache PMTU information.
Should be ok as long as all userspace applications that use UDP or
RAW sockets handle pmtu event notifications properly.
ping might be a special case, but now the behaviour of a big
sized ping (say 1400 byte on a network that has a router with
mtu 1300 along the path) with IP_PMTUDISC_WANT might depend on
whether the cached pmtu informations are updated by a recent
tcp connection.
If we had no tcp connection before, we see the behaviour that
I described in my first mail. All packets have the DF bit set.
If a tcp connection updated the cached pmtu informations recently,
the packets don't have the DF bit set. They are fragmented according
the cached pmtu informations instead.
Other applications that do not care for pmtu event notifications
might be in a similar situation. So perhaps we need the kind of
patch you are suggested.
On Mon, Jun 11, 2012 at 04:02:58PM -0700, David Miller wrote:
quoted
UDP works by notifying userspace of PMTU events. And this is
mandatory, if we're setting DF we have to get the user to decrease the
size of it's datagram writes below the reported PMTU value.
As a consequence I believe RAW sockets should also work via
notifications.
And therefore it can be argued that in neither case should we update
the routing cache PMTU information.
Should be ok as long as all userspace applications that use UDP or
RAW sockets handle pmtu event notifications properly.
I am convinced that they absolutely must, if they use IP_PMTUDISC_DO.
Otherwise they will continue making larger-than-PMTU sendmsg()
requests. As these are datagram sockets, we can't simply segment the
data.
ping might be a special case, but now the behaviour of a big
sized ping (say 1400 byte on a network that has a router with
mtu 1300 along the path) with IP_PMTUDISC_WANT might depend on
whether the cached pmtu informations are updated by a recent
tcp connection.
If we had no tcp connection before, we see the behaviour that
I described in my first mail. All packets have the DF bit set.
If a tcp connection updated the cached pmtu informations recently,
the packets don't have the DF bit set. They are fragmented according
the cached pmtu informations instead.
Other applications that do not care for pmtu event notifications
might be in a similar situation. So perhaps we need the kind of
patch you are suggested.
We can't do exactly as my patch did, because it allows remote entities
to easily poison PMTU information. All they have to know is that
there is some UDP or RAW socket open with a certain ID and then send
forged ICMP to us.
What we possibly could do is adjust the socket's IP_PMTUDISC_* setting
from IP_PMTUDISC_WANT to IP_PMTUDISC_DONT in response to PMTU
messages.
This seems to solve all the problems. Individual RAW and UDP sockets
get the behavior they did before, and route cache PMTU poisoning is
less of an issue.
From: David Miller <davem@davemloft.net> Date: 2012-06-13 04:22:39
From: David Miller <davem@davemloft.net>
Date: Tue, 12 Jun 2012 13:33:33 -0700 (PDT)
What we possibly could do is adjust the socket's IP_PMTUDISC_* setting
from IP_PMTUDISC_WANT to IP_PMTUDISC_DONT in response to PMTU
messages.
This seems to solve all the problems. Individual RAW and UDP sockets
get the behavior they did before, and route cache PMTU poisoning is
less of an issue.
@@ -416,6 +416,16 @@ static inline struct ipv6_pinfo * inet6_sk(const struct sock *__sk)returninet_sk(__sk)->pinet6;}+/* We don't want to update the routing tables because there is no way+*tovalidatethelegitimacyofthisPMTUevent.Instead,downgrade+*thePMTUsettingofthesocket.+*/+staticinlinevoidinet6_datagram_pmtu_event(structipv6_pinfo*np)+{+if(np->pmtudisc==IP_PMTUDISC_WANT)+np->pmtudisc=IP_PMTUDISC_DONT;+}+staticinlinestructinet6_request_sock*inet6_rsk(conststructrequest_sock*rsk){
@@ -199,6 +199,16 @@ static inline void inet_sk_copy_descendant(struct sock *sk_to,}#endif+/* We don't want to update the routing cache because there is no way+*tovalidatethelegitimacyofthisPMTUevent.Instead,downgrade+*thePMTUsettingofthesocket.+*/+staticinlinevoidinet_datagram_pmtu_event(structinet_sock*inet)+{+if(inet->pmtudisc==IP_PMTUDISC_WANT)+inet->pmtudisc=IP_PMTUDISC_DONT;+}+externintinet_sk_rebuild_header(structsock*sk);externu32inet_ehash_secret;
On Tue, Jun 12, 2012 at 01:33:33PM -0700, David Miller wrote:
We can't do exactly as my patch did, because it allows remote entities
to easily poison PMTU information. All they have to know is that
there is some UDP or RAW socket open with a certain ID and then send
forged ICMP to us.
Yes, I know what you mean. But not updating the the cached pmtu
informations results in slow path fragmentation along the path.
Btw. what happens to ipv6 if we stop doing pmtu discovery?
Shouldn't we reduce the packet size to 1280 bytes then?
What we possibly could do is adjust the socket's IP_PMTUDISC_* setting
from IP_PMTUDISC_WANT to IP_PMTUDISC_DONT in response to PMTU
messages.
I think an application that sets IP_PMTUDISC_WANT explicitly will
rely on the fact that the kernel does pmtu discovery. Changing
the socket setting to IP_PMTUDISC_DONT the first time we get into
trouble makes IP_PMTUDISC_WANT pointless for udp and raw sockets.
Another option would be to change the sockets default setting
from IP_PMTUDISC_WANT to IP_PMTUDISC_DONT (at least for udp and
raw) and do pmtu discovery if an application sets IP_PMTUDISC_WANT.
With this we don't have the pmtu cache poisoning issue as the default.
We would only have it if a sockets sets IP_PMTUDISC_WANT explicitly.
This is not perfect too, but I fear there is no perfect solution here.
I think an application that sets IP_PMTUDISC_WANT explicitly will
rely on the fact that the kernel does pmtu discovery. Changing
the socket setting to IP_PMTUDISC_DONT the first time we get into
trouble makes IP_PMTUDISC_WANT pointless for udp and raw sockets.
How so?
We are mimicking exactly what would happen if we had just created
a new routing cache entry when the application openned the socket.
There is no behavioral difference whatsoever.
We absolutely do perform PMTU discovery, the first large packet
will trigger it. And then, as if we had lowered the PMTU in
the routing cache entry, we will stop setting DF in the packets.
Because that is how the IP_PMTUDISC_* checks work in the IP output
path in the place that decides whether to set DF or not.
Another option would be to change the sockets default setting
from IP_PMTUDISC_WANT to IP_PMTUDISC_DONT (at least for udp and
raw) and do pmtu discovery if an application sets IP_PMTUDISC_WANT.
Changing defaults doesn't make the problem go away, and is also
unexpected.
I did all of my testing using the "-M" option of ping.
I think an application that sets IP_PMTUDISC_WANT explicitly will
rely on the fact that the kernel does pmtu discovery. Changing
the socket setting to IP_PMTUDISC_DONT the first time we get into
trouble makes IP_PMTUDISC_WANT pointless for udp and raw sockets.
How so?
We are mimicking exactly what would happen if we had just created
a new routing cache entry when the application openned the socket.
There is no behavioral difference whatsoever.
We absolutely do perform PMTU discovery, the first large packet
will trigger it. And then, as if we had lowered the PMTU in
the routing cache entry, we will stop setting DF in the packets.
Maybe I missunderstood what you meant. I thought that you don't want
to update the pmtu cache informations at all on udp and raw.
If we update the pmtu cache informations with first large packet,
I agree absolutely.
I think an application that sets IP_PMTUDISC_WANT explicitly will
rely on the fact that the kernel does pmtu discovery. Changing
the socket setting to IP_PMTUDISC_DONT the first time we get into
trouble makes IP_PMTUDISC_WANT pointless for udp and raw sockets.
How so?
We are mimicking exactly what would happen if we had just created
a new routing cache entry when the application openned the socket.
There is no behavioral difference whatsoever.
We absolutely do perform PMTU discovery, the first large packet
will trigger it. And then, as if we had lowered the PMTU in
the routing cache entry, we will stop setting DF in the packets.
Maybe I missunderstood what you meant. I thought that you don't want
to update the pmtu cache informations at all on udp and raw.
If we update the pmtu cache informations with first large packet,
I agree absolutely.
We don't update the PMTU.
But we behave as if we did.
The only effect the IP_PMTUDISC_* values have is in deciding whether
to set the DF flag in the outgoing packets.
I think an application that sets IP_PMTUDISC_WANT explicitly will
rely on the fact that the kernel does pmtu discovery. Changing
the socket setting to IP_PMTUDISC_DONT the first time we get into
trouble makes IP_PMTUDISC_WANT pointless for udp and raw sockets.
How so?
We are mimicking exactly what would happen if we had just created
a new routing cache entry when the application openned the socket.
There is no behavioral difference whatsoever.
We absolutely do perform PMTU discovery, the first large packet
will trigger it. And then, as if we had lowered the PMTU in
the routing cache entry, we will stop setting DF in the packets.
Maybe I missunderstood what you meant. I thought that you don't want
to update the pmtu cache informations at all on udp and raw.
If we update the pmtu cache informations with first large packet,
I agree absolutely.
We don't update the PMTU.
But we behave as if we did.
The only effect the IP_PMTUDISC_* values have is in deciding whether
to set the DF flag in the outgoing packets.
With your patch applied, we stop setting the DF bit after we received
a 'need to frag' ICMP message, but we don't fragment. We send the packets
out unfragmented. Before we removed ip_rt_frag_needed(), we did the
fragmentation according to the pmtu informations we got from the icmp
message. Now the router with the low mtu has to do the fragmentation.
With your patch applied, we stop setting the DF bit after we
received a 'need to frag' ICMP message, but we don't fragment. We
send the packets out unfragmented. Before we removed
ip_rt_frag_needed(), we did the fragmentation according to the pmtu
informations we got from the icmp message. Now the router with the
low mtu has to do the fragmentation.
Ok, then if we want to do the fragmentation locally then we have to
consider my initial patch which updates the PMTU in raw_err().
Did you test that? I mean specifically, this patch:
http://marc.info/?l=linux-netdev&m=133945597319917&w=2
If it works for you, I will try to extend it to the other datagram
cases.
Thanks.
On Wed, Jun 13, 2012 at 10:42:03PM -0700, David Miller wrote:
Ok, then if we want to do the fragmentation locally then we have to
consider my initial patch which updates the PMTU in raw_err().
Did you test that? I mean specifically, this patch:
http://marc.info/?l=linux-netdev&m=133945597319917&w=2
If it works for you, I will try to extend it to the other datagram
cases.
Yes, I can confirm that this one works. It restores the old behaviour.
With your patch applied, we stop setting the DF bit after we
received a 'need to frag' ICMP message, but we don't fragment. We
send the packets out unfragmented. Before we removed
ip_rt_frag_needed(), we did the fragmentation according to the pmtu
informations we got from the icmp message. Now the router with the
low mtu has to do the fragmentation.
Ok, then if we want to do the fragmentation locally then we have to
consider my initial patch which updates the PMTU in raw_err().
Did you test that? I mean specifically, this patch:
http://marc.info/?l=linux-netdev&m=133945597319917&w=2
If it works for you, I will try to extend it to the other datagram
cases.
Actually, thinking some more, we could extend my inet->pmtudisc patch
to achieve a similar effect.
Essentially we'd have a socket local PMTU value for datagram sockets.
Would you be OK with that approach?
I like the inet->pmtudisc way of solving this problem, because it:
1) Requires no special code to "remember" the flow used for the last
socket sendmsg() call.
2) In the events of a malicious attempt to poison the routing cache
PMTU information, only one socket will be harmed, rather than
the whole system.
I tried to look for inspiration in other systems, but all of them lack
source based routing and other things we support, so they just use
a purely destination address based cache for PMTU information.
Other systems also don't have to deal with SO_BINDTODEVICE which
influences the route.
So we absolutely have to make our PMTU operations with the full
context used to emit the packet.
On Wed, Jun 13, 2012 at 10:59:41PM -0700, David Miller wrote:
Actually, thinking some more, we could extend my inet->pmtudisc patch
to achieve a similar effect.
Essentially we'd have a socket local PMTU value for datagram sockets.
Would you be OK with that approach?
This would require to maintain socket local pmtu expire times too.
Also, which of these pmtu values do we report if a user asks for
that? And how should we flush all these pmtu values?
It could have some side effects. But if we get it to work,
it would be an improvement. I'm fine with everything
that works in the end :-)
On Wed, Jun 13, 2012 at 10:59:41PM -0700, David Miller wrote:
quoted
Actually, thinking some more, we could extend my inet->pmtudisc patch
to achieve a similar effect.
Essentially we'd have a socket local PMTU value for datagram sockets.
Would you be OK with that approach?
This would require to maintain socket local pmtu expire times too.
Also, which of these pmtu values do we report if a user asks for
that? And how should we flush all these pmtu values?
It could have some side effects. But if we get it to work,
it would be an improvement. I'm fine with everything
that works in the end :-)
Your right, maybe the route updating approach is therefore better.
I'll play around with my original patch.
Thanks.