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