in_ifinit() fix for SIOCSIFADDR

Sepherosa Ziehau sepherosa at
Sat May 24 20:31:05 PDT 2008

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

Live Free or Die

More information about the Submit mailing list