ICMP_PARAMETERPROB and ICMP_TIME_EXCEEDED during connect

DISCUSSION Jakub Kicinski 5 participants 8 messages Mar 26, 2024 – Apr 2, 2024

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Jakub Kicinski

author maintainer

2024-03-26 20:34 UTC #0 lore raw

Hi!

I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:

A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.

Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?

Somewhat related in tcp_v4_err() we do:

	switch (sk->sk_state) {
	case TCP_SYN_SENT:
	case TCP_SYN_RECV:
		[...]

		if (!sock_owned_by_user(sk)) {
			WRITE_ONCE(sk->sk_err, err);

			sk_error_report(sk);

			tcp_done(sk);
		} else {
			WRITE_ONCE(sk->sk_err_soft, err);
		}
		goto out;
	}

So the error is soft if socket is locked, and I can't find anything in backlog processing which would pay attention. So it seems that under certain conditions we already ignore it.

Can we ignore it always, or perhaps conditionally based on IP_RECVERR?

Neal Cardwell

2024-03-26 22:03 UTC #1 lore raw

Quoted 16 lines · Jakub Kicinski

Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.

Quoted 2 lines

Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?

Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:

0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP

However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:

b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP

I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.

neal

Jakub Kicinski

author maintainer

2024-03-26 23:55 UTC #2 lore raw

Quoted 29 lines · On Tue, 26 Mar 2024 23:03:26 +0100 Neal Cardwell wrote:

Quoted 16 lines · Jakub Kicinski
Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.
Quoted 2 lines
Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?
Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:
0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP
However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:

Quoted 6 lines

b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP
I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.

Ah.

Thank you!!

Eric Dumazet

2024-03-27 13:05 UTC #3 lore raw

Quoted 33 lines · Jakub Kicinski

Quoted 31 lines · On Tue, 26 Mar 2024 23:03:26 +0100 Neal Cardwell wrote:
Quoted 16 lines · Jakub Kicinski
Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.
Quoted 2 lines
Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?
Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:
0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP
However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:
b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP

Quoted 8 lines

Quoted 4 lines
I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.
Ah.
Thank you!!

For the record, Leon Romanovsky brought this issue directly to Linus Torvalds, stating that I broke things.

It tooks weeks before Shachar did some debugging, but with no conclusion I recall.

This kind of stuff makes me not very eager to work on this point.

Leon Romanovsky

2024-04-02 13:21 UTC #4 lore raw

Quoted 39 lines · Eric Dumazet

Quoted 38 lines · Jakub Kicinski
Quoted 35 lines · On Tue, 26 Mar 2024 23:03:26 +0100 Neal Cardwell wrote:
Quoted 16 lines · Jakub Kicinski
Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.
Quoted 2 lines
Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?
Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:
0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP
However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:
b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP
I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.

Quoted 6 lines

Quoted 3 lines
Ah.
Thank you!!
For the record, Leon Romanovsky brought this issue directly to Linus Torvalds, stating that I broke things.

Just to make it clear, Linus was involved after we didn't progress for more than one month after initial starting "Bug report connect to VM with Vagrant", while approaching to merge window.
https://lore.kernel.org/netdev/MN2PR12MB44863139E562A59329E89DBEB982A@MN2PR12MB4486.namprd12.prod.outlook.com/

Despite long standing netdev patch flow: apply fast -> revert fast, this patch was treated differently.

Quoted 3 lines

It tooks weeks before Shachar did some debugging, but with no conclusion I recall.

Shachar didn't do debugging, she didn't write the bisected patch. She is verification engineer who was ready to run ANY tests and try ANY debug patch which you wanted.

Quoted 3 lines

This kind of stuff makes me not very eager to work on this point.

OK, so it is not important at the end.

Eric Dumazet

2024-04-02 13:32 UTC #5 lore raw

Quoted 48 lines · Leon Romanovsky

Quoted 39 lines · Eric Dumazet
Quoted 38 lines · Jakub Kicinski
Quoted 35 lines · On Tue, 26 Mar 2024 23:03:26 +0100 Neal Cardwell wrote:
Quoted 16 lines · Jakub Kicinski
Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.
Quoted 2 lines
Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?
Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:
0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP
However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:
b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP
I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.
Quoted 6 lines
Quoted 3 lines
Ah.
Thank you!!
For the record, Leon Romanovsky brought this issue directly to Linus Torvalds, stating that I broke things.

Quoted 7 lines

Just to make it clear, Linus was involved after we didn't progress for more than one month after initial starting "Bug report connect to VM with Vagrant", while approaching to merge window.
https://lore.kernel.org/netdev/MN2PR12MB44863139E562A59329E89DBEB982A@MN2PR12MB4486.namprd12.prod.outlook.com/
Despite long standing netdev patch flow: apply fast -> revert fast, this patch was treated differently.

I was waiting input from you. I think you only waited for "revert first"

Quoted 10 lines

Quoted 4 lines
It tooks weeks before Shachar did some debugging, but with no conclusion I recall.
Shachar didn't do debugging, she didn't write the bisected patch. She is verification engineer who was ready to run ANY tests and try ANY debug patch which you wanted.
Quoted 1 line

Quoted 4 lines

Quoted 2 lines
This kind of stuff makes me not very eager to work on this point.
OK, so it is not important at the end.

I certainly do not want to waste time arguing with you on a valid patch, which happens to break some buggy user space.

Apparently some people think RFC are not important.

You won.

Jason Xing

2024-04-02 14:17 UTC #6 lore raw

Quoted 73 lines · Eric Dumazet

