Bug 221137 - FreeBSD 11+ does not send ICMP redirects
Summary: FreeBSD 11+ does not send ICMP redirects
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.1-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: Andrey V. Elsukov
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2017-08-01 08:04 UTC by igorr
Modified: 2018-08-28 07:28 UTC (History)
7 users (show)

See Also:
koobs: mfc-stable11?
koobs: mfc-stable10?


Attachments
Very naive patch to support ICMP redirects. (1.80 KB, patch)
2017-08-01 08:04 UTC, igorr
no flags Details | Diff
New patch for rev 322037. Uses net.inet.ip.redirect as condition to use ip_tryforward() (1.19 KB, patch)
2017-08-04 07:31 UTC, igorr
no flags Details | Diff
Plausible icmp redirect fix for ipv4 and ipv6 (1.72 KB, patch)
2018-08-14 04:56 UTC, Stephen McKay
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description igorr 2017-08-01 08:04:09 UTC
Created attachment 184886 [details]
Very naive patch to support ICMP redirects.

On default gateway "internal" interface has IP 192.168.2.4/21
Also there is dedicated router for 192.168.8.0/23 network. 
So part of routing table on default gateway looks like:
192.168.0.0/21     link#5             U           em0
192.168.8.0/23     192.168.5.116      UGS         em0

Router 192.168.5.116 is accessible from our internal network (of course).

When we had FreeBSD 9.2 on 192.168.2.4 and tried to access some host in 192.168.8.0/23 network it would send ICMP redirect message with new route:

PING 192.168.8.118 (192.168.8.118): 56 data bytes
36 bytes from gw.stc (192.168.2.4): Redirect Host(New addr: 192.168.5.116)
Vr HL TOS  Len   ID Flg  off TTL Pro  cks      Src      Dst
 4  5  00 0054 4442   0 0000  3f  01 ab86 192.168.2.26  192.168.8.118 


I have upgraded FreeBSD version on our default gateway from 9.2 to 11.0. And now it does not send ICMP redirects.

I assume that this is caused by removing net.inet.ip.fastforwarding sysctl:
https://svnweb.freebsd.org/base?view=revision&revision=r290383

Unfortunattely we need ICMP redirects, because not all equipment support getting routes via DHCP options, also some of our workstations and servers have static IP addresses and don't use DHCP.

