Subject: Revised address selection preference API draft-chakrabarti-ipv6-addrselect-api-04

From: Julien Laganier <[email-masked]>
Date: Fri, 20 Oct 2006 13:20:21 +0200

Folks,

I just resubmitted a revised draft on the address 
selection preference API incorporating comments and 
suggestions we got from reviewers (thanks!) since -03 
went out last year.

Until it shows up in the repository, you find it here:

<http://julien.laganier.free.fr/draft-chakrabarti-ipv6-addrselect-api-04.txt>

Along with a side-by-side HTML diff with previous 
revision:

<http://julien.laganier.free.fr/draft-chakrabarti-ipv6-addrselect-api-04-from-03.html>

The intent is to publish this draft as an Informational 
RFC, now that oustanding issues have been resolved.

Your comments are welcomed.

Thanks.

--julien

--------------------------------------------------------------------
IETF IPv6 working group mailing list
[email-masked]
Administrative Requests: https://www1.ietf.org/mailman/listinfo/ipv6
--------------------------------------------------------------------

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Fri, 20 Oct 2006 15:41:40 +0300

Hello,

Some random comments:

The proposed API adds a new IPv6-level socket option
(IPV6_ADDRESS_PREFERENCES). IMHO, it ought to
also specify that this can also be used as a "non-sticky" option through
sendmsg() ancilliarry data, like other IPv6 options found in the
advanced IPv6 API.

It might be nicer to have the SA/SB DA/DB example for getaddrinfo() in a
subsection of its own mentionning that it is an example. The struct
definition should probably be moved into an appendix as an example
implementation detail. Similarly, the actual specification part of that
chapter deserves to be separated from introductory text and example.

The example getaddrinfo() is flawed. DO NOT let this flow into an RFC...
freeaddrinfo() must be applied to the pointer returned by getaddrinfo(),
i.e. the pointer to the first struct addrinfo in the linked list. Your
code is essentially equivalent to freeaddrinfo(NULL) !

Please specify the interaction between AI_PREFERENCES and AI_PASSIVE -
the current spec seems to assume AI_PASSIVE is not set.

More seriously, there are some design flaws in the proposed
getaddrinfo() extensions:

For a start, there is a race condition if the locally available
addresses or the routing table change between the getaddrinfo()
invocation and connect() calls, we may get unwanted results. Maybe a
solution would be to return ai_bindaddr and ai_connaddr instead of just
ai_addr.

Also, extending the byte size of an already existing structure might
pose some problem: imagine an application initializes a "hint" struct
addrinfo, and passes it to a library API that would tweak it depending
on some configuration or whatever, then the application would use the
same hint as a parameter to getaddrinfo(). Now what if the application
was built with the old-style struct addrinfo without the ai_eflags, but
the library uses it? we risk a buffer overflow there.

IMHO, it might be wiser to define extended getaddrinfo, freeaddrinfo and
struct addrinfo variants (geteaddrinfo ?) - that woud allow to use the
ai_bindaddr and ai_connaddr thing at the same time.

Regards,

-- 
Rémi Denis-Courmont,
looking for a job
http://www.simphalempin.com/home/infos/CV-en.pdf

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Fri, 20 Oct 2006 15:48:29 +0300

Hello again,

For consistency with the current getaddrinfo and setsockopt usage, I
think the preference flags ought to be an "int" rather than an
"uin32_t", and that resetting the socket option to system default should
be achieved by passing "-1" as the socket option value, rather than
using getsockopt() before setsockopt().

Regards,

PS: sorry about the earlier sendmsg() note, I see it's addressed in an
appendix already.

-- 
Rémi Denis-Courmont
looking for a job

---

From: Julien Laganier <[email-masked]>
Date: Mon, 23 Oct 2006 10:35:38 +0200

On Friday 20 October 2006 14:41, Rémi Denis-Courmont 
> 	Hello,

Hi,

Thanks for those; see my response inlined below:

> also specify that this can also be used as a
> "non-sticky" option through sendmsg() ancilliarry
> data, like other IPv6 options found in the advanced
> IPv6 API.

But this is specified (and somehow discouraged) in the 
Appendix: Doing per packet address selection would 
certainly be costly because running the algorithm 
isn't as cheap as setting a field in a packet.

Also, note that there's already setsockopt only and 
ancillary only options (e.g. RFC3493 has 
IPV6_UNICAST_HOPS and IPV6_MULTICAST_HOPS for 
get/setsockopt, while RFC3542 has IPV6_HOPLIMIT only 
for ancillary.) 

> [...]
>
> The struct definition should probably be moved into
> an appendix as an example implementation detail.

I do not think that's an implementation detail. The 
addrinfo struct is part of the core of RFC3493, not of 
the appendix. 

> [...]
>
> The example getaddrinfo() is flawed. DO NOT let this
> flow into an RFC... freeaddrinfo() must be applied
> to the pointer returned by getaddrinfo(), i.e. the
> pointer to the first struct addrinfo in the linked
> list. Your code is essentially equivalent to
> freeaddrinfo(NULL) !

Good catch, the initial addrinfo returned by 
getaddrinfo should be saved for later freeing.

> Please specify the interaction between
> AI_PREFERENCES and AI_PASSIVE - the current spec
> seems to assume AI_PASSIVE is not set.

Setting AI_PASSIVE means you want to bind to a socket 
without actually initiating communications on your 
own. In that case source address selection algorithm 
does not apply since there's no 

> More seriously, there are some design flaws in the
> proposed getaddrinfo() extensions:
>
> For a start, there is a race condition if the
> locally available addresses or the routing table
> change between the getaddrinfo() invocation and
> connect() calls, we may get unwanted results. 

