Page MenuHomeFreeBSD

ip_forward(): Fix a possible next-hop refcount leak.
ClosedPublic

Authored by markj on Jun 30 2020, 3:36 PM.

Details

Summary

This is based on some code review for PR 246951. I suspect that we
could move the lookup to after the IPSec check, but I would like a
minimal patch that I can backport to stable branches (there is a ia ref
leak there too).

I looked through other uses of NHR_REF but didn't find any other obvious
leaks.

I am also a bit uncertain about nhop_ref_object(). It uses
refcount_acquire_if_not_zero(), but most callers don't check the return
value, presumably because it's assumed that it can never fail in those
cases. Should we assert that?

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Jun 30 2020, 3:36 PM

Re nhop_ref_object(): indeed, in many cases we acquire refcount under radix read or write lock, which implies that some routes are pointing to that nhop -> it has non-zero count.
In some corner cases we want to do it without holding radix lock and in that scenario we need to check whether we were able to acquire reference or not.

IT actually should be in the manpage(s), which I haven't written yet :-(

This revision is now accepted and ready to land.Jun 30 2020, 9:44 PM

Re nhop_ref_object(): indeed, in many cases we acquire refcount under radix read or write lock, which implies that some routes are pointing to that nhop -> it has non-zero count.
In some corner cases we want to do it without holding radix lock and in that scenario we need to check whether we were able to acquire reference or not.

It might be preferable to add a separate function for those cases where we know it can't fail, since in that case we can use the cheaper refcount_acquire() (which does fetchadd instead of cmpset).

It might be preferable to add a separate function for those cases where we know it can't fail, since in that case we can use the cheaper refcount_acquire() (which does fetchadd instead of cmpset).

https://reviews.freebsd.org/D25535