DragonFlyBSD

:Hi all,
:
:Following scenario will cause inaddr hash table contains dangling
:reference to 'ia':
:- ifaceX has an AF_INET ia
:- SIOCSIFADDR is used to change address, and new address' hash value
:is different from ia's
:- in in_ifinit()
: o ia is currently in hash bucket B1
:...

Oooh, nice.  I think you found the corruption that has been bugging
    me for a long time.

The patch looks good except for that XXX assumption.  I can't quite
    follow that code sequence.

:Old code will also leave ia in the wrong hash bucket, if the rtinit()
:in in_ifinit() fails, is this an intended behavior?
:

It looks like in_ifinit() assumes that the (ia) is in some hash list
    somewhere no matter what, but it should probably not be in the wrong
    hash table.  I think it would be appropriate to move it to the correct
    hash table for that failure case.

That whole code section around line 749 of in.c, in the original file,
    is very confusing:

if (ifp->if_ioctl &&
            (error = ifp->if_ioctl(ifp, SIOCSIFADDR, (caddr_t)ia, NULL))) {
                lwkt_serialize_exit(ifp->if_serializer);
                /* LIST_REMOVE(ia, ia_hash) is done in in_control */
                ia->ia_addr = oldaddr;
                if (ia->ia_addr.sin_family == AF_INET)
                        LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr
),
                            ia, ia_hash);
                return (error);
        }

I'm not sure what is going on with (ia) there.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

On Sun, May 25, 2008 at 1:42 AM, Matthew Dillon
<dillon@apollo.backplane.com> wrote:

:Hi all,
:
:Following scenario will cause inaddr hash table contains dangling
:reference to 'ia':
:- ifaceX has an AF_INET ia
:- SIOCSIFADDR is used to change address, and new address' hash value
:is different from ia's
:- in in_ifinit()
: o ia is currently in hash bucket B1
:...

Oooh, nice. I think you found the corruption that has been bugging
me for a long time.

The patch looks good except for that XXX assumption. I can't quite
follow that code sequence.

There are only three possible branches in the final switch block that
will reach the end of in_control_internal()
1) cmd is SIOCDIFADDR
2) cmd is SIOCAIFADDR, ia is newly allocated and in_ifinit() fails
3) cmd is SIOCSIFADDR, ia is newly allocated and in_ifinit() fails

After patch, in_ifinit() will remove ia from hash table if in_ifinit()
fails, so for 2) and 3) there is no need to remove ia again

For 1), ia was installed into hash table only if
ia->ia_addr.sin_family == AF_INET. This forms the condition test
before the XXX comment. IMHO, using sin_family to indicate, whether
ia is in hash table or not, is not a good idea, that's why I added XXX
comment. Once I parallelize ia hash table, I will add a flag to
indicate whether ia is in hash table or not.

I mean following code segment in in_ifinit() (~839 in.c rev1.34):
if ((error = rtinit(&ia->ia_ifa, RTM_ADD, flags)) != 0) {
ia->ia_addr = oldaddr;
return (error);
}
ia's address is restored to oldaddr, but ia itself is left in hash
bucket indexed by newaddr's hash value.

Best Regards,
sephe

:After patch, in_ifinit() will remove ia from hash table if in_ifinit()
:fails, so for 2) and 3) there is no need to remove ia again
:
:For 1), ia was installed into hash table only if
:ia->ia_addr.sin_family == AF_INET. This forms the condition test
:before the XXX comment. IMHO, using sin_family to indicate, whether
:ia is in hash table or not, is not a good idea, that's why I added XXX
:comment. Once I parallelize ia hash table, I will add a flag to
:indicate whether ia is in hash table or not.

Ok, then I'm fine with it.  After reading the first part of your
    answer I was going to suggest having a flag.  It's a good idea
    even if the flag state is known, along with adding a few KKASSERT's
    to catch coding mistakes.

:I mean following code segment in in_ifinit() (~839 in.c rev1.34):
: if ((error = rtinit(&ia->ia_ifa, RTM_ADD, flags)) != 0) {
: ia->ia_addr = oldaddr;
: return (error);
: }
:ia's address is restored to oldaddr, but ia itself is left in hash
:bucket indexed by newaddr's hash value.
:
:Best Regards,
:sephe
:
:--
:Live Free or Die

Ok.  Yes, that should definitely be moved to the correct bucket.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

committed