On Tue, Aug 27, 2013 at 01:39:14PM +0200, Martin Pieuchot wrote:
> 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.
We should not do that since this was done for tun(4) switching between
modes without getting new ifindexes. It also reduces the holes in the
ifindex array on system that dynamically allocate interfaces for e.g.
VPNs.
> 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;
>
--
:wq Claudio
