Page MenuHomeFreeBSD

ifnet: Defer detaching address family dependent data
AcceptedPublic

Authored by zlei on Fri, Mar 21, 3:12 PM.

Details

Reviewers
glebius
markj
Group Reviewers
network
Summary

While diagnosing PR 279653 and PR 285129, I observed that thread may
write to freed memory but the system do not crash. This hides the real
problem. A clear NULL pointer derefence is much better than writing to
freed memory.

PR: 279653
PR: 285129
MFC after: 2 weeks

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

sys/net/if.c
1022

according to the debug core I have both if_afdata_initialized and all of if_afdata was zeroed, see https://reviews.freebsd.org/D49212#1122671

sys/net/if.c
1022

according to the debug core I have both if_afdata_initialized and all of if_afdata was zeroed, see https://reviews.freebsd.org/D49212#1122671

It is also possible the writes from detaching thread is not global visible to other threads without proper sync. Then other threads may see stall data.

@markj Can the crash dump shows the cache status of different cores ?

sys/net/if.c
1022

@markj Can the crash dump shows the cache status of different cores ?

Nope crash dump is the status of the RAM, we can only guess what was there in the caches of CPUs. We have a patch at Netflix that enables "last branch record" feature of modern Intel CPUs and you can read what were the most recent branching steps of a core before crash. This helps to deduct what a CPU have had in its cache. Hopefully the patch will get upstreamed soon.

This seems right to me wrt the usage of epoch(9). But I didn't do a due analysis of the review in a larger scope. If it improves stability, I am fine with that in general. This also can be a great MFC candidate.

However, I have a plan that would eliminate both the domain(9) ifattach/detach and the mostly empty af_data array that hangs from an ifnet. That change, once done, won't be MFCed.

If you will save the stress tests that you use to test your changes for stability, that actually would be a bigger value for FreeBSD CURRENT than the change itself. Please share them with Peter Holm, too.

This revision is now accepted and ready to land.Sat, Mar 22, 6:28 AM