Page MenuHomeFreeBSD

libalias: Possible packet_limit underrun
AbandonedPublic

Authored by donner on Jul 10 2021, 12:51 PM.

Details

Reviewers
se
Group Reviewers
network
Summary

On low traffic boxes, the packet_limit can become zero (if less than
three packets per seconds are processed), which results in a division
by zero panic.

The obvious solution is to check for this case explicitly. Thankfully
@se already applied this hotfix to the code. OTOH such a check adds to
the instruction count of each packet processing, which is bad for high
traffic boxes.

So the proposed solution is to add an arbitrary fixed value to the
calculation. There is no risk in delaying the housekeeping, because
the necessary expiry checks are always done before using an entry. The
only effect is, that some old entries stay somewhat longer in the data
structure.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 40430
Build 37319: arc lint + arc unit

Event Timeline

sys/netinet/libalias/alias_db.c
67
1774
1780
1782

I'd use a different approach, since the suggested one does not work well for very low packet rates.

u_long seconds = (u_long)now - (u_long)LibAliasTime;

if (seconds != 0) {
	/* retry three times a second */
	packet_limit = packets / (3 * seconds) + 1;
	packets = 0;
	LibAliasTime = now;
}

For the (pathological, but possible) case of a constant rate of 1 packet per second, the packet_limit would be 10, leading to 10 seconds before the modulo calculation results in a value of 0.
That would lead to packet_limit = 10 / 3 + 10 = 13 seconds until the condition becomes true again.

BEWARE: LibAliasTime is currently of type int, not time_t, and will not be able to hold the value of "now" after 2038, and thus "now != LibAliasTime" will then be always true ...

The obvious solution is to check for this case explicitly. Thankfully
@se already applied this hotfix to the code. OTOH such a check adds to
the instruction count of each packet processing, which is bad for high
traffic boxes.

The hot fix added a comparison of the modulo operand (already in a register) with a small constant and I'd be surprised if it added a measurable delay on any modern processor.
The implicit subtraction that is performed for this comparison will be performed in parallel to the modulo on modern multi-issue CPUs (with speculative execution of the modulo and no trap taken if packet_limit is 0).
But adding a small constant in a code path only taken a few times per second will obviously be very cheap, too.
Since the recalculation of packet_limit is only performed a few times per second, a slightly more complex (but more correct) calculation of that value does also cause only negligible extra CPU cycles.

The current situation is good enough.