Page MenuHomeFreeBSD

Fix ifa refcount leak in ifa_ifwithnet()
ClosedPublic

Authored by rstone on Feb 16 2021, 4:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 17, 12:10 PM
Unknown Object (File)
Thu, Apr 11, 9:27 AM
Unknown Object (File)
Mar 7 2024, 11:48 PM
Unknown Object (File)
Jan 16 2024, 3:48 AM
Unknown Object (File)
Sep 14 2023, 9:21 PM
Unknown Object (File)
Sep 10 2023, 7:51 AM
Unknown Object (File)
Aug 20 2023, 2:23 AM
Unknown Object (File)
Aug 5 2023, 2:40 PM

Details

Summary

In 4f6c66cc9c75c8, ifa_ifwithnet() was changed to no longer
ifa_ref() the returned ifaddr, and instead the caller was required
to stay in the net_epoch for as long as they wanted the ifaddr
to remain valid. However, this missed the case where an AF_LINK
lookup would call ifaddr_byindex(), which still does ifa_ref()
the ifaddr. This would cause a refcount leak.

Fix this by inlining the relevant parts of ifaddr_byindex() here,
with the ifa_ref() call removed. This also avoids an unnecessary
entry and exit from the net_epoch for this case.

I've audited all in-tree consumers of ifa_ifwithnet() that could
possibly perform an AF_LINK lookup and confirmed that none of them
will expect the ifaddr to have a reference that they need to
release.

MFC after: 2 months
Sponsored by: Dell Inc

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I haven't been able to find a repro scenario for the leak outside of $WORK's repo, but by inspection this fix is obviously correct so I'm reopening this.

melifaro added inline comments.
sys/net/if.c
1964–1971

Q: why moving sdl declaration outside of the block it's used?

1966

This eliminates the last caller of the ifaddr_byindex(). Do we want to retire the function as well?
If not, maybe it's worth changing the function semantic to not reference the returned ifp, to match ifnet_byindex().

This revision is now accepted and ready to land.Apr 27 2021, 8:31 PM
sys/net/if.c
1964–1971

style(9) cleanup while I'm modifying this block anyway

1966

I think that removing the function entirely makes the most sense. I'll send a follow-up review that does this.

sys/net/if.c
1964–1971

Iirc style(9) doesn’t require this anymore

is this waiting for anything?

If @rstone timeouts in a week, I'll take a look at this.

This revision was automatically updated to reflect the committed changes.