inet6: Use IfAPI helper in in6_ifstat_inc

[jhibbits]

Summary

The only use of ifnet internals that ipsec has is through the
in6_ifstat_inc macro. Make this instead use the IfAPI if_getafdata API,
and remove if_private from ipsec.

[zlei]

sys/netinet6/in6_var.h

549

The macro in6_ifstat_inc() has lots of consumers.

Do we need to evaluate the impact to the consumers in the fast path:

1. sys/netinet6/ip6_fastfwd.c: in6_ifstat_inc(nh->nh_ifp, ifs6_out_forward)
2. sys/netinet6/ip6_input.c: in6_ifstat_inc(rcvif, ifs6_in_receive)
3. sys/netinet6/ip6_input.c: in6_ifstat_inc(rcvif, ifs6_in_deliver)
4. sys/netinet6/ip6_output.c: in6_ifstat_inc(ifp, ifs6_out_request)

[glebius]

The if_afdata[] array comes from the old BSD times when it was expected that there would be support for many many address families (e.g. IPX, AppleTalk, etc). Right now it has only two entries AF_INET and AF_INET6. It is very very very unlikely it will ever get a third one. It is much more likely that the array will go away and we will have just ifp->if_inet and ifp->if_inet6. Or maybe something more complicated. Anyway, the access to this data is going to change anyway, so there is no point in overdesigning it right now. Any solution for the sake of IfAPI cleanness is acceptable.

I'll add couple more people to confirm or argue my statement.

[ae]

In D42988#980253, @glebius wrote:

The if_afdata[] array comes from the old BSD times when it was expected that there would be support for many many address families (e.g. IPX, AppleTalk, etc). Right now it has only two entries AF_INET and AF_INET6. It is very very very unlikely it will ever get a third one. It is much more likely that the array will go away and we will have just ifp->if_inet and ifp->if_inet6. Or maybe something more complicated. Anyway, the access to this data is going to change anyway, so there is no point in overdesigning it right now. Any solution for the sake of IfAPI cleanness is acceptable.

Do you plan to rework access to if_afdata? There are still several panics related to access to already freed if_afdata[AF_INET6].

[gallatin]

In D42988#980253, @glebius wrote:

The if_afdata[] array comes from the old BSD times when it was expected that there would be support for many many address families (e.g. IPX, AppleTalk, etc). Right now it has only two entries AF_INET and AF_INET6. It is very very very unlikely it will ever get a third one. It is much more likely that the array will go away and we will have just ifp->if_inet and ifp->if_inet6. Or maybe something more complicated. Anyway, the access to this data is going to change anyway, so there is no point in overdesigning it right now. Any solution for the sake of IfAPI cleanness is acceptable.

I'll add couple more people to confirm or argue my statement.

Agreed

[glebius]

In D42988#980277, @ae wrote:

Do you plan to rework access to if_afdata? There are still several panics related to access to already freed if_afdata[AF_INET6].

Can you please assign those PRs to me? Or send links to information if there is no PR.

[zlei]

In D42988#980253, @glebius wrote:

The if_afdata[] array comes from the old BSD times when it was expected that there would be support for many many address families (e.g. IPX, AppleTalk, etc). Right now it has only two entries AF_INET and AF_INET6. It is very very very unlikely it will ever get a third one. It is much more likely that the array will go away and we will have just ifp->if_inet and ifp->if_inet6. Or maybe something more complicated. Anyway, the access to this data is going to change anyway, so there is no point in overdesigning it right now. Any solution for the sake of IfAPI cleanness is acceptable.

I'll add couple more people to confirm or argue my statement.

Agreed.

[ae]

In D42988#981761, @glebius wrote:

Do you plan to rework access to if_afdata? There are still several panics related to access to already freed if_afdata[AF_INET6].

Can you please assign those PRs to me? Or send links to information if there is no PR.

Recently I saw two panics with slightly different traces, but they have the same problem - some packet got delayed (queued into some deferred handling), then an interface has been destroyed, then packet returns back into the flow and this leads to the panic due to mbuf references destroyed ifnet. An ifnet pointer is still valid, but if_afdata[AF_INET6] is already freed and NULL, so in6_ifstat_inc() at the end of ip6_input() produces panic.

--- trap 0xc, rip = 0xffffffff80d3a819, rsp = 0xfffffe01fdb05930, rbp = 0xfffffe01fdb05a00 ---
ip6_input() at ip6_input+0x659/frame 0xfffffe01fdb05a00
netisr_dispatch_src() at netisr_dispatch_src+0xca/frame 0xfffffe01fdb05a50
dummynet_send() at dummynet_send+0x179/frame 0xfffffe01fdb05a90
dummynet_task() at dummynet_task+0x2b2/frame 0xfffffe01fdb05b00

The second one was also triggered by NULL if_afdata, but through the ND_IFINFO(ifp) macro:

