On 26/08/13(Mon) 13:36, Mike Belopuhov wrote:
> hi,
>
> in order to make our life a bit easier and prevent rogue
> accesses to the routing table from the hardware interrupt
> context violating all kinds of spl assumptions we would
> like if_link_state_change that is called by network device
> drivers in their interrupt service routines to defer its
> work to the process context or thereabouts.
>
> i did some testing here, but wouldn't mind if someone
> tries this diff in gre/vlan/ospf/anything-weird setups.
> making sure that hot-plugging/unplugging usb interfaces
> doesn't produce any undesirable effects would be superb
> as well.
I just tested vlan and usb interfaces, it works just fine.
> please note that a token (an interface index) is passed
> to the workq in order to make sure that if the interface
> would be gone by the time syswq goes around to run the
> task it would just fall through.
I think that's the right approach but the current code generating
interfaces indexes is too clever from my point of view, it tries
to reuse the last index if possible. This could lead to some
funny races if we detach and reattach an interface before the
task get scheduled.
So I'm ok with your diff if we avoid to reuse the last index too
soon. Diff below does that.
Index: net/if.c
===================================================================
RCS file: /home/ncvs/src/sys/net/if.c,v
retrieving revision 1.262
diff -u -p -r1.262 if.c
--- net/if.c 20 Aug 2013 09:14:22 -0000 1.262
+++ net/if.c 27 Aug 2013 10:22:44 -0000
@@ -204,31 +204,37 @@ if_attachsetup(struct ifnet *ifp)
{
int wrapped = 0;
- if (ifindex2ifnet == 0)
+ /*
+ * Always increment the index to avoid races.
+ */
+ if_index++;
+
+ /*
+ * If we hit USHRT_MAX, we skip back to 1 since there are a
+ * number of places where the value of ifp->if_index or
+ * if_index itself is compared to or stored in an unsigned
+ * short. By jumping back, we won't botch those assignments
+ * or comparisons.
+ */
+ if (if_index == USHRT_MAX) {
if_index = 1;
- else {
- while (if_index < if_indexlim &&
- ifindex2ifnet[if_index] != NULL) {
- if_index++;
+ wrapped++;
+ }
+
+ while (if_index < if_indexlim && ifindex2ifnet[if_index] != NULL) {
+ if_index++;
+
+ if (if_index == USHRT_MAX) {
/*
- * If we hit USHRT_MAX, we skip back to 1 since
- * there are a number of places where the value
- * of ifp->if_index or if_index itself is compared
- * to or stored in an unsigned short. By
- * jumping back, we won't botch those assignments
- * or comparisons.
+ * If we have to jump back to 1 twice without
+ * finding an empty slot then there are too many
+ * interfaces.
*/
- if (if_index == USHRT_MAX) {
- if_index = 1;
- /*
- * However, if we have to jump back to 1
- * *twice* without finding an empty
- * slot in ifindex2ifnet[], then there
- * there are too many (>65535) interfaces.
- */
- if (wrapped++)
- panic("too many interfaces");
- }
+ if (wrapped)
+ panic("too many interfaces");
+
+ if_index = 1;
+ wrapped++;
}
}
ifp->if_index = if_index;