Yes there might be race conditions; that is however not 
specific to the use of getaddrinfo() while expressing 
address selection preferences.

> Maybe a solution would be to return ai_bindaddr and
> ai_connaddr instead of just ai_addr.

One goal of this API is to be preserve the existing 
basic IPv6 socket API.

Besides, I don't understand how would ai_connaddr and 
ai_bindaddr solve the issue you describe.

> Also, extending the byte size of an already existing
> structure might pose some problem: imagine an
> application initializes a "hint" struct addrinfo,
> and passes it to a library API that would tweak it
> depending on some configuration or whatever, then
> the application would use the same hint as a
> parameter to getaddrinfo(). Now what if the
> application was built with the old-style struct
> addrinfo without the ai_eflags, but the library uses
> it? we risk a buffer overflow there.

It should never be assumed (by application, library 
kernel...) that the ai_eflags field is present in a 
struct addrinfo if the struct doesn't have the 
AI_PREFERENCES flag set in its ai_flags field.

Hence a buffer overflow shouldn't happen unless the 
library of you scenario is broken.

> IMHO, it might be wiser to define extended
> getaddrinfo, freeaddrinfo and struct addrinfo
> variants (geteaddrinfo ?) - that woud allow to use
> the ai_bindaddr and ai_connaddr thing at the same
> time.

IMHO it's nice to avoid introducing yet another name 
resolution primitive. Regarding ai_bindaddr and 
ai_connaddr, see my comment above.

Thanks,

--julien

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Mon, 23 Oct 2006 17:28:11 +0300

> isn't as cheap as setting a field in a packet.

It surely is more expensive that a sticky option, but it is definitely 
much less expensive that resetting the sticky option at each send. It's 
ok by me if it's discouraged, as long as it is specified, in case it 
really is needed.

> One goal of this API is to be preserve the existing

Expanding a structure breaks the API already.

> ai_bindaddr solve the issue you describe.

It makes sure the source address that will actually be bound in the one 
that was selected by getaddrinfo().

> AI_PREFERENCES flag set in its ai_flags field.

IMHO, that's an ugly hack. It breaks the ABI.

> library of you scenario is broken.

Sure, but that does not solve my problem.

In fact buffer overflow will happen in even less obvious ways. If some 
function compiled against the new ABI memset() an addrinfo pointer to a 
structure allocated by a function built with the old ABI, you have a 
buffer overflow, and well, you can't test for AI_PREFERENCES, since you 
don't even know it exists. Very bad idea. Changing a struct storage 
size is always a very bad idea. Why do you think people invented struct 
sockaddr_storage instead of increasing the size of struct sockaddr when 
rolling out the IPv6 API?

> ai_connaddr, see my comment above.

I don't see any point in sparing extra primitive and structure names.
I think that it is much nicer than partially breaking the ABI by 
changing the size of structure.

---

From: Julien Laganier <[email-masked]>
Date: Thu, 26 Oct 2006 06:24:18 +0200

Hi,

> For consistency with the current getaddrinfo and
> setsockopt usage, I think the preference flags ought
> to be an "int" rather than an "uin32_t", 

What should we be consistent with? I don't understand 
how that would be more consistent...

A flag set is really no more than a bitstring, each 
bits encoding one flag, I don't see how that can be 
signed... Or did you meant it should be an uint rather 
that uint32_t?

> and that resetting the socket option to system
> default should be achieved by passing "-1" as the
> socket option value, rather than using getsockopt()
> before setsockopt().

I don't think we discussed that already on the ML. 

What we discussed however is to restore system default 
by passing the value zero, and the draft does not use 
that method because it is problematic; quoting Erik:

> The issue is that we want the semantics of a 
> setsockopt that doesn't include any of a X and a 
> not-X pair not to affect the X-related setting. For 
> instance, a setsockopt that only specifies Y (or Y|
> not-Z) shouldn't affect X. With that approach the 
> logical effect of a setsockopt with no flags set 
> (zero) must be a no-op. Thus it would be very odd to 
> define that no flags set (i.e. zero) has some other 
> semantics.        

Now if I consider your proposal of passing -1 as a 
method to restore system defaults, that would mean 
that all bits in the flags set bitstring are set to 1, 
i.e. all possible flags are set. 

I don't think it is semantically clean to affect a 
"restore system default" semantic to a flag set with 
all possible flags set. Therefore it would probably be 
required to reserve that semantic to the sign bit, and 
don't use it for encoding any address preference flag. 

IMHO it's not simpler that what we have today, but 
perhaps I'm missing something.

Best,

--julien

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Thu, 26 Oct 2006 09:13:50 +0300

Hello,

This is all a matter of being consistent with the Advanced IPv6 API, and
with the established practice in POSIX world. Pretty much every
setsockopt uses int, unless it has very peculiar reason not to do so.
Using something else will only confuse people, and add a (minor) extra
implementation burden on implementors.

This is bound to cause problem. I'm pretty confident some people will
use int regardless and well, too bad.

> that method because it is problematic; quoting Erik:

This only works if you assume 0 is the default value.

> i.e. all possible flags are set. 

Though you should never use the sign bit to store any flag anyway, so it
is not at all ambiguous. The hop limit and traffic class setsockopt
already uses this model. When 0 is a legal value, it's not quite correct
to assume that will be the "reset to default" as well

> don't use it for encoding any address preference flag. 

I do think it is semantically clean to use an "impossible" value as
restore to system default, while it is NOT clean to use a "possible"
value for that.

The IETF thought otherwise in may 2003.
Have you had a look at RFC3542 at all? (hint: §6.3 and §6.5)