Date: Thu, 13 Jul 2000 08:43:33 -0700 From: Julian Elischer <julian@elischer.org> To: "Yevmenkin, Maksim N, CSCIO" <myevmenkin@att.com> Cc: "'freebsd-current@freebsd.org'" <freebsd-current@freebsd.org> Subject: Re: possible NETGRAPH/NG_ETHER bug Message-ID: <396DE3A5.167EB0E7@elischer.org> References: <E598F159668DD311B9C700902799EAF4473459@njb140po01.ems.att.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I object to these patches. the idea is good but these patches are misguided.. Yevmenkin, Maksim N, CSCIO wrote: > > Hello All, > > i was working on integration of Ethernet TAP driver and NETGRAPH > and found strange thing. the problem is that NG_ETHER nodes do not > detach correctly when interface is gone. i was taking a very quick > look at it, and, it seems to me that we are missing one reference > to a node. i think it is ng_name_node/ng_unname pair. This is quite possible because until recently interfaces could never be removed. Therefore the act of removing a node was really just a case of RESETTING the node. It was not removed. > > quick patch (works for NG_ETHER, but i did not have time to look > at all NG_XXXX modules) available at > > http://home.earthlink.net/~evmax/ng_ether-diffs.tar.gz > > this problem could, possibly, affect other modules. > > after patch i was able to: > - load Ethernet TAP driver > - create several virtual Ethernet interfaces > - load NG_ETHER module (and check nodes via ngctl) > - unload Ethernet TAP driver (all virtual Ethernet interfaces are gone) > - check that all NG_ETHER nodes are gone (via ngctl) > - unload NG_ETHER module it's rathe rude to do this in such an un-nested manner, but yes it would be good for this to work. my comments on you patches follow: ng_base.c Wed Jul 12 22:54:09 2000 *************** *** 632,638 **** if (node->name) { FREE(node->name, M_NETGRAPH); node->name = NULL; ! ng_unref(node); } } --- 632,638 ---- if (node->name) { FREE(node->name, M_NETGRAPH); node->name = NULL; ! node->refs--; } } YUK!!!!! never never never decrement refs without checking that is has not gone to 0. If it has gone to 0 the object MUST be freed.!!!!!!! That is what ng_unref is for. If you do not want it to be freed than you should add a reference which represents the reference that you have to it. That way you can guarantee that it will never go to 0. NEVER EVER EVER subvert a reference count. (you go blind if you do that) *** ng_ether.c.old Wed Jul 12 22:53:38 2000 --- ng_ether.c Wed Jul 12 22:55:41 2000 *************** *** 287,297 **** --- 287,299 ---- if (node == NULL) /* no node (why not?), ignore */ return; /* break all links to other nodes */ + node->flags |= NG_INVALID; IFP2NG(ifp) = NULL; /* detach node from interface */ priv = node->private; /* free node private info */ bzero(priv, sizeof(*priv)); FREE(priv, M_NETGRAPH); node->private = NULL; + ng_unname(node); /* unname node */ ng_unref(node); /* free node itself */ } Ok, so the idea is that the actual underlying interface is going away, and that you want the node to go away too. The correct way is to signal to ng_ether_rmnode() that it SHOULD remove the node. At present this code assumes that the ethernet interface is permenent, and that the ng_ether node should thus also be persistant. What you need is a way for it to distinguish between the case where it should not remove the node, and the case where the interface is doomed, and it should remove the node. A flag somewhere would suffice. It seems to me from a quick look at the code that is in the -current tree, that there is some confusion about device node removal as applied to ethernet interfaces. Archie's changes (when he applies them) will give a clearer picture at to how this should be done. I suggest that you hold off until his patches are added because it will have an effect. It should be done any day now. The way you propose to do it will not meet my design criterea for netgraph. -- __--_|\ Julian Elischer / \ julian@elischer.org ( OZ ) World tour 2000 ;_.---._/ presently in: Budapest v To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?396DE3A5.167EB0E7>