in_ifinit() fix for SIOCSIFADDR

[Sepherosa Ziehau]

On Sun, May 25, 2008 at 1:42 AM, Matthew Dillon
<dillon at 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.

>
> :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.

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