Whenever ifindex_alloc() returns USHRT_MAX the old pointer must be set else if_alloc() may free bogus value on the stack causing various kinds of failures in addition to going into an infinite loop. This change is a minor refactoring of the ifindex management. Use a single SX lock to protect the interface pointer array. Because this code is not in the fast path prefer simplicity over complexity. Some minor adaptions for epoch(9) usage.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 20337
Event Timeline
| sys/net/if.c | ||
|---|---|---|
| 577 | I suggest adding a printf() there stating the cause of failure at least in dmesg. | |
| sys/net/if.c | ||
|---|---|---|
| 1310 | This means that the thread never leaves the kernel and is not interruptible, am I wrong ? previous version of the patch did not looped indefinitely. | |
| sys/net/if.c | ||
|---|---|---|
| 1310 | Yes, that is correct, until there is a network device available. Is panicing better here? | |
| sys/net/if.c | ||
|---|---|---|
| 1310 | It would be better to place some limit on the loop then fail. | |
Comment Actions
Silly question; can you explain that test case? How do 255 multicast addresses assigned to 255 interfaces make you run out of ifindex space?
But more importantly I was wondering why this was epoch()ified in first place; if_grow() is a 8 times in an uptime operation if my memory serves me right. Taking a lock for that and waiting a few ms longer won't slow you down much more than a timer tick does during packet processing. Just seems weird to me.
Comment Actions
Silly question; can you explain that test case? How do 255 multicast addresses assigned to 255 interfaces make you run out of ifindex space?
The code was hung for other reasons in epoch_wait_preempt() due to an EPOCH LOR, and you're right that the test doesn't hang with condition idx == USHRT_MAX. This was my first shot. Then I realized the code was a bit broken in other ways too.
| sys/net/if.c | ||
|---|---|---|
| 1310 | Sure, I'll try to extend the patch. | |
Comment Actions
lo(4) or disc(4) might be easier. And you don't need addresses on then; And you don't need 64k though that's good to test the edge case as well for the newly discovered problem, for the original bumping over the next power-of-2 would have called if_grow() and given you a test.
I am still wondering if simply reverting to classic locking for this might make a lot more sense?
Comment Actions
What's the reason we can't just use the IFNET_WLOCK() and be good with it and not rewriting the entire code but going back to what it was?
The original version if_grow() could not fail, ifindex_alloc() had at best one retry, ... I am still not getting why this wasn't good enough?
Comment Actions
@bz: Answering your one question off the bat: IF_WLOCK() doesn't allow sleeping I.E. malloc() with M_WAITOK. That was one point for this change to allow all operation under one lock, instead of unlocking and then trying to catch races and goto retry and so on.
I'll come back to your second question tomorrow.
Comment Actions
@hselasky this is getting super complicated - how much can we avoid by replacing the epoch_wait()+free() with an epoch_call()?
Comment Actions
@mmacy: The problem in this patch is that the network device table is growing dynamically and that we are replacing it as we need more ifunits. Given this code is called pretty rarely, I don't see any point using epoch_call() to free the old structure. Using epoch_call() does not get rid of the grow SX-lock.