Bug #1019

in_ifinit() fix for SIOCSIFADDR

Added by sepherosa about 6 years ago. Updated almost 6 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

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
o ia is removed from B1 and installed into hash table using new
address hash value, assume its new hash bucket is B2, and B1 != B2
o ifnet.if_ioctl fails
o ia is reinstalled into hash bucket B1, but without being first
removed from hash bucket B2
o hash bucket B2 will have a dangling reference to ia

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

SIOCAIFADDR is not affected.

Please review following patch:
http://leaf.dragonflybsd.org/~sephe/in_ifaddr.diff

Best Regards,
sephe

History

#1 Updated by dillon about 6 years ago

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

#2 Updated by sepherosa about 6 years ago

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

#3 Updated by dillon about 6 years ago

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

#4 Updated by sepherosa almost 6 years ago

committed

Also available in: Atom PDF