Subject: Re: CVS commit: syssrc
To: enami tsugutomo <enami@but-b.or.jp>
From: None <itojun@iijlab.net>
List: source-changes
Date: 10/09/2000 14:08:08
>> >Log Message:
>> >- Keep track of allhost multicast address record we joined into
>> >  each in_ifaddr and delete it when an address is purged.
>> 
>> 	maybe the above commit changed the goal slightly.
>> 	the goal of multicast kludge table (which i ported from
>> 	sys/netinet6/in6.c) was to keep multicast group information ONLY,
>> 	when all the interface address is gone.  it was not my intention
>> 	to avoid the removal of the last IPv4 interface address.
>> 	could you tell me why is the change?
>
>Hmm, I guess you're misunderstanding my change or the comment I wrote
>was miss the point.  I didn't change ``to avoid the removal of the
>last IPv4 interface address.''
>
>The change this comment refer is:
>
>	- Add ia_allhost to in_ifaddr.
>	- Remember the value in_addmulti returns in it.  Note that
>	  same in_multi may be shared.
>	- Call in_delmulti for it when address is deleted.
>
>I just used the last in_ifaddr for the chain head of in_multi.  And we
>have to keep track of the value retrun by in_addmulti for the reason I
>explain below.
>
>> >- Don't simply try to delete a multicast address record listed in the
>> >  ia_multiaddrs.  It results a dangling pointer.  Let who holds a
>> >  reference to it to delete it.
>> 
>> 	this refers to in_purgemkludge(), correct?  i experimented this
>> 	with IPv6 case and IPv4 case, and don't understand why we have
>> 	dangling pointer.   i assumed that in_purgemkludge() is called
>> 	only after we flushed all interface addresses.
>
>It refers the both loops in the patch like this:
>	for (inm = LIST_FIRST(&ia->ia_multiaddrs); ....)
>		in_delmulti(inm);
>
>As I wrote in the other mail, since the struct in_multi is reference
>counted, this is insufficient.  Of course, the reference in
>ia_multiadrs isn't counted, since the counter is for the caller who
>called in_addmulti().  It is refered from a socket and now from a
>in_ifaddr.  The in_multi refered from a socket is used when the socket
>is closed.  So, we have to remove the reference when interface is
>gone.  Without it, we gets panic after a program who uses multicast
>close a socket.  And, it is done in in_pcbpurgeif().
>
>For example, in_purgeif() and in_pcbpurgeif() are called with
>following order.
>
>	in_purgeif(...)
>	in_pcbpurgeif(&udbtable, ...)
>	in_purgeif(...)
>	in_pcbpurgeif(&tcbtable, ...)
>	in_purgeif(...)
>	in_pcbpurgeif(&rawcbtable, ...)
>
>So, for the in_multi refered from socket, calling in_delmulti() for it
>while in in_purgeif() is wrong.  And it should be unnecessary if all
>references are kept.  To keep all reference, I've add ia_allhosts to
>in_ifaddr.
>
>There was also a bug that if an interface is removed while in_multi
>was kept in kludge table, in_delmulti() will dereference the null
>pointer.

	i still see couple of issues around here.

	the problem with the current code is that, item in in_mk will never
	be freed if we have removed the interface (pcmcia).

	suppose we have removed a pcmcia interface, and an interface address
	is put into in_mk.  now, we will never be able to recover the in_ifaddr
	(it will stay forever in in_mk), as in_restoremkluge() will never be
	able to recover it (we check by "ia->ia_ifp == ifp", and ifp is gone).

	if we maintain in_multi in multicast kludge table, and we nuke
	in_ifaddr in in_savemkludge() (like before), we do not need to keep
	in_ifaddr forever in in_mk.
	whenever we kill the last reference to in_multi (maybe from pcb -
	reference counted) in in_delmulti, in_multi will go away.
	we can nuke a "multi_kludge" entry in in_mk chain, whenever the last
	in_multi is gone.

	-> when to leave from all-nodes multicast address?
	   it is enough to leave it when the last in_addr is gone (in_purgeaddr)
	-> are there any better way to recover multicast group from in_mk?
	   this is also related to "what should happen to the multicast groups
	   joined, if interface is removed".  if we have a better way than
	   "ia->ia_ifp == ifp", the problem is solved.  but anyway, the past
	   code is easier for memory cleanup.

	so, i'd propose to go back to the last code, with all-node multicast
	address handling fixed.  opinions?

itojun