I have created very naive patch against FreeBSD 11-STABLE (revision 321782). And now ICMP redirects work. Patch is attached. It just checks if packet should be routed to same interface it was received from and in this case just pass control from ip_tryforward() to ip_input() by returning not NULL.
Comment 1 Andrey V. Elsukov freebsd_committer freebsd_triage 2017-08-01 10:50:17 UTC
You can add some security policy as a workaround, this disables the using of ip_tryforward().
Comment 2 igorr 2017-08-01 10:56:08 UTC
But it is good to have ICMP redirects without special tweaks.
Everything worked in 9.X, and I have checked that 10.3 also sends ICMP redirects.
Comment 3 Andrey V. Elsukov freebsd_committer freebsd_triage 2017-08-01 11:09:13 UTC
(In reply to igorr from comment #2)
> But it is good to have ICMP redirects without special tweaks.
> Everything worked in 9.X, and I have checked that 10.3 also sends ICMP
> redirects.

Yes, it was broken with ip_tryforward introduction. But I don't like the solution  with M_SKIP_FIREWALL. This flag can be used by pfil module and clearing it can break something. I prefer to reintroduce sysctl variable to disable tryforward.
Comment 4 Eugene Grosbein freebsd_committer freebsd_triage 2017-08-01 11:19:02 UTC
(In reply to Andrey V. Elsukov from comment #3)

We already have sysctl net.inet.ip.redirect with default value of 1 that is broken now. We could change default to 0 (keep fastforwarding on by default) and disable fast path if net.inet.ip.redirect changed to 1.
Comment 5 igorr 2017-08-01 11:28:24 UTC
I have used M_SKIP_FIREWALL to avoid reintroducing packet into pfil_run_hooks().

As ICMP redirects are not very common, and if there are no much trouble with reprocessing packet in input firewall rules we can remove M_SKIP_FIREWALL from my patch. Or use some different flag.

Also I have not found any places referencing to M_SKIP_FIREWALL except ip_output.c. And it is called after ip_input().

So with my patch ip_tryforward() will be used most of the time. And in rare cases when ICMP redirect should be sent it will not be used.

But I have nothing against using net.inet.ip.redirect as flag for disablinmg fast path and enabling ICMP redirects.
Comment 6 igorr 2017-08-04 07:31:02 UTC
Created attachment 185012 [details]
New patch for rev 322037. Uses net.inet.ip.redirect as condition to use ip_tryforward()

I have made new patch which uses sysctl net.inet.ip.redirect to decide whenever use ip_tryforward().
Also it changes default value for net.inet.ip.redirect to 0.
Comment 7 igorr 2017-08-22 04:27:15 UTC
Can somebody check my patch and if everything is good commit it?
Comment 8 Stephen McKay freebsd_committer freebsd_triage 2018-08-14 04:56:23 UTC
Created attachment 196187 [details]
Plausible icmp redirect fix for ipv4 and ipv6

I'm surprised that this bug is still unfixed after a year and quite surprised that it is marked "Affects Only Me".  ICMP redirects are still in all the RFCs and are not deprecated, so FreeBSD's recent inability to generate them affects everyone who wishes to be RFC compliant.

I have attached a plausible minimalist fix.  When net.inet.ip.redirect is set (net.inet6.ip6.redirect for ipv6) fast path forwarding (which lacks redirect generation ability) is not attempted.  This means the standard code is used and ICMP redirects are generated.

Anyone wishing to have fast path forwarding at the cost of never generating ICMP redirects can disable redirects using sysctl.

This might be considered a strong position to take, but the alternative (ignoring an obvious bug) seems to me to be a stronger position with no up side.

I think this issue should be resolved before 12 ships.  Please let your thoughts be known.
Comment 9 Kubilay Kocak freebsd_committer freebsd_triage 2018-08-14 05:08:05 UTC
CC base r290383 committer
Comment 10 commit-hook freebsd_committer freebsd_triage 2018-08-14 07:54:32 UTC
A commit references this bug:

Author: ae
Date: Tue Aug 14 07:54:14 UTC 2018
New revision: 337736
URL: https://svnweb.freebsd.org/changeset/base/337736

Log:
  Restore ability to send ICMP and ICMPv6 redirects.

  It was lost when tryforward appeared. Now ip[6]_tryforward will be enabled
  only when sending redirects for corresponding IP version is disabled via
  sysctl. Otherwise will be used default forwarding function.

  PR:		221137
  Submitted by:	mckay@
  MFC after:	2 weeks

Changes:
  head/sys/netinet/ip_input.c
  head/sys/netinet6/ip6_input.c
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2018-08-14 08:06:26 UTC
Thank you Andrey
Comment 12 commit-hook freebsd_committer freebsd_triage 2018-08-28 07:24:17 UTC
A commit references this bug:

Author: ae
Date: Tue Aug 28 07:24:10 UTC 2018
New revision: 338343
URL: https://svnweb.freebsd.org/changeset/base/338343

Log:
  MFC r337736:
    Restore ability to send ICMP and ICMPv6 redirects.

    It was lost when tryforward appeared. Now ip[6]_tryforward will be enabled
    only when sending redirects for corresponding IP version is disabled via
    sysctl. Otherwise will be used default forwarding function.

    PR:		221137

Changes:
_U  stable/11/
  stable/11/sys/netinet/ip_input.c
  stable/11/sys/netinet6/ip6_input.c
Comment 13 Andrey V. Elsukov freebsd_committer freebsd_triage 2018-08-28 07:28:04 UTC
Fixed in stable/11 and head/. Thanks!