On Thu, Jul 10, 2014 at 01:58:56PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 10, 2014 at 07:33:22AM -0400, Neil Horman wrote:
> > On Wed, Jul 09, 2014 at 12:51:09PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Jul 09, 2014 at 02:27:02PM -0400, Neil Horman wrote:
> > >
> > > > > Hum, this looks funny without a family assigment, how does the the
> > > > > next thing to look at sk know if is a AF_INET vs AF_INET6?
> > > > >
> > > > Neither clause has a family assignment. Hmm, I wonder if they need one.
> > >
> > > I don't know off hand, I think it depends what happens to 'sk' later
> > >
> > Ah, its filled out in the protocols from_sk method, and ipv6 doesn't curently
> > have one, so we don't need to worry about it (yet).
>
> Hurm.
>
> No, it is just more deeply broken then that..
>
> getpeername() and getsockname() just don't work properly at all with
> V6, it returns garbage with a v4 address becomes involved.
>
> For an AF_INET6 socket the local and peer address should always be
> stored in the inet6_sk as a mapped address, the inet_sk stuff
> shouldn't really be used..
>
> I fixed this in the revised patch by making to_sk_daddr/saddr part of
> the pf and removing all sensitivity on the address af. The conversion
> now uniformly stores any value as AF_INET6.
>
> Instead I get the v4mapped behavior at the new getname entry point,
> which fixes all the problems I could see here.
>
> > Ah, sorry, didn't understand what you were getting at. You will
> > enter the above path if you have an ipv6 socket created and try to
> > bind it to a local ipv4 address. Test that with and without
> > v4mapped set, and you should get complete coverage of that
> > conditional.
>
> No, I am already testing that..
>
> Hurm, there is more that needs fixing..
>
> Testing binding to ::ffff:127.0.0.1 and ::1 (ie both are passed into
> the kernel as AF_INET6) causes a behavior difference based on
> v4mapped, when 0 we get 'sctp_bindx - 99: Cannot assign requested
> address'
>
> 1) Bind should not be sensitive to v4mapped or not, so lets drop this
> test:
>
> @@ -880,9 +872,6 @@ static int sctp_inet6_bind_verify(struct sctp_sock *opt, union sctp_addr *addr)
> return 0;
> }
> rcu_read_unlock();
> - } else if (type == IPV6_ADDR_MAPPED) {
> - if (!opt->v4mapped)
> - return 0;
> }
>
>
> That fixes the bindx failure, and now we hit coverage in
> sctp_v6_available
>
> 2) I added a BUG_ON(addr->v4.sin_family != AF_INET) to
> sctp_v4_available, and now it triggers, as I said, the flow
> changed in the patch. So drop the test for mapped in available, and
> unconditionally map.
>
> @@ -554,12 +564,10 @@ static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp)
>
> type = ipv6_addr_type(in6);
> if (IPV6_ADDR_ANY == type)
> return 1;
> if (type == IPV6_ADDR_MAPPED) {
> - if (sp && !sp->v4mapped)
> - return 0;
> if (sp && ipv6_only_sock(sctp_opt2sk(sp)))
> return 0;
> sctp_v6_map_v4(addr);
> return sctp_get_af_specific(AF_INET)->available(addr, sp);
> }
>
> Note, this makes sense because none of these routines are generating
> addresses to be passed back to user space, so none should be looking
> at mapped.
>
> The attached patch fixes all my test cases, and having looked it very
> carefully now, I'm pretty comfortable with the bulk, what worries me
> is other stuff that might be missed...
>
> I'm a little worried about sctp_addr_id2transport, I'm not sure it
> should be calling addr_v4map, that probably should be an unconditional
> promotion?
>
> I also wonder if sctp_v6_addr_v4map should demote mapped v6 back into
> v4? I suspect yes it should.
>
> From d3cc46c03975688ef144e8ef68ae55a9a391e7ca Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
> Date: Thu, 10 Jul 2014 13:49:03 -0600
> Subject: [PATCH] sctp: Fixup v4mapped behaviour to comply with Sock API
>
> The SCTP socket extensions API document describes the v4mapping option as
> follows:
>
> 8.1.15. Set/Clear IPv4 Mapped Addresses (SCTP_I_WANT_MAPPED_V4_ADDR)
>
> This socket option is a Boolean flag which turns on or off the
> mapping of IPv4 addresses. If this option is turned on, then IPv4
> addresses will be mapped to V6 representation. If this option is
> turned off, then no mapping will be done of V4 addresses and a user
> will receive both PF_INET6 and PF_INET type addresses on the socket.
> See [RFC3542] for more details on mapped V6 addresses.
>
> This description isn't really in line with what the code does though.
>
> Change the handling so that v6 sockets mostly keeps track of things
> internally as mapped v4 addresses and then convert them to AF_INET
> at the syscall boundary.
>
> Tested bind, getpeername, getsockname, connect, and recvmsg for proper
> behaviour in v4mapped = 1 and 0 cases.
>
> Signed-off-by: Neil Horman <nhorman@xxxxxxxxxxxxx>
> Tested-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
> ---
> include/net/sctp/structs.h | 8 ++---
> net/sctp/ipv6.c | 76 ++++++++++++++++++++++++++++++++--------------
> net/sctp/protocol.c | 4 +--
> net/sctp/socket.c | 13 ++++----
> net/sctp/transport.c | 4 +--
> 5 files changed, 67 insertions(+), 38 deletions(-)
>
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index f38588bf3462..a5704e597c9a 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -461,10 +461,6 @@ struct sctp_af {
> int saddr);
> void (*from_sk) (union sctp_addr *,
> struct sock *sk);
> - void (*to_sk_saddr) (union sctp_addr *,
> - struct sock *sk);
> - void (*to_sk_daddr) (union sctp_addr *,
> - struct sock *sk);
> void (*from_addr_param) (union sctp_addr *,
> union sctp_addr_param *,
> __be16 port, int iif);
> @@ -506,6 +502,10 @@ struct sctp_pf {
> struct sock *(*create_accept_sk) (struct sock *sk,
> struct sctp_association *asoc);
> void (*addr_v4map) (struct sctp_sock *, union sctp_addr *);
> + void (*to_sk_saddr) (union sctp_addr *,
> + struct sock *sk);
> + void (*to_sk_daddr) (union sctp_addr *,
> + struct sock *sk);
> struct sctp_af *af;
> };
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 1999592ba88c..88c3a263eb57 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -434,7 +434,7 @@ static void sctp_v6_from_sk(union sctp_addr *addr, struct sock *sk)
> /* Initialize sk->sk_rcv_saddr from sctp_addr. */
> static void sctp_v6_to_sk_saddr(union sctp_addr *addr, struct sock *sk)
> {
> - if (addr->sa.sa_family == AF_INET && sctp_sk(sk)->v4mapped) {
> + if (addr->sa.sa_family == AF_INET) {
> sk->sk_v6_rcv_saddr.s6_addr32[0] = 0;
> sk->sk_v6_rcv_saddr.s6_addr32[1] = 0;
> sk->sk_v6_rcv_saddr.s6_addr32[2] = htonl(0x0000ffff);
> @@ -448,7 +448,7 @@ static void sctp_v6_to_sk_saddr(union sctp_addr *addr, struct sock *sk)
> /* Initialize sk->sk_daddr from sctp_addr. */
> static void sctp_v6_to_sk_daddr(union sctp_addr *addr, struct sock *sk)
> {
> - if (addr->sa.sa_family == AF_INET && sctp_sk(sk)->v4mapped) {
> + if (addr->sa.sa_family == AF_INET) {
> sk->sk_v6_daddr.s6_addr32[0] = 0;
> sk->sk_v6_daddr.s6_addr32[1] = 0;
> sk->sk_v6_daddr.s6_addr32[2] = htonl(0x0000ffff);
> @@ -556,8 +556,6 @@ static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp)
> if (IPV6_ADDR_ANY == type)
> return 1;
> if (type == IPV6_ADDR_MAPPED) {
> - if (sp && !sp->v4mapped)
> - return 0;
> if (sp && ipv6_only_sock(sctp_opt2sk(sp)))
> return 0;
> sctp_v6_map_v4(addr);
> @@ -587,8 +585,6 @@ static int sctp_v6_addr_valid(union sctp_addr *addr,
> /* Note: This routine is used in input, so v4-mapped-v6
> * are disallowed here when there is no sctp_sock.
> */
> - if (!sp || !sp->v4mapped)
> - return 0;
> if (sp && ipv6_only_sock(sctp_opt2sk(sp)))
> return 0;
> sctp_v6_map_v4(addr);
> @@ -723,6 +719,7 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> char *msgname, int *addrlen)
> {
> struct sockaddr_in6 *sin6, *sin6from;
> + struct sockaddr_in *sin;
>
> if (msgname) {
> union sctp_addr *addr;
> @@ -731,6 +728,7 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> asoc = event->asoc;
> sctp_inet6_msgname(msgname, addrlen);
> sin6 = (struct sockaddr_in6 *)msgname;
> + sin = (struct sockaddr_in *)msgname;
> sin6->sin6_port = htons(asoc->peer.port);
> addr = &asoc->peer.primary_addr;
>
> @@ -739,12 +737,17 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> */
>
> /* Map ipv4 address into v4-mapped-on-v6 address. */
> - if (sctp_sk(asoc->base.sk)->v4mapped &&
> - AF_INET == addr->sa.sa_family) {
> - sctp_v4_map_v6((union sctp_addr *)sin6);
> - sin6->sin6_addr.s6_addr32[3] =
> - addr->v4.sin_addr.s_addr;
> - return;
> + if (addr->sa.sa_family == AF_INET) {
> + if (sctp_sk(asoc->base.sk)->v4mapped) {
> + sctp_v4_map_v6((union sctp_addr *)sin6);
> + sin6->sin6_addr.s6_addr32[3] =
> + addr->v4.sin_addr.s_addr;
> + return;
> + } else {
> + sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
> + sin->sin_family = AF_INET;
> + sin->sin_port = htons(asoc->peer.port);
> + }
> }
>
> sin6from = &asoc->peer.primary_addr.v6;
> @@ -760,22 +763,32 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
> {
> struct sctphdr *sh;
> struct sockaddr_in6 *sin6;
> + struct sockaddr_in *sin;
>
> if (msgname) {
> sctp_inet6_msgname(msgname, addr_len);
> sin6 = (struct sockaddr_in6 *)msgname;
> + sin = (struct sockaddr_in *)msgname;
> +
> sh = sctp_hdr(skb);
> - sin6->sin6_port = sh->source;
>
> /* Map ipv4 address into v4-mapped-on-v6 address. */
> - if (sctp_sk(skb->sk)->v4mapped &&
> - ip_hdr(skb)->version == 4) {
> - sctp_v4_map_v6((union sctp_addr *)sin6);
> - sin6->sin6_addr.s6_addr32[3] = ip_hdr(skb)->saddr;
> + if (ip_hdr(skb)->version == 4) {
> + if (sctp_sk(skb->sk)->v4mapped) {
> + sin6->sin6_port = sh->source;
> + sctp_v4_map_v6((union sctp_addr *)sin6);
> + sin6->sin6_addr.s6_addr32[3] =
> + ip_hdr(skb)->saddr;
> + } else {
> + sin->sin_addr.s_addr = ip_hdr(skb)->saddr;
> + sin->sin_family = AF_INET;
> + sin->sin_port = sh->source;
> + }
> return;
> }
>
> /* Otherwise, just copy the v6 address. */
> + sin6->sin6_port = sh->source;
> sin6->sin6_addr = ipv6_hdr(skb)->saddr;
> if (ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL) {
> struct sctp_ulpevent *ev = sctp_skb2event(skb);
> @@ -857,9 +870,6 @@ static int sctp_inet6_bind_verify(struct sctp_sock *opt, union sctp_addr *addr)
> return 0;
> }
> rcu_read_unlock();
> - } else if (type == IPV6_ADDR_MAPPED) {
> - if (!opt->v4mapped)
> - return 0;
> }
>
> af = opt->pf->af;
> @@ -914,6 +924,26 @@ static int sctp_inet6_supported_addrs(const struct sctp_sock *opt,
> return 1;
> }
>
> +/* Handle SCTP_I_WANT_MAPPED_V4_ADDR for getpeername and getsockname() */
> +static int sctp_getname(struct socket *sock, struct sockaddr *uaddr,
> + int *uaddr_len, int peer)
> +{
> + struct sockaddr_in6 *sin = (struct sockaddr_in6 *)uaddr;
> + struct sctp_sock *sp = sctp_sk(sock->sk);
> + int rc;
> +
> + rc = inet6_getname(sock, uaddr, uaddr_len, peer);
> + if (rc != 0)
> + return rc;
> +
> + if (!sp->v4mapped && ipv6_addr_v4mapped(&sin->sin6_addr)) {
> + sctp_v6_map_v4((union sctp_addr *)uaddr);
> + *uaddr_len = sizeof(struct sockaddr_in);
> + }
> +
> + return rc;
> +}
> +
> static const struct proto_ops inet6_seqpacket_ops = {
> .family = PF_INET6,
> .owner = THIS_MODULE,
> @@ -922,7 +952,7 @@ static const struct proto_ops inet6_seqpacket_ops = {
> .connect = inet_dgram_connect,
> .socketpair = sock_no_socketpair,
> .accept = inet_accept,
> - .getname = inet6_getname,
> + .getname = sctp_getname,
> .poll = sctp_poll,
> .ioctl = inet6_ioctl,
> .listen = sctp_inet_listen,
> @@ -974,8 +1004,6 @@ static struct sctp_af sctp_af_inet6 = {
> .copy_addrlist = sctp_v6_copy_addrlist,
> .from_skb = sctp_v6_from_skb,
> .from_sk = sctp_v6_from_sk,
> - .to_sk_saddr = sctp_v6_to_sk_saddr,
> - .to_sk_daddr = sctp_v6_to_sk_daddr,
> .from_addr_param = sctp_v6_from_addr_param,
> .to_addr_param = sctp_v6_to_addr_param,
> .cmp_addr = sctp_v6_cmp_addr,
> @@ -1006,6 +1034,8 @@ static struct sctp_pf sctp_pf_inet6 = {
> .supported_addrs = sctp_inet6_supported_addrs,
> .create_accept_sk = sctp_v6_create_accept_sk,
> .addr_v4map = sctp_v6_addr_v4map,
> + .to_sk_saddr = sctp_v6_to_sk_saddr,
> + .to_sk_daddr = sctp_v6_to_sk_daddr,
> .af = &sctp_af_inet6,
> };
>
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 6789d785e698..db48bf739f5f 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -977,6 +977,8 @@ static struct sctp_pf sctp_pf_inet = {
> .supported_addrs = sctp_inet_supported_addrs,
> .create_accept_sk = sctp_v4_create_accept_sk,
> .addr_v4map = sctp_v4_addr_v4map,
> + .to_sk_saddr = sctp_v4_to_sk_saddr,
> + .to_sk_daddr = sctp_v4_to_sk_daddr,
> .af = &sctp_af_inet
> };
>
> @@ -1047,8 +1049,6 @@ static struct sctp_af sctp_af_inet = {
> .copy_addrlist = sctp_v4_copy_addrlist,
> .from_skb = sctp_v4_from_skb,
> .from_sk = sctp_v4_from_sk,
> - .to_sk_saddr = sctp_v4_to_sk_saddr,
> - .to_sk_daddr = sctp_v4_to_sk_daddr,
> .from_addr_param = sctp_v4_from_addr_param,
> .to_addr_param = sctp_v4_to_addr_param,
> .cmp_addr = sctp_v4_cmp_addr,
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 429899689408..fc5ec3e698a6 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -396,7 +396,7 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
> /* Copy back into socket for getsockname() use. */
> if (!ret) {
> inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num);
> - af->to_sk_saddr(addr, sk);
> + sp->pf->to_sk_saddr(addr, sk);
> }
>
> return ret;
> @@ -1053,7 +1053,6 @@ static int __sctp_connect(struct sock *sk,
> struct sctp_association *asoc2;
> struct sctp_transport *transport;
> union sctp_addr to;
> - struct sctp_af *af;
> sctp_scope_t scope;
> long timeo;
> int err = 0;
> @@ -1081,6 +1080,8 @@ static int __sctp_connect(struct sock *sk,
> /* Walk through the addrs buffer and count the number of addresses. */
> addr_buf = kaddrs;
> while (walk_size < addrs_size) {
> + struct sctp_af *af;
> +
> if (walk_size + sizeof(sa_family_t) > addrs_size) {
> err = -EINVAL;
> goto out_free;
> @@ -1205,8 +1206,7 @@ static int __sctp_connect(struct sock *sk,
>
> /* Initialize sk's dport and daddr for getpeername() */
> inet_sk(sk)->inet_dport = htons(asoc->peer.port);
> - af = sctp_get_af_specific(sa_addr->sa.sa_family);
> - af->to_sk_daddr(sa_addr, sk);
> + sp->pf->to_sk_daddr(sa_addr, sk);
> sk->sk_err = 0;
>
> /* in-kernel sockets don't generally have a file allocated to them
> @@ -4301,8 +4301,8 @@ static int sctp_getsockopt_autoclose(struct sock *sk, int len, char __user *optv
> int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> {
> struct sctp_association *asoc = sctp_id2assoc(sk, id);
> + struct sctp_sock *sp = sctp_sk(sk);
> struct socket *sock;
> - struct sctp_af *af;
> int err = 0;
>
> if (!asoc)
> @@ -4324,8 +4324,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
> /* Make peeled-off sockets more like 1-1 accepted sockets.
> * Set the daddr and initialize id to something more random
> */
> - af = sctp_get_af_specific(asoc->peer.primary_addr.sa.sa_family);
> - af->to_sk_daddr(&asoc->peer.primary_addr, sk);
> + sp->pf->to_sk_daddr(&asoc->peer.primary_addr, sk);
>
> /* Populate the fields of the newsk from the oldsk and migrate the
> * asoc to the newsk.
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 7dd672fa651f..cf6c267a119c 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -289,8 +289,8 @@ void sctp_transport_route(struct sctp_transport *transport,
> */
> if (asoc && (!asoc->peer.primary_path ||
> (transport == asoc->peer.active_path)))
> - opt->pf->af->to_sk_saddr(&transport->saddr,
> - asoc->base.sk);
> + opt->pf->to_sk_saddr(&transport->saddr,
> + asoc->base.sk);
> } else
> transport->pathmtu = SCTP_DEFAULT_MAXSEGMENT;
> }
> --
> 1.9.1
>
>
Sorry to do this, but I'm about to head out on vacation. I'll take a look at
this in about a week though.
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
