freebsd_network: Check for c_due > 0 in the callout invocation.

Here is the scenario in which this matters:

1. A callout comes due, it is removed from the list, and c_due is set
   to 0. We then wind up in the mtx_lock here inside invoke_callout;
   while some other thread holds the lock.

2. The other thread, holding the lock, decides to *re*schedule the callout
   for a later time than the present. callout_reset sees the callout has
   a c_due of 0, thus indicating it is not in the sTimers list, adds it,
   and sets a (future) c_due.

3. The other thread unlocks c_mtx, thus the callout thread acquires it
   and proceeds.

Under the code I wrote in hrev56875, before this commit, the following
would occur (at least with the ipro1000 driver on some hardware):

4. c_due would be set to -1, thus signaling the callout was no longer
   scheduled, and thus not in the sTimers list (despite being in it!).
   The callout's own method would decide to reschedule itself, and
   invoke callout_reset as such.

5. callout_reset would, seeing c_due of <= 0, try to add the callout to
   the sTimers list. However it is already in the list, and in many
   circumstances the only item in the list, so it would get silently
   linked to itself.

6. An infinite loop due to the looped linked-list and/or double-remove
   resulting in NULL dereferences would result.

Steps 1-2 coinciding in just the right way was apparently very rare, hence
why this problem only appeared very infrequently. The code I wrote to force
their coinciding made the problem happen extremely frequently.

This fixes #18334.

(Figuring out that the problem was an item being linked to itself, and
most critically a double-*reschedule* not a double-*removal*, took
far too long to figure out. I will be refactoring this code more
in subsequent commits, but also introducing new assertions to our
linked-list systems which enabled me to finally track down this problem
at all. Who knows; perhaps they will shake loose other bugs.)
This commit is contained in:
Augustin Cavalier
2023-03-31 23:56:24 -04:00
parent 1110e6fc65
commit f863f473b5
+1 -1
View File
@@ -39,7 +39,7 @@ invoke_callout(callout *c, struct mtx *c_mtx)
if (c_mtx != NULL) {
mtx_lock(c_mtx);
if (c->c_due < 0) {
if (c->c_due < 0 || c->c_due > 0) {
mtx_unlock(c_mtx);
return;
}