Quoted 48 lines · Leon Romanovsky
Quoted 39 lines · Eric Dumazet
Quoted 38 lines · Jakub Kicinski
Quoted 35 lines · On Tue, 26 Mar 2024 23:03:26 +0100 Neal Cardwell wrote:
Quoted 16 lines · Jakub Kicinski
Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.
Quoted 2 lines
Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?
Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:
0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP
However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:
b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP
I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.
Quoted 6 lines
Quoted 3 lines
Ah.
Thank you!!
For the record, Leon Romanovsky brought this issue directly to Linus Torvalds, stating that I broke things.
Quoted 7 lines
Just to make it clear, Linus was involved after we didn't progress for more than one month after initial starting "Bug report connect to VM with Vagrant", while approaching to merge window.
https://lore.kernel.org/netdev/MN2PR12MB44863139E562A59329E89DBEB982A@MN2PR12MB4486.namprd12.prod.outlook.com/
Despite long standing netdev patch flow: apply fast -> revert fast, this patch was treated differently.
I was waiting input from you. I think you only waited for "revert first"
Quoted 13 lines
Quoted 4 lines
It tooks weeks before Shachar did some debugging, but with no conclusion I recall.
Shachar didn't do debugging, she didn't write the bisected patch. She is verification engineer who was ready to run ANY tests and try ANY debug patch which you wanted.
Quoted 3 lines
This kind of stuff makes me not very eager to work on this point.

Quoted 6 lines

Quoted 1 line
OK, so it is not important at the end.
I certainly do not want to waste time arguing with you on a valid patch, which happens to break some buggy user space.
Apparently some people think RFC are not important.

RFC is important.

Honestly, I read those threads over and over again. Since she provided some tcpdump logs which do not include ICMP, my question is still the same as Eric: why does this breakage have a relationship with this patch??? I get lost. It doesn't make sense really...

If someone is able to more easily reproduce this issue, I'm happy to help debug.

Thanks,
Jason

Leon Romanovsky

2024-04-02 17:32 UTC #7 lore raw

Quoted 79 lines · Jason Xing

Quoted 78 lines · Eric Dumazet
Quoted 48 lines · Leon Romanovsky
Quoted 39 lines · Eric Dumazet
Quoted 38 lines · Jakub Kicinski
Quoted 35 lines · On Tue, 26 Mar 2024 23:03:26 +0100 Neal Cardwell wrote:
Quoted 16 lines · Jakub Kicinski
Hi!
I got a report from a user surprised/displeased that ICMP_TIME_EXCEEDED breaks connect(), while TCP RFCs say it shouldn't. Even pointing a finger at Linux, RFC5461:
A number of TCP implementations have modified their reaction to all ICMP soft errors and treat them as hard errors when they are received for connections in the SYN-SENT or SYN-RECEIVED states. For example, this workaround has been implemented in the Linux kernel since version 2.0.0 (released in 1996) [Linux]. However, it should be noted that this change violates section 4.2.3.9 of [RFC1122], which states that these ICMP error messages indicate soft error conditions and that, therefore, TCP MUST NOT abort the corresponding connection.
Quoted 2 lines
Is there any reason we continue with this behavior or is it just that nobody ever sent a patch?
Back in November of 2023 Eric did merge a patch to bring the processing in line with section 4.2.3.9 of [RFC1122]:
0a8de364ff7a tcp: no longer abort SYN_SENT when receiving some ICMP
However, the fixed behavior did not meet some expectations of Vagrant (see the netdev thread "Bug report connect to VM with Vagrant"), so for now it got reverted:
b59db45d7eba tcp: Revert no longer abort SYN_SENT when receiving some ICMP
I think the hope was to root-cause the Vagrant issue, fix Vagrant's assumptions, then resubmit Eric's commit. Eric mentioned on Jan 8, 2024: "We will submit the patch again for 6.9, once we get to the root cause." But I don't think anyone has had time to do that yet.
Quoted 6 lines
Quoted 3 lines
Ah.
Thank you!!
For the record, Leon Romanovsky brought this issue directly to Linus Torvalds, stating that I broke things.
Quoted 7 lines
Just to make it clear, Linus was involved after we didn't progress for more than one month after initial starting "Bug report connect to VM with Vagrant", while approaching to merge window.
https://lore.kernel.org/netdev/MN2PR12MB44863139E562A59329E89DBEB982A@MN2PR12MB4486.namprd12.prod.outlook.com/
Despite long standing netdev patch flow: apply fast -> revert fast, this patch was treated differently.
I was waiting input from you. I think you only waited for "revert first"
Quoted 10 lines
Quoted 4 lines
It tooks weeks before Shachar did some debugging, but with no conclusion I recall.
Shachar didn't do debugging, she didn't write the bisected patch. She is verification engineer who was ready to run ANY tests and try ANY debug patch which you wanted.
Quoted 1 line
Quoted 4 lines
Quoted 2 lines
This kind of stuff makes me not very eager to work on this point.
OK, so it is not important at the end.
I certainly do not want to waste time arguing with you on a valid patch, which happens to break some buggy user space.

Quoted 8 lines

Quoted 1 line
Apparently some people think RFC are not important.
RFC is important.
Honestly, I read those threads over and over again. Since she provided some tcpdump logs which do not include ICMP, my question is still the same as Eric: why does this breakage have a relationship with this patch??? I get lost. It doesn't make sense really...

It was unfortunate outcome of moving the discussion to be private.
https://lore.kernel.org/netdev/CANn89i+e2TcvSU1EgrVZRUoEmZ5NDauXd3=kEkjpsGjmaypHOw@mail.gmail.com/

Quoted 6 lines

If someone is able to more easily reproduce this issue, I'm happy to help debug.
Thanks,
Jason