DragonFly submit List (threaded) for 2008-05
[
Date Prev][
Date Next]
[
Thread Prev][
Thread Next]
[
Date Index][
Thread Index]
Re: in_ifinit() fix for SIOCSIFADDR
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.
>
> :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
[
Date Prev][
Date Next]
[
Thread Prev][
Thread Next]
[
Date Index][
Thread Index]