[erlang-patches] [PATCH] Fix SCTP multihoming for IPv6
Raimo Niskanen
raimo+erlang-patches@REDACTED
Tue Apr 24 16:11:07 CEST 2012
On Mon, Apr 23, 2012 at 11:21:08PM +0200, Tomas Abrahamsson wrote:
> On Mon, Apr 23, 2012 at 16:16 +02:00, Raimo Niskanen wrote:
> > Is it so that the sctp_bindx argument 2 declared as (struct sockaddr *)
> > is in fact misused to be able to contain IPv6 addresses by packing
> > them unaligned?
> >
> > If so, I would like this fact more visible in the code by defining 'addrs' as:
> > char addrs[256 * sizeof(struct inet_address)]
> >
> > If not, the code looks erroneous (but since there are test cases in the patch
> > I would guess the code is right and it is the sctp_bindx call that is weird.
> >
> > I would have expected argument 2 to be (struct sockaddr_storage *) but on
> > my Linux it is not so...
>
> I think I see what you mean, but it seems the 2nd arg
> is supposed to be either an array of sockaddr_in or
> an array of sockaddr_in6, not the union of them:
>
> addrs is a pointer to an array of one or more socket
> addresses. Each address is contained in its appropriate
> structure(i.e. struct sockaddr_in or struct
> sockaddr_in6).
>
> (quoted from the Linux man page, the Solaris
> man page reads similarly)
But before that the Linux man page says:
If sd is an IPv4 socket, the addresses passed must be IPv4 addresses.
If sd is an IPv6 socket, the addresses passed can be either IPv4 or
IPv6 addresses.
So it might be possible to mix them... And if that is allowed the only way
I can see is packing them in a char array.
If it is not allowed to pack them in a char array, the sctp_bindx call
should be able to detect mixed addresses immediately on the first
differing address.
>
> Looking at it now again, I first thought that addrs should be
> either sockaddr_in or sockaddr_in6, not inet_address,
> which is the union, but then I re-read the inet_set_address
> function, which seems to do just what the man page for
> sctp_bindx requires, if I read it correctly.
>
> So I declared the addrs to be of type inet_address to be able
> to use the inet_set_address function, and the casting to
> (struct sockaddr *) was simply to avoid a compiler warning.
>
> Btw, I did develop the test first, and it failed until
> I changed inet_drv.c, so I think it actually works as intended.
>
> Please, let me know if there's anything that I need to do
> to get the patch accepted.
Well I smell an issue if you manage to mix IPv4 and IPv6 addresses...
1) So, testcase that mix IPv4 and IPv6 addresses. Note that especially
FreeBSD in general does not allow using IPv4 on IPv6 sockets so what
is right might differ. Also test IPv4 addresses on IPv6 sockets and
vice versa. I know that was not tested before but since you now enable
IPv6 multihoming there has become more to test...
2) Handling in the code if necessary; I do not know if some upper layer
protects inet_drv from mixed addresses to SCTP_REQ_BINDX...
Failing on mixed addresses if mixed addresses are not allowed,
either by checking or by relying on the sctp_bindx call to fail.
If the OS allows mixing addresses so should Erlang.
3) A code change that reflects the duality of 'addrs', either by as I just
suggested above char addrs[256 * sizeof(struct inet_address)],
or I in the case of mixed addresses not being allowed I thought about
a union addr { struct sockaddr sa[256];
struct sockaddr_in si[256;
struct sockaddr_in6 si6[256]; }
but that would become really awkward except for the call to sctp_bindx
that simply could us addr.sa for argument 2.
>
> BRs
> Tomas
>
> > On Fri, Dec 30, 2011 at 09:10:58PM +0100, Tomas Abrahamsson wrote:
> >> Setting several ip addresses for an SCTP socket worked for IPv4, but
> >> for IPv6 the call to gen_sctp:open/1 failed with badarg.
> >> ---
> >> erts/emulator/drivers/common/inet_drv.c | 12 +++---
> >> lib/kernel/test/gen_sctp_SUITE.erl | 64 ++++++++++++++++++++++++++++++-
> >> 2 files changed, 68 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/erts/emulator/drivers/common/inet_drv.c b/erts/emulator/drivers/common/inet_drv.c
> >> index eeaa4d2..f712653 100644
> >> --- a/erts/emulator/drivers/common/inet_drv.c
> >> +++ b/erts/emulator/drivers/common/inet_drv.c
> >> @@ -10111,8 +10111,9 @@ static ErlDrvSSizeT packet_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf,
> >> /* Construct the list of addresses we bind to. The curr limit is
> >> 256 addrs. Buff structure: Flags(1), ListItem,...:
> >> */
> >> - struct sockaddr addrs[256];
> >> + inet_address addrs[256];
> >> char* curr;
> >> + char* addr_dest;
> >> int add_flag, n, rflag;
> >>
> >> if (!IS_SCTP(desc))
> >> @@ -10122,6 +10123,7 @@ static ErlDrvSSizeT packet_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf,
> >> add_flag = get_int8(curr);
> >> curr++;
> >>
> >> + addr_dest = (char *)addrs;
> >> for(n=0; n < 256 && curr < buf+len; n++)
> >> {
> >> /* List item format: Port(2), IP(4|16) -- compatible with
> >> @@ -10132,16 +10134,14 @@ static ErlDrvSSizeT packet_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf,
> >> if (curr == NULL)
> >> return ctl_error(EINVAL, rbuf, rsize);
> >>
> >> - /* Now: we need to squeeze "tmp" into the size of "sockaddr",
> >> - which is smaller than "tmp" for IPv6 (extra IN6 info will
> >> - be cut off): */
> >> - memcpy(addrs + n, &tmp, sizeof(struct sockaddr));
> >> + memcpy(addr_dest, &tmp, alen);
> >> + addr_dest += alen;
> >> }
> >> /* Make the real flags: */
> >> rflag = add_flag ? SCTP_BINDX_ADD_ADDR : SCTP_BINDX_REM_ADDR;
> >>
> >> /* Invoke the call: */
> >> - if (p_sctp_bindx(desc->s, addrs, n, rflag) < 0)
> >> + if (p_sctp_bindx(desc->s, (struct sockaddr *)addrs, n, rflag) < 0)
> >> return ctl_error(sock_errno(), rbuf, rsize);
> >>
> >> desc->state = INET_STATE_BOUND;
> >> diff --git a/lib/kernel/test/gen_sctp_SUITE.erl b/lib/kernel/test/gen_sctp_SUITE.erl
> >> index 8f490b6..6d6b154 100644
> >> --- a/lib/kernel/test/gen_sctp_SUITE.erl
> >> +++ b/lib/kernel/test/gen_sctp_SUITE.erl
> >> @@ -31,14 +31,16 @@
> >> [basic/1,
> >> api_open_close/1,api_listen/1,api_connect_init/1,api_opts/1,
> >> xfer_min/1,xfer_active/1,def_sndrcvinfo/1,implicit_inet6/1,
> >> - basic_stream/1, xfer_stream_min/1, peeloff/1, buffers/1]).
> >> + basic_stream/1, xfer_stream_min/1, peeloff/1, buffers/1,
> >> + open_multihoming_ipv4_socket/1, open_multihoming_ipv6_socket/1]).
> >>
> >> suite() -> [{ct_hooks,[ts_install_cth]}].
> >>
> >> all() ->
> >> [basic, api_open_close, api_listen, api_connect_init,
> >> api_opts, xfer_min, xfer_active, def_sndrcvinfo, implicit_inet6,
> >> - basic_stream, xfer_stream_min, peeloff, buffers].
> >> + basic_stream, xfer_stream_min, peeloff, buffers,
> >> + open_multihoming_ipv4_socket, open_multihoming_ipv6_socket].
> >>
> >> groups() ->
> >> [].
> >> @@ -1329,3 +1331,61 @@ match_unless_solaris(A, B) ->
> >> {unix,sunos} -> B;
> >> _ -> A = B
> >> end.
> >> +
> >> +open_multihoming_ipv4_socket(doc) ->
> >> + "Test opening a multihoming ipv4 socket";
> >> +open_multihoming_ipv4_socket(suite) ->
> >> + [];
> >> +open_multihoming_ipv4_socket(Config) when is_list(Config) ->
> >> + ?line Hostname = ok(inet:gethostname()),
> >> + ?line HostAddrIPv4 = ok(inet:getaddr(Hostname, inet)),
> >> + ?line S = ok(gen_sctp:open(0, [{ip,HostAddrIPv4},{ip,{127,0,0,1}}])),
> >> + ?line ok = gen_sctp:listen(S, true),
> >> + ?line setup_connection(S, HostAddrIPv4, inet),
> >> + ?line ok = gen_sctp:close(S).
> >> +
> >> +open_multihoming_ipv6_socket(doc) ->
> >> + "Test opening a multihoming ipv6 socket";
> >> +open_multihoming_ipv6_socket(suite) ->
> >> + [];
> >> +open_multihoming_ipv6_socket(Config) when is_list(Config) ->
> >> + ?line Hostname = ok(inet:gethostname()),
> >> + ?line LoopbackIPv6 = {0,0,0,0,0,0,0,1},
> >> + ?line
> >> + case inet:getaddr(Hostname, inet6) of
> >> + {ok,HostAddrIPv6} when HostAddrIPv6 =/= LoopbackIPv6 ->
> >> + ?line
> >> + case is_good_ipv6_address(HostAddrIPv6) of
> >> + true ->
> >> + ?line io:format("using ipv6 addresses ~p and ~p~n",
> >> + [HostAddrIPv6, LoopbackIPv6]),
> >> + ?line S = ok(gen_sctp:open(
> >> + 0, [{ip,HostAddrIPv6},
> >> + {ip,LoopbackIPv6}, inet6])),
> >> + ?line ok = gen_sctp:listen(S, true),
> >> + ?line setup_connection(S, HostAddrIPv6, inet6),
> >> + ?line ok = gen_sctp:close(S);
> >> + false ->
> >> + {skip,"Need 2 good IPv6 addresses"}
> >> + end;
> >> + {ok,HostAddrIPv6} when HostAddrIPv6 =:= LoopbackIPv6 ->
> >> + {skip,"Need 2 different IPv6 addresses, found only ::1"};
> >> + {error,eafnosupport} ->
> >> + {skip,"Can not look up IPv6 address"}
> >> + end.
> >> +
> >> +is_good_ipv6_address({0,0,0,0,0,16#ffff,_,_}) -> false; %% ipv4 mapped
> >> +is_good_ipv6_address({16#fe80,_,_,_,_,_,_,_}) -> false; %% link-local
> >> +is_good_ipv6_address(_) -> true.
> >> +
> >> +setup_connection(S1, Addr, IpFamily) ->
> >> + ?line P1 = ok(inet:port(S1)),
> >> + ?line S2 = ok(gen_sctp:open(0, [IpFamily])),
> >> + ?line P2 = ok(inet:port(S2)),
> >> + ?line #sctp_assoc_change{state=comm_up} =
> >> + ok(gen_sctp:connect(S2, Addr, P1, [])),
> >> + ?line case ok(gen_sctp:recv(S1)) of
> >> + {Addr,P2,[],#sctp_assoc_change{state=comm_up}} ->
> >> + ok
> >> + end,
> >> + ?line ok = gen_sctp:close(S2).
> >> --
> >> 1.7.5.4
> >>
> >> _______________________________________________
> >> erlang-patches mailing list
> >> erlang-patches@REDACTED
> >> http://erlang.org/mailman/listinfo/erlang-patches
> >
> > --
> >
> > / Raimo Niskanen, Erlang/OTP, Ericsson AB
> _______________________________________________
> erlang-patches mailing list
> erlang-patches@REDACTED
> http://erlang.org/mailman/listinfo/erlang-patches
--
/ Raimo Niskanen, Erlang/OTP, Ericsson AB
More information about the erlang-patches
mailing list