Pardon my latency...
On Thu, May 18, 2006 at 02:36:05PM -0400, James Carlson wrote:
> Dan McDonald writes:
> > if (v4 != NULL) {
> > if (!(v4->ill_flags & ILL_CONDEMNED)) {
> > ------- YIKE! ----> tun =
> > (tun_t *)v4->ill_wq->q_next->q_ptr;
>
> Yikes indeed. It assumes that q_next here points to an instance of
> 'tun'. Given the existence of _I_INSERT, I don't see how you can
> guarantee that q_next is stable, let alone that it points to the
> instance expected. One user-pushed module, and it's game over.
So I have to be careful to find "tun". That's okay by me.
> The queue in ill_wq should be valid as long as the ill_t is around.
> The actual contents of that pointed-to structure doesn't have the same
> guarantee.
All ops I do are while I hold a reference to ill. In my original code
fragment, I don't ill_refrele() until I'm finished.
> You also need to be really careful about the references held to make
> sure there are no cycles here. You're taking a reference on tun_t.
Actually, I'm taking a reference in "itp", which is a structure that tun_t
reference holds.
> If tearing down tun_t (which will happen when the ill_t is unplumbed)
> requires that this module holding the reference (whatever it's doing)
> goes away, then you'll deadlock on unplumb.
Since I only hold the itp, and unplumbing does a ITP_REFRELE() (you'll see
when Tunnel Reform comes up for Code Preview and later Code Review), I'm in
good shape on the deadlock front.
Thanks for the warning - I'll be sure to turn the "YIKE" line into:
tun = carefully_find_tun(v4);
where carefully_find_tun():
- Is safe because "v4" is reference-held.
- Will have a number-of-modules-traversed limit.
- Will insure that it finds "tun", as opposed to some other module.
Thanks for the tip,
Dan
p.s. This all goes away when Clearview IP Tunnels happen, but Tunnel Reform
is our last big missing piece of IPsec, so we need to get it out
independently of Clearview.
_______________________________________________
networking-discuss mailing list
[email protected]
