> > > Changeset: 8533:66762dc10e32 > > > Comments: > > > mcast_restart_timers_thread() may not always run when requested > > > mcast_restart_timers_thread() panics on exit > > > ilm_walker_step() should skip ILM_DELETE ILMs > > > IPMP stress reveals igmp_input() ill_lock deadlock > > > IPMP stress reveals race between ipif_free() and ill_src_ipif > > > assorted comment fixes > > > > This wad deserves some careful review; please see: > > > > http://zhadum.east/ws/clearview/clearview-ipmpdev/webrev.ipfixes.2/ > > ip_multi.c: > > 1950: In the case where ill is an underlying interface, we're calling > ill_ilm_walker_hold() on ill, but not on ilw_ipmp_ill (the IPMP > interface). Can that lead to ilm's on the IPMP interface returned by > the ilm_walker_*() being manipulated or deleted by other threads while > referenced by the caller?
If we're called with an underlying interface, then we will always try to set ilw_ipmp_ill via ipmp_ill_hold_ipmp_ill(). That call *could* fail if e.g. the IPMP ill was changing or somesuch. In that case though, we won't walk to the IPMP ill because ilw_ipmp_ill wil be NULL, and therefore we won't set ilw_walk_ill() to it. In other words, the code is written such that we always acquire holds on all of the ills that we will walk over (but there could be a bug.) > 1642: This function's implementation is a bit twisted given the comment > above it that states that it's only used for IPv4. If that's the case, > then why go through the trouble of converting things to IPv6 addresses > to compare against ilm_v6addr instead of just comparing against > ilm_addr? Anyway, that's not your doing, it was like that before. It does seem more difficult than necessary. I'll change it. > 1966: ilm_walker_step_helper() is only used by ilm_walker_step(), so it > should be static. Perhaps ilm_walker_step_all() would be more > descriptive (just a suggestion). Sure, I'll make those changes. -- meem
