Re: Ooops with SCTP

[Neil Horman <nhorman@tuxdriver.com>]

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@obsidianresearch.com>
> 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@tuxdriver.com>
> Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  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