@bz: There is another bug I believe in the IPv6 VNET shutdown code.

Where is the counterpart mtx_destroy() for the following mtx_init():

void
frag6_init(void)
{

struct ip6q *q6;
int i;

V_ip6_maxfragpackets = IP6_MAXFRAGPACKETS;
frag6_set_bucketsize();
for (i = 0; i < IP6REASS_NHASH; i++) {
        q6 = IP6Q_HEAD(i);
        q6->ip6q_next = q6->ip6q_prev = q6;
        mtx_init(&V_ip6q[i].lock, "ip6qlock", NULL, MTX_DEF);
        V_ip6q[i].count = 0;
}

In D19622#430172, @hselasky wrote:

@bz: There is another bug I believe in the IPv6 VNET shutdown code.

Where is the counterpart mtx_destroy() for the following mtx_init():

void
frag6_init(void)
{

struct ip6q *q6;
int i;

V_ip6_maxfragpackets = IP6_MAXFRAGPACKETS;
frag6_set_bucketsize();
for (i = 0; i < IP6REASS_NHASH; i++) {
        q6 = IP6Q_HEAD(i);
        q6->ip6q_next = q6->ip6q_prev = q6;
        mtx_init(&V_ip6q[i].lock, "ip6qlock", NULL, MTX_DEF);
        V_ip6q[i].count = 0;
}

This came in with the SA at r337776 by @jtl and @jhb . Seems they missed the case. Let's open a separate review to add the missing mtx_destroy().

I'll register strong desire to discard fragments immediately on interface vanishing. I don't expect that to come with any significant weight, my rational is whatever makes state management easier within current codebase and usage patterns of fbsd.

@jtl: Can you please comment?

@kbowling : This is what the previous version of this patch does.

@jtl: Given that we don't have a significant number of panics in this area, I don't see a problem just freeing the fragments. I.E. the current behaviour leads to a panic if a fragment is leftover from an interface. This is only done when interfaces are destroyed and does not affect LINK-flapping.

In D19622#440454, @kbowling wrote:

I'll register strong desire to discard fragments immediately on interface vanishing. I don't expect that to come with any significant weight, my rational is whatever makes state management easier within current codebase and usage patterns of fbsd.

Why do you want to discard a packet if the interface it was received from is going away?

The packet is there, it doesn't matter if the interface goes away after it was received. And we don't have to wait for the interface to come back before continuing processing. We should be able to just process the packet even without that interface it was received on still being present when the processing happens.

In D19622#441636, @tuexen wrote:

In D19622#440454, @kbowling wrote:

I'll register strong desire to discard fragments immediately on interface vanishing. I don't expect that to come with any significant weight, my rational is whatever makes state management easier within current codebase and usage patterns of fbsd.

Why do you want to discard a packet if the interface it was received from is going away?

The packet is there, it doesn't matter if the interface goes away after it was received. And we don't have to wait for the interface to come back before continuing processing. We should be able to just process the packet even without that interface it was received on still being present when the processing happens.

And all kinds of upper-layer logics will keep working with an NULL ifp? I am just wondering when we do address list walks, statistics, hashes, ...

What do we do with packets in ifqs when the interface goes away? We don't process them anymore either.

Why would you want the packet to stay so badly. The Internet is designed that we can lose packets and we do all the time. Buffer overflows, link drops, routing glitches, .. out of memory conditions in our stack, ... There are zero guarantees any packet will ever make it. The chances that a partial packet sitting in the reassembly queue will become a "full" packet when an interface goes away become narrowly zero.

Even if this patch doesn't change the behaviour for link flapping, I'd even argue the same thing for link-flaps. What happens on real hardware in these cases? I mean the time a link-flap takes is usually going to be a multiple of receive rings/buffers/.. so by the time your link comes back up the remainders of your fragments for the "full" packet have long gone down the orcus, or worse, might even have triggered ICMP(v6) back to the sender already where the connection was dropped rather than waiting on resend-timers.

I think preserving any kind of state which we can get rid of, we are just trying to be to complicated instead of KISS.

And we have two problems to address here:
(1) purging state from all kinds of queues (or in a future trying not to have that state in this way anymore)
(2) the mbuf ifp problem (which is a way larger problem to address and affects all parts of the stack) and should be a different problem

Both problems are intermixed here; let's step-by-step solve (1) and leave (2) for another review.

@bz - once a packet (fragment in this case) has made it to it's destination, I would argue that you shouldn't needlessly drop that packet any more. (Host vs. Router behavior). An interface flapping frequently, and removing all enqueued fragments while doing this, would reduce the usability of such an interface from marginal to completely inoperable.

Using the ifp of the last received fragment rather the ifp of the initial one (in time, not relative offset) sounds to me like a good compromise.

In D19622#443714, @rscheff_gmx.at wrote:

@bz - once a packet (fragment in this case) has made it to it's destination, I would argue that you shouldn't needlessly drop that packet any more. (Host vs. Router behavior). An interface flapping frequently, and removing all enqueued fragments while doing this, would reduce the usability of such an interface from marginal to completely inoperable.

Using the ifp of the last received fragment rather the ifp of the initial one (in time, not relative offset) sounds to me like a good compromise.

But wait - this should only be called when an ifp is DELETED, not when the interface flaps link / administrative state. If an interface is /deleted/ then we should honestly drop the frames.

I feel like "interface flap" is a bit mis-labeled here.. :/

In D19622#439705, @jtl wrote:

In the interests of avoiding discussion fragmentation, I am adding the feedback from the transport working group meeting today.

@hselasky raised this on the working group call and solicited feedback. The strong (seemingly-unanimous) feedback on the call was that we should not discard fragments when the associated interface goes away. Some points that were raised include:

• Systems which have multiple interfaces can receive different fragments over different interfaces. Therefore, the other fragments may still arrive over other interfaces.
• Systems with flapping interfaces may receive fragments after the interface recovers.
• It is not unheard of for systems to queue fragments for multiple seconds. Therefore, it is possible that the fragments will arrive once connectivity is restored and/or rerouted.

It was the strong feeling of the participants on the call that this problem should be solved in a way which preserves the fragments on the queue. Some suggestions included:

• At the time of enqueueing, copying the information from the ifnet pointer which is necessary to process the packet. Save this information, rather than the ifnet pointer.
• When an interface goes away, clear the ifnet pointer from the fragments on the queue. Skip processing steps which depend on this information (which @hselasky described as incrementing interface statistics and sending an error response when a fragment is dropped).

In a later call I think your suggestion was along the lines of:

(a) if ifnet goes away nuke the recvif pointer from the queued mbufs
(b) if reassembly times out do as outlined above and skip sending ICMP error/per-IF statistics/.. if the ifnet pointer was nuked
(c) if another fragment arrives (or the last fragment to complete the packet arrives) use that ones recvif pointer as that interface is expected to still be there to pass the packet on

Now there is a gray area between (b) and (c) in which (b) could be extended to "if we cannot find an ifnet pointer in the expected place, scan the fragments of the packet in question for any ifnet pointer and use that one" for error handling. It'd be a one-time slightly more expensive operation. If there's an attack however that is kind-of the extra work you'd want to avoid. Without the extra work, you may not find out as easily what kind of problem you are running into though as you are lacking statistics. Trade-off...

In D19622#476255, @bz wrote:

In a later call I think your suggestion was along the lines of:

(a) if ifnet goes away nuke the recvif pointer from the queued mbufs
(b) if reassembly times out do as outlined above and skip sending ICMP error/per-IF statistics/.. if the ifnet pointer was nuked
(c) if another fragment arrives (or the last fragment to complete the packet arrives) use that ones recvif pointer as that interface is expected to still be there to pass the packet on

Now there is a gray area between (b) and (c) in which (b) could be extended to "if we cannot find an ifnet pointer in the expected place, scan the fragments of the packet in question for any ifnet pointer and use that one" for error handling. It'd be a one-time slightly more expensive operation. If there's an attack however that is kind-of the extra work you'd want to avoid. Without the extra work, you may not find out as easily what kind of problem you are running into though as you are lacking statistics. Trade-off...

There was no grey area in my mind. :-) My proposal was (and still is) to skip anything ifnet-dependent if the reassembly times out and we don't have a pointer to the ifnet.

I agree with the rest of your statement, however. I do recognize the trade-off you have noted. And, while I have a preference for foregoing the extra work, I'm OK with resolving this either way.

@bz If you don't need to send any ICMP errors on timeout, then the rcvifp could just be cleared when packets entering the ICMP hash. That would simplify the code.

I've added prints in the new code paths and verified that:

1. fragment loss does not cause a panic after 30 seconds (ifp is really cleared)
2. creating and destroying a jail using vnet does not panic either (NULL check added causes a return)

All tests succeeded at my side.