Plug some ifaddr refcount leaks.
ClosedPublic
Actions

Authored by markj on Dec 23 2019, 7:09 PM.

Details

Reviewers

ae
melifaro

Group Reviewers

network

Commits

rS356107: Plug some ifaddr refcount leaks.

Summary

Don't take a ref in rt_exportinfo(). This was quite wrong in that we took the ref even when NHR_REF was not set.
Don't unconditionally take a ref in rtrequest1_fib(). rt_getifa_fib() will acquire a reference, in which case we would previously acquire two references.
Stop taking a reference in rtinit1() before calling rtrequest1_fib(). rtrequest1_fib() will acquire a reference for the RTM_ADD case.

Test Plan

This fixes a memory leak observed when removing an interface address.

I would appreciate any testing that others can provide.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 28291
Build 26401: arc lint + arc unit

Event Timeline

markj created this revision.Dec 23 2019, 7:09 PM

Herald added a subscriber: ae. · View Herald TranscriptDec 23 2019, 7:09 PM

Harbormaster completed remote builds in B28291: Diff 65930.Dec 23 2019, 7:09 PM

markj edited the test plan for this revision. (Show Details)Dec 23 2019, 7:10 PM

markj added a reviewer: network.

markj added reviewers: ae, melifaro.

LGTM.

Any chance you have some dtrace / kgdb scripts verifying refcounting that can be added to the test suite?

This revision is now accepted and ready to land.Dec 25 2019, 10:07 AM

In D22912#502087, @melifaro wrote:

LGTM.

Any chance you have some dtrace / kgdb scripts verifying refcounting that can be added to the test suite?

I'm afraid I don't. I added some dtrace probes to ifa_ref() and friends to help debug the issue and looked at the vmstat -m stats to verify the change, but it was all ad-hoc.

sys/net/route.c
909	I guess it would be formally correct to take a ref in the NHR_REF case, though no existing callers care. I will do that and make sure we release the reference.