--- trap 0xc, rip = 0xffffffff80d37633, rsp = 0xfffffe00d3b006e0, rbp = 0xfffffe00d3b007f0 ---
in6_selectsrc() at in6_selectsrc+0x6e3/frame 0xfffffe00d3b007f0
in6_selectsrc_addr() at in6_selectsrc_addr+0x62/frame 0xfffffe00d3b00850
icmp6_reflect() at icmp6_reflect+0x1a2/frame 0xfffffe00d3b00910
icmp6_error() at icmp6_error+0x360/frame 0xfffffe00d3b00960
ip6_tryforward() at ip6_tryforward+0x14f/frame 0xfffffe00d3b009d0
ip6_input() at ip6_input+0x6cd/frame 0xfffffe00d3b00ab0
swi_net() at swi_net+0x12b/frame 0xfffffe00d3b00b20

So, dummynet, ipsec, netisr can lead to such panics. These panics was triggered with ifconfig ipsecX destroy && ifconfig ipsecX create.
Current workaround is ifconfig ipsecX down && sleep 2 && ifconfig ipsecX destroy && ifconfig ipsecX create.

[ae]

I think https://reviews.freebsd.org/D32811, https://reviews.freebsd.org/D33064 also are related.

[kp]

In D42988#981761, @glebius wrote:

In D42988#980277, @ae wrote:

Do you plan to rework access to if_afdata? There are still several panics related to access to already freed if_afdata[AF_INET6].

Can you please assign those PRs to me? Or send links to information if there is no PR.

We see this issue on pfSense as well: https://redmine.pfsense.org/issues/14431 . There are a few different backtraces, but I believe they're all the same root problem.
The short version of the story is that there's outbound traffic, which does a route lookup (often to get an IPv6 hop limit), only to be returned a netxhop with a struct ifnet that has a NULL afdata[AF_INET6].
I believe it to correlated with said struct ifnet going away (in the case of pfSense users often because they're PPPoE users).

In addition to the pfSense bug report there's also a summary e-mail on net@: https://lists.freebsd.org/archives/freebsd-net/2023-October/004104.html

I still have both core dump and debug kernel for at least one occurrence of this issue (on a pfsense kernel, roughly equivalent to main from August).

[glebius]

In D42988#981874, @kp wrote:

In D42988#981761, @glebius wrote:

In D42988#980277, @ae wrote:

Do you plan to rework access to if_afdata? There are still several panics related to access to already freed if_afdata[AF_INET6].

Can you please assign those PRs to me? Or send links to information if there is no PR.

We see this issue on pfSense as well: https://redmine.pfsense.org/issues/14431 . There are a few different backtraces, but I believe they're all the same root problem.
The short version of the story is that there's outbound traffic, which does a route lookup (often to get an IPv6 hop limit), only to be returned a netxhop with a struct ifnet that has a NULL afdata[AF_INET6].
I believe it to correlated with said struct ifnet going away (in the case of pfSense users often because they're PPPoE users).

In addition to the pfSense bug report there's also a summary e-mail on net@: https://lists.freebsd.org/archives/freebsd-net/2023-October/004104.html

I still have both core dump and debug kernel for at least one occurrence of this issue (on a pfsense kernel, roughly equivalent to main from August).

There is definitely lots of place for improvement & simplification here. For example, dom_ifdetach should be retired in favor of eventhandler(9), just like I did for other original 4.3BSD domain/proto stuff.

Looks like we got a lot of evidence about the problem. Can we have a single place to track it? Be it pfsense redmine, FreeBSD bugzilla, phabricator?

[jhibbits]

In D42988#982075, @glebius wrote:

In D42988#981874, @kp wrote:

In D42988#981761, @glebius wrote:

In D42988#980277, @ae wrote:

Do you plan to rework access to if_afdata? There are still several panics related to access to already freed if_afdata[AF_INET6].

Can you please assign those PRs to me? Or send links to information if there is no PR.

We see this issue on pfSense as well: https://redmine.pfsense.org/issues/14431 . There are a few different backtraces, but I believe they're all the same root problem.
The short version of the story is that there's outbound traffic, which does a route lookup (often to get an IPv6 hop limit), only to be returned a netxhop with a struct ifnet that has a NULL afdata[AF_INET6].
I believe it to correlated with said struct ifnet going away (in the case of pfSense users often because they're PPPoE users).

In addition to the pfSense bug report there's also a summary e-mail on net@: https://lists.freebsd.org/archives/freebsd-net/2023-October/004104.html

I still have both core dump and debug kernel for at least one occurrence of this issue (on a pfsense kernel, roughly equivalent to main from August).

There is definitely lots of place for improvement & simplification here. For example, dom_ifdetach should be retired in favor of eventhandler(9), just like I did for other original 4.3BSD domain/proto stuff.

Looks like we got a lot of evidence about the problem. Can we have a single place to track it? Be it pfsense redmine, FreeBSD bugzilla, phabricator?

I love this discussion, but can it be moved elsewhere? I think it goes beyond this review :)