Ooops with SCTP

Ooops with SCTP

[Jason Gunthorpe]

Hello All,

I've recently started doing some work with SCTP and noticed a few
bugs, the worst being a kernel oops.

 1) Requesting COOKIE_ACK to be auth'd but not COOKIE_ECHO (or vice
    versa) caused an immediate panic in a IRQ context, 'go reboot your
    machine' with sctp traceback. I would need to setup in a VM to
    capture the oops text..
 2) SCTP_I_WANT_MAPPED_V4_ADDR returns a 0 AF_INET6 for all IPv4
    addresses, looking at the code I think the functionality is just
    not implemented?
 3) Using auth on COOKIE_ECHO and COOKIE_ACK and combining that with
    peer-peer connection does not seem to work. If the peers collide
    the handshake never completes. Works if the peers do not collide.

  1   0.000000   10.0.0.161 -> 10.0.0.177   SCTP 174 INIT 
  5   1.037194   10.0.0.177 -> 10.0.0.161   SCTP 146 INIT 
  6   1.037313   10.0.0.161 -> 10.0.0.177   SCTP 494 INIT_ACK 
  7   1.037649   10.0.0.177 -> 10.0.0.161   SCTP 402 AUTH COOKIE_ECHO 
  8   3.003226   10.0.0.161 -> 10.0.0.177   SCTP 174 INIT 
  9   3.003588   10.0.0.177 -> 10.0.0.161   SCTP 466 INIT_ACK 
 10   3.003641   10.0.0.161 -> 10.0.0.177   SCTP 402 AUTH COOKIE_ECHO 
 11   4.042864   10.0.0.177 -> 10.0.0.161   SCTP 402 AUTH COOKIE_ECHO 
 12   6.011268   10.0.0.161 -> 10.0.0.177   SCTP 402 AUTH COOKIE_ECHO 

    I see in the RFCs there is a corner case here in how to choose the
    proper keying material.

If there is someone out there interested in these things I can
probably provide code to reproduce?

Thanks,
Jason

Re: Ooops with SCTP

[Neil Horman]

On Fri, Jul 04, 2014 at 06:16:06PM -0600, Jason Gunthorpe wrote:
> Hello All,
> 
> I've recently started doing some work with SCTP and noticed a few
> bugs, the worst being a kernel oops.
> 
>  1) Requesting COOKIE_ACK to be auth'd but not COOKIE_ECHO (or vice
>     versa) caused an immediate panic in a IRQ context, 'go reboot your
>     machine' with sctp traceback. I would need to setup in a VM to
>     capture the oops text..

Do you have the panic backtrace?

>  2) SCTP_I_WANT_MAPPED_V4_ADDR returns a 0 AF_INET6 for all IPv4
>     addresses, looking at the code I think the functionality is just
>     not implemented?
No, its implemented, not sure why you would get a 0 address here.

>  3) Using auth on COOKIE_ECHO and COOKIE_ACK and combining that with
>     peer-peer connection does not seem to work. If the peers collide
>     the handshake never completes. Works if the peers do not collide.
> 
>   1   0.000000   10.0.0.161 -> 10.0.0.177   SCTP 174 INIT 
>   5   1.037194   10.0.0.177 -> 10.0.0.161   SCTP 146 INIT 
>   6   1.037313   10.0.0.161 -> 10.0.0.177   SCTP 494 INIT_ACK 
>   7   1.037649   10.0.0.177 -> 10.0.0.161   SCTP 402 AUTH COOKIE_ECHO 
>   8   3.003226   10.0.0.161 -> 10.0.0.177   SCTP 174 INIT 
>   9   3.003588   10.0.0.177 -> 10.0.0.161   SCTP 466 INIT_ACK 
>  10   3.003641   10.0.0.161 -> 10.0.0.177   SCTP 402 AUTH COOKIE_ECHO 
>  11   4.042864   10.0.0.177 -> 10.0.0.161   SCTP 402 AUTH COOKIE_ECHO 
>  12   6.011268   10.0.0.161 -> 10.0.0.177   SCTP 402 AUTH COOKIE_ECHO 
> 
>     I see in the RFCs there is a corner case here in how to choose the
>     proper keying material.
> 
> If there is someone out there interested in these things I can
> probably provide code to reproduce?
> 
> Thanks,
> Jason
> 

Re: Ooops with SCTP

[Jason Gunthorpe]

On Sat, Jul 05, 2014 at 09:03:49AM -0400, Neil Horman wrote:

> >  1) Requesting COOKIE_ACK to be auth'd but not COOKIE_ECHO (or vice
> >     versa) caused an immediate panic in a IRQ context, 'go reboot your
> >     machine' with sctp traceback. I would need to setup in a VM to
> >     capture the oops text..
> 
> Do you have the panic backtrace?

No, the machine crashed in a way that produced no permanent log. I
will have to reproduce it in a VM.
 
> >  2) SCTP_I_WANT_MAPPED_V4_ADDR returns a 0 AF_INET6 for all IPv4
> >     addresses, looking at the code I think the functionality is just
> >     not implemented?
> No, its implemented, not sure why you would get a 0 address here.

Sorry, I see I was unclear, SCTP_I_WANT_MAPPED_V4_ADDR=1 (the
default works fine), it is SCTP_I_WANT_MAPPED_V4_ADDR=0 that doesn't
seem implemented.

Look at functions like this:

/* 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) {
                inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
                inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
                inet6_sk(sk)->rcv_saddr.s6_addr32[2] 		htonl(0x0000ffff);
                inet6_sk(sk)->rcv_saddr.s6_addr32[3]                         addr->v4.sin_addr.s_addr;
        } else {
                inet6_sk(sk)->rcv_saddr = addr->v6.sin6_addr;
        }
}

There is no if block to handle 'addr->sa.sa_family = AF_INET &&
!sctp_sk(sk)->v4mapped'

Instead it falls through to copying memory beyond the end of the
socket address. This has always resulted in a zero AF_INET6 address in
my handful of tests.

Jason

Re: Ooops with SCTP

[Neil Horman]

On Sat, Jul 05, 2014 at 10:39:09AM -0600, Jason Gunthorpe wrote:
> On Sat, Jul 05, 2014 at 09:03:49AM -0400, Neil Horman wrote:
> 
> > >  1) Requesting COOKIE_ACK to be auth'd but not COOKIE_ECHO (or vice
> > >     versa) caused an immediate panic in a IRQ context, 'go reboot your
> > >     machine' with sctp traceback. I would need to setup in a VM to
> > >     capture the oops text..
> > 
> > Do you have the panic backtrace?
> 
> No, the machine crashed in a way that produced no permanent log. I
> will have to reproduce it in a VM.
>  
Ok, please do, otherwise you don't have enough information for us to help you
here.

> > >  2) SCTP_I_WANT_MAPPED_V4_ADDR returns a 0 AF_INET6 for all IPv4
> > >     addresses, looking at the code I think the functionality is just
> > >     not implemented?
> > No, its implemented, not sure why you would get a 0 address here.
> 
> Sorry, I see I was unclear, SCTP_I_WANT_MAPPED_V4_ADDR=1 (the
> default works fine), it is SCTP_I_WANT_MAPPED_V4_ADDR=0 that doesn't
> seem implemented.
> 
> Look at functions like this:
> 
> /* 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) {
>                 inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
>                 inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
>                 inet6_sk(sk)->rcv_saddr.s6_addr32[2] > 		htonl(0x0000ffff);
>                 inet6_sk(sk)->rcv_saddr.s6_addr32[3] >                         addr->v4.sin_addr.s_addr;
>         } else {
>                 inet6_sk(sk)->rcv_saddr = addr->v6.sin6_addr;
>         }
> }
> 
> There is no if block to handle 'addr->sa.sa_family = AF_INET &&
> !sctp_sk(sk)->v4mapped'
> 
Yes, there is, its the else clause there.  This is the ipv6 to_sk_saddr
function.  If you don't want a v4mapped address, you should assign a real ipv6
address to the socket.  What seems wierd is that there isn't an extra check to
ensure that the family is AF_INET6 in the else clause, but I think its done
higher up the call stack.  Although if its not, that could be a problem

> Instead it falls through to copying memory beyond the end of the
> socket address. This has always resulted in a zero AF_INET6 address in
> my handful of tests.
> 
Hmm, looking at the code, I'm not sure how we reach that point.  You need to set
you sa_family code to AF_INET6 to get the v6 mapping function to be called, but
it only does the v4mapping above if the sa_family is AF_INET.  Looks like that
might need fixing.  I'll look at that on monday.


Neil

Re: Ooops with SCTP

[Jason Gunthorpe]

On Sun, Jul 06, 2014 at 08:21:33AM -0400, Neil Horman wrote:

> > /* 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) {
> >                 inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
> >                 inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
> >                 inet6_sk(sk)->rcv_saddr.s6_addr32[2] > > 		htonl(0x0000ffff);
> >                 inet6_sk(sk)->rcv_saddr.s6_addr32[3] > >                         addr->v4.sin_addr.s_addr;
> >         } else {
> >                 inet6_sk(sk)->rcv_saddr = addr->v6.sin6_addr;
> >         }
> > }
> > 
> > There is no if block to handle 'addr->sa.sa_family = AF_INET &&
> > !sctp_sk(sk)->v4mapped'

> Yes, there is, its the else clause there.  This is the ipv6 to_sk_saddr
> function.  If you don't want a v4mapped address, you should assign a real ipv6
> address to the socket.  What seems wierd is that there isn't an extra check to
> ensure that the family is AF_INET6 in the else clause, but I think its done
> higher up the call stack.  Although if its not, that could be a problem

Well, if sctp_v6_to_sk_saddr is called with AF_INET and v4mapped = 0,
the result would match the symptoms I observed.

Note, the sequence I used was
  socket(AF_INET6)
  sctp_bindx(..) (to both IPv4 and IPv6 addresses)
  setsockopt(SCTP_I_WANT_MAPPED_V4_ADDR = 0)
  [peer connects using IPv4]
  recvmsg().. // Get AF_INET6 with '::' as the address

> Hmm, looking at the code, I'm not sure how we reach that point.  You need to set
> you sa_family code to AF_INET6 to get the v6 mapping function to be called, but
> it only does the v4mapping above if the sa_family is AF_INET.  Looks like that
> might need fixing.  I'll look at that on monday.

Well, I looked a bit, and there are certainly cases, eg bind calls
sctp_v6_to_sk_saddr unconditionally.. 

Persumably there are cases with v4/v6 interworking where the netstack
delivers AF_INET addresses and that function gets called?

Regards,
Jason

Re: Ooops with SCTP

[Neil Horman]

On Sun, Jul 06, 2014 at 10:48:33PM -0600, Jason Gunthorpe wrote:
> On Sun, Jul 06, 2014 at 08:21:33AM -0400, Neil Horman wrote:
> 
> > > /* 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) {
> > >                 inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
> > >                 inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
> > >                 inet6_sk(sk)->rcv_saddr.s6_addr32[2] > > > 		htonl(0x0000ffff);
> > >                 inet6_sk(sk)->rcv_saddr.s6_addr32[3] > > >                         addr->v4.sin_addr.s_addr;
> > >         } else {
> > >                 inet6_sk(sk)->rcv_saddr = addr->v6.sin6_addr;
> > >         }
> > > }
> > > 
> > > There is no if block to handle 'addr->sa.sa_family = AF_INET &&
> > > !sctp_sk(sk)->v4mapped'
> 
> > Yes, there is, its the else clause there.  This is the ipv6 to_sk_saddr
> > function.  If you don't want a v4mapped address, you should assign a real ipv6
> > address to the socket.  What seems wierd is that there isn't an extra check to
> > ensure that the family is AF_INET6 in the else clause, but I think its done
> > higher up the call stack.  Although if its not, that could be a problem
> 
> Well, if sctp_v6_to_sk_saddr is called with AF_INET and v4mapped = 0,
> the result would match the symptoms I observed.
> 
> Note, the sequence I used was
>   socket(AF_INET6)
>   sctp_bindx(..) (to both IPv4 and IPv6 addresses)
>   setsockopt(SCTP_I_WANT_MAPPED_V4_ADDR = 0)
>   [peer connects using IPv4]
>   recvmsg().. // Get AF_INET6 with '::' as the address
> 
Oh, I may see the problem.  Or at least the disconnect between the documentation
and the code.  The docs for the sctp socket api says:

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.

But the code, when MAPPED_V4_ADDR is turned off treats everything as an ipv6
address, which doesn't really jive with whats described above.  If
MAPPED_V4_ADDR is turned off, we should I think be filling out a sockaddr_in
structure if the received frame type header is version 4.  Can you try the patch
attached below to see if it fixes the issue?


> > Hmm, looking at the code, I'm not sure how we reach that point.  You need to set
> > you sa_family code to AF_INET6 to get the v6 mapping function to be called, but
> > it only does the v4mapping above if the sa_family is AF_INET.  Looks like that
> > might need fixing.  I'll look at that on monday.
> 
> Well, I looked a bit, and there are certainly cases, eg bind calls
> sctp_v6_to_sk_saddr unconditionally.. 
> 
No it doesn't, it calls whatever the get_saddr function is assigned based on the
protocol family that is selected, though I suppose thats just minutae based on
the above findings.

> Persumably there are cases with v4/v6 interworking where the netstack
> delivers AF_INET addresses and that function gets called?
> 
No, that one is for outgoing frames, based on what you told me above about yur
operational sequence, I think we're looking at a problem in
sctp_inet6_skb_msgname here, hence the patch below.


diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 1999592..2e24d8f 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -760,22 +760,31 @@ 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);

Re: Ooops with SCTP

[Jason Gunthorpe]

On Mon, Jul 07, 2014 at 08:44:44AM -0400, Neil Horman wrote:

> > Well, if sctp_v6_to_sk_saddr is called with AF_INET and v4mapped = 0,
> > the result would match the symptoms I observed.
> > 
> > Note, the sequence I used was
> >   socket(AF_INET6)
> >   sctp_bindx(..) (to both IPv4 and IPv6 addresses)
> >   setsockopt(SCTP_I_WANT_MAPPED_V4_ADDR = 0)
> >   [peer connects using IPv4]
> >   recvmsg().. // Get AF_INET6 with '::' as the address

> Oh, I may see the problem.  Or at least the disconnect between the documentation
> and the code.  The docs for the sctp socket api says:
>
> 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.
> 
> But the code, when MAPPED_V4_ADDR is turned off treats everything as an ipv6
> address, which doesn't really jive with whats described above.  If
> MAPPED_V4_ADDR is turned off, we should I think be filling out a sockaddr_in
> structure if the received frame type header is version 4.  Can you try the patch
> attached below to see if it fixes the issue?

Right, I noticed every use of v4mapped seemed strange to me, like
why should v4mapped cause sctp_v6_available and sctp_v6_addr_valid to
behave any different? (And curiously, after I added tracing I didn't
hit those cases, so I'm not sure what they are for)

So your patch is the right idea, but we need to fix every case, I
added code for sctp_inet6_event_msgname too and now my little test
works OK:

After recvmsg: sockaddr is 2 -- 127.0.0.1
Connection attempt failed 0
After recvmsg: sockaddr is 2 -- 127.0.0.1
Peer connection is UP
After recvmsg: sockaddr is 2 -- 127.0.0.1
Peer connection is UP

See attached patch for what I tested.

But, I also added BUG_ON(addr->sa.sa_family != AF_INET6); to the else
in the sk_s/daddr functions, and it triggers:

[<c024dd18>] (sctp_v6_to_sk_saddr+0x0/0x64) from [<c0237cdc>] (sctp_transport_route+0xd0/0xe4)
[<c0237c0c>] (sctp_transport_route+0x0/0xe4) from [<c0236938>] (sctp_assoc_add_peer+0xc0/0x264)
[<c0236878>] (sctp_assoc_add_peer+0x0/0x264) from [<c0242558>] (__sctp_connect+0x1dc/0x464)
[<c024237c>] (__sctp_connect+0x0/0x464) from [<c02428d0>] (sctp_connect+0x4c/0x68)
[<c0242884>] (sctp_connect+0x0/0x68) from [<c01d6fc4>] (inet_dgram_connect+0x78/0x8c)
 r6:c7be3400 r5:c6f8ff00 r4:00000010 r3:c0242884
[<c01d6f4c>] (inet_dgram_connect+0x0/0x8c) from [<c01731bc>] (SyS_connect+0x70/0x94)
 r6:b6374d64 r5:00000010 r4:c742eb40 r3:00000802
[<c017314c>] (SyS_connect+0x0/0x94) from [<c0009060>] (ret_fast_syscall+0x0/0x2c)

Which looks like it is because userspace is calling connect(AF_INET)..

So there is still more wrong here..

Jason

From 0862335e5b6e338339e0000267dc8568ee2aae6b Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Mon, 7 Jul 2014 11:09:16 -0600
Subject: [PATCH] net: sctp: Fix SCTP_I_WANT_MAPPED_V4_ADDR

If this option was set to 0 then the kernel would return a bogus
sockaddr from recv for v4 associations instead of the AF_INET sockaddr
it should return.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 net/sctp/ipv6.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 7567e6f1a920..bf18661cfaeb 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -736,12 +736,19 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
 		 * will change.
 		 */
 
-		/* 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;
+		/* Map ipv4 address into v4-mapped-on-v6 address. */
+		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;
+			} else {
+				struct sockaddr_in *sin +				    (struct sockaddr_in *)msgname;
+				sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
+				sin->sin_family = AF_INET;
+				sin->sin_port = htons(asoc->peer.port);
+			}
 			return;
 		}
 
@@ -758,22 +765,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);
-- 
1.9.1

Re: Ooops with SCTP

[Neil Horman]

On Mon, Jul 07, 2014 at 11:45:31AM -0600, Jason Gunthorpe wrote:
> On Mon, Jul 07, 2014 at 08:44:44AM -0400, Neil Horman wrote:
> 
> > > Well, if sctp_v6_to_sk_saddr is called with AF_INET and v4mapped = 0,
> > > the result would match the symptoms I observed.
> > > 
> > > Note, the sequence I used was
> > >   socket(AF_INET6)
> > >   sctp_bindx(..) (to both IPv4 and IPv6 addresses)
> > >   setsockopt(SCTP_I_WANT_MAPPED_V4_ADDR = 0)
> > >   [peer connects using IPv4]
> > >   recvmsg().. // Get AF_INET6 with '::' as the address
> 
> > Oh, I may see the problem.  Or at least the disconnect between the documentation
> > and the code.  The docs for the sctp socket api says:
> >
> > 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.
> > 
> > But the code, when MAPPED_V4_ADDR is turned off treats everything as an ipv6
> > address, which doesn't really jive with whats described above.  If
> > MAPPED_V4_ADDR is turned off, we should I think be filling out a sockaddr_in
> > structure if the received frame type header is version 4.  Can you try the patch
> > attached below to see if it fixes the issue?
> 
> Right, I noticed every use of v4mapped seemed strange to me, like
> why should v4mapped cause sctp_v6_available and sctp_v6_addr_valid to
> behave any different? (And curiously, after I added tracing I didn't
> hit those cases, so I'm not sure what they are for)
> 

The places we need to check are:

sctp_v6_to_sk_saddr - Fixed in the patch

sctp_v6_to_sk_daddr - Needs to be fixed the same way as above

sctp_v6_available - looks like maybe we should just call sctp_v6_map_v4 if
v4mapped is true.  Otherwise just pass it to the v4 available method as an ipv4
address. Actually we can probably just call sctp_v6_addr_v4map unilaterally as
the proper check is there

sctp_v6_addr_valid - Ditto, just map the address if we support mapping

sctp_inet6_event_msgname - Need to do like we did in sk_saddr and friends, and
fill out the msghdr as an ipv4 msg if we don't support mapping.

sctp_inet6_skb_msgname - ditto to event_msgname

sctp_inet6_bind_verify - not sure on this, think its ok

I'll try get to expanding this patch in the next few days

Neil
 
> So your patch is the right idea, but we need to fix every case, I
> added code for sctp_inet6_event_msgname too and now my little test
> works OK:
> 
> After recvmsg: sockaddr is 2 -- 127.0.0.1
> Connection attempt failed 0
> After recvmsg: sockaddr is 2 -- 127.0.0.1
> Peer connection is UP
> After recvmsg: sockaddr is 2 -- 127.0.0.1
> Peer connection is UP
> 
> See attached patch for what I tested.
> 
> But, I also added BUG_ON(addr->sa.sa_family != AF_INET6); to the else
> in the sk_s/daddr functions, and it triggers:
> 
> [<c024dd18>] (sctp_v6_to_sk_saddr+0x0/0x64) from [<c0237cdc>] (sctp_transport_route+0xd0/0xe4)
> [<c0237c0c>] (sctp_transport_route+0x0/0xe4) from [<c0236938>] (sctp_assoc_add_peer+0xc0/0x264)
> [<c0236878>] (sctp_assoc_add_peer+0x0/0x264) from [<c0242558>] (__sctp_connect+0x1dc/0x464)
> [<c024237c>] (__sctp_connect+0x0/0x464) from [<c02428d0>] (sctp_connect+0x4c/0x68)
> [<c0242884>] (sctp_connect+0x0/0x68) from [<c01d6fc4>] (inet_dgram_connect+0x78/0x8c)
>  r6:c7be3400 r5:c6f8ff00 r4:00000010 r3:c0242884
> [<c01d6f4c>] (inet_dgram_connect+0x0/0x8c) from [<c01731bc>] (SyS_connect+0x70/0x94)
>  r6:b6374d64 r5:00000010 r4:c742eb40 r3:00000802
> [<c017314c>] (SyS_connect+0x0/0x94) from [<c0009060>] (ret_fast_syscall+0x0/0x2c)
> 
> Which looks like it is because userspace is calling connect(AF_INET)..
> 
> So there is still more wrong here..
> 
> Jason
> 
> From 0862335e5b6e338339e0000267dc8568ee2aae6b Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Date: Mon, 7 Jul 2014 11:09:16 -0600
> Subject: [PATCH] net: sctp: Fix SCTP_I_WANT_MAPPED_V4_ADDR
> 
> If this option was set to 0 then the kernel would return a bogus
> sockaddr from recv for v4 associations instead of the AF_INET sockaddr
> it should return.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  net/sctp/ipv6.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 7567e6f1a920..bf18661cfaeb 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -736,12 +736,19 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
>  		 * will change.
>  		 */
>  
> -		/* 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;
> +		/* Map ipv4 address into v4-mapped-on-v6 address. */
> +		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;
> +			} else {
> +				struct sockaddr_in *sin > +				    (struct sockaddr_in *)msgname;
> +				sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
> +				sin->sin_family = AF_INET;
> +				sin->sin_port = htons(asoc->peer.port);
> +			}
>  			return;
>  		}
>  
> @@ -758,22 +765,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);
> -- 
> 1.9.1
> 
> 

Re: Ooops with SCTP

[Jason Gunthorpe]

On Mon, Jul 07, 2014 at 02:22:36PM -0400, Neil Horman wrote:

> I'll try get to expanding this patch in the next few days

Sure, I should be able to test your expansion on the patch I sent, let
me know.

Regards,
Jason

Re: Ooops with SCTP

[Neil Horman]

On Mon, Jul 07, 2014 at 01:39:41PM -0600, Jason Gunthorpe wrote:
> On Mon, Jul 07, 2014 at 02:22:36PM -0400, Neil Horman wrote:
> 
> > I'll try get to expanding this patch in the next few days
> 
> Sure, I should be able to test your expansion on the patch I sent, let
> me know.
> 
> Regards,
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


Sorry for the delay, busy few days.  I think this fixes up all the required call
sites that check v4mapped.  I've not tested it yet, but it builds.  If you could
please give it a spin and let me know, I'll officially propose it.

Thanks!
Neil


commit 469f5a1527ffcdd08fdcb1c45c80b840f9a7819c
Author: Neil Horman <nhorman@tuxdriver.com>
Date:   Wed Jul 9 11:36:33 2014 -0400

    sctp: Fixup v4mapped behavior 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.  If we
    have an AF_INET6 socket, that recieves a ipv4 frame and MAPPED_V4_ADDR isn't
    set, it either ignores the frame, or drops the address into an ipv6 sockaddr_in6
    structure in the wrong place (with the wrong sa_family field set).  We need to
    construct proper sockaddr_in structures when handling AF_INET packets on
    AF_INET6 sockets when MAPPED_V4_ADDR is set to 0.
    
    Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 1999592..5261ac5 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -434,12 +434,17 @@ 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) {
-		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);
-		sk->sk_v6_rcv_saddr.s6_addr32[3] +	if (addr->sa.sa_family = AF_INET) {
+		if (sctp_sk(sk)->v4mapped) {
+			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);
+			sk->sk_v6_rcv_saddr.s6_addr32[3]  			addr->v4.sin_addr.s_addr;
+		} else {
+			inet_sk(sk)->inet_rcv_saddr +			addr->v4.sin_addr.s_addr;
+		}
 	} else {
 		sk->sk_v6_rcv_saddr = addr->v6.sin6_addr;
 	}
@@ -448,11 +453,16 @@ 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) {
-		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);
-		sk->sk_v6_daddr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
+	if (addr->sa.sa_family = AF_INET) {
+		if (sctp_sk(sk)->v4mapped) {
+			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);
+			sk->sk_v6_daddr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
+		} else {
+			inet_sk(sk)->inet_daddr = addr->v4.sin_addr.s_addr;
+		}
+
 	} else {
 		sk->sk_v6_daddr = addr->v6.sin6_addr;
 	}
@@ -556,11 +566,10 @@ 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);
+		if (sp && sp->v4mapped)
+			sctp_v6_map_v4(addr);
 		return sctp_get_af_specific(AF_INET)->available(addr, sp);
 	}
 	if (!(type & IPV6_ADDR_UNICAST))
@@ -587,11 +596,10 @@ 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);
+		if (sp && sp->v4mapped)
+			sctp_v6_map_v4(addr);
 		return sctp_get_af_specific(AF_INET)->addr_valid(addr, sp, skb);
 	}
 
@@ -723,6 +731,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 +740,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 +749,16 @@ 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;
+			}
 		}
 
 		sin6from = &asoc->peer.primary_addr.v6;
@@ -760,22 +774,31 @@ 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);

Re: Ooops with SCTP

[Jason Gunthorpe]

On Wed, Jul 09, 2014 at 11:50:15AM -0400, Neil Horman wrote:

> Sorry for the delay, busy few days.  I think this fixes up all the
> required call sites that check v4mapped.  I've not tested it yet,
> but it builds.  If you could please give it a spin and let me know,
> I'll officially propose it.

Thanks, I'll try and run it today..

> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 1999592..5261ac5 100644
> +++ b/net/sctp/ipv6.c
> @@ -434,12 +434,17 @@ 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) {
> -		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);
> -		sk->sk_v6_rcv_saddr.s6_addr32[3] > +	if (addr->sa.sa_family = AF_INET) {
> +		if (sctp_sk(sk)->v4mapped) {
> +			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);
> +			sk->sk_v6_rcv_saddr.s6_addr32[3] >  			addr->v4.sin_addr.s_addr;
> +		} else {
> +			inet_sk(sk)->inet_rcv_saddr > +			addr->v4.sin_addr.s_addr;

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?

> @@ -556,11 +566,10 @@ 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);
> +		if (sp && sp->v4mapped)
> +			sctp_v6_map_v4(addr);

What scenario is required to go into this if? When I run my test I
did not hit (sp && !sp->v4mapped) ..

> @@ -587,11 +596,10 @@ static int sctp_v6_addr_valid(union sctp_addr *addr,
> -		sctp_v6_map_v4(addr);
> +		if (sp && sp->v4mapped)
> +			sctp_v6_map_v4(addr);

Ditto?

> @@ -739,12 +749,16 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> +			} else {
> +				sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
> +				sin->sin_family = AF_INET;

Missing port?

+                               sin->sin_port = htons(asoc->peer.port);

Jason

Re: Ooops with SCTP

[Neil Horman]

On Wed, Jul 09, 2014 at 10:28:14AM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 09, 2014 at 11:50:15AM -0400, Neil Horman wrote:
> 
> > Sorry for the delay, busy few days.  I think this fixes up all the
> > required call sites that check v4mapped.  I've not tested it yet,
> > but it builds.  If you could please give it a spin and let me know,
> > I'll officially propose it.
> 
> Thanks, I'll try and run it today..
> 
> > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> > index 1999592..5261ac5 100644
> > +++ b/net/sctp/ipv6.c
> > @@ -434,12 +434,17 @@ 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) {
> > -		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);
> > -		sk->sk_v6_rcv_saddr.s6_addr32[3] > > +	if (addr->sa.sa_family = AF_INET) {
> > +		if (sctp_sk(sk)->v4mapped) {
> > +			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);
> > +			sk->sk_v6_rcv_saddr.s6_addr32[3] > >  			addr->v4.sin_addr.s_addr;
> > +		} else {
> > +			inet_sk(sk)->inet_rcv_saddr > > +			addr->v4.sin_addr.s_addr;
> 
> 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.

> > @@ -556,11 +566,10 @@ 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);
> > +		if (sp && sp->v4mapped)
> > +			sctp_v6_map_v4(addr);
> 
> What scenario is required to go into this if? When I run my test I
> did not hit (sp && !sp->v4mapped) ..
> 
Not sure what you mean here.  Just because you're testing didn't hit it doesn't
really mean anything.  if v4_mapped is enabled you want to map the incomming v4
address into a v6 address, otherwise, you just want to pass the address to the
sctp v4 available method.

> > @@ -587,11 +596,10 @@ static int sctp_v6_addr_valid(union sctp_addr *addr,
> > -		sctp_v6_map_v4(addr);
> > +		if (sp && sp->v4mapped)
> > +			sctp_v6_map_v4(addr);
> 
> Ditto?
> 
Ditto.

> > @@ -739,12 +749,16 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> > +			} else {
> > +				sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
> > +				sin->sin_family = AF_INET;
> 
> Missing port?
Shoot, I was thinking that was covered by the sin6 port assignment, but they
don't overlap.  Can you add that during your testing?

> 
> +                               sin->sin_port = htons(asoc->peer.port);
> 
> Jason
> 

Re: Ooops with SCTP

[Jason Gunthorpe]

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

> > > @@ -556,11 +566,10 @@ 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);
> > > +		if (sp && sp->v4mapped)
> > > +			sctp_v6_map_v4(addr);
> > 
> > What scenario is required to go into this if? When I run my test I
> > did not hit (sp && !sp->v4mapped) ..

> Not sure what you mean here.  Just because you're testing didn't hit it doesn't
> really mean anything.  if v4_mapped is enabled you want to map the incomming v4
> address into a v6 address, otherwise, you just want to pass the address to the
> sctp v4 available method.

Well, I quickly tested many combinations of v4 and v6 interworking and
it never covered that if. So if I want to see that the above is
working properly what do I need to have the test do?

Also, looking at the whole sequence, I just noticed:

-               sctp_v6_map_v4(addr);
+               if (sp && sp->v4mapped)
+                       sctp_v6_map_v4(addr);
                return sctp_get_af_specific(AF_INET)->available(addr, sp);
        }

It doesn't look right to call '(AF_INET)->available(addr' when addr is
an AF_INET6.. Doesn't making the call to sctp_v6_map_v4 conditional
create that possibility now?

> > > @@ -739,12 +749,16 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> > > +			} else {
> > > +				sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
> > > +				sin->sin_family = AF_INET;
> > 
> > Missing port?

> Shoot, I was thinking that was covered by the sin6 port assignment, but they
> don't overlap.  Can you add that during your testing?

Sure

Jason

Re: Ooops with SCTP

[Neil Horman]

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).

> > > > @@ -556,11 +566,10 @@ 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);
> > > > +		if (sp && sp->v4mapped)
> > > > +			sctp_v6_map_v4(addr);
> > > 
> > > What scenario is required to go into this if? When I run my test I
> > > did not hit (sp && !sp->v4mapped) ..
> 
> > Not sure what you mean here.  Just because you're testing didn't hit it doesn't
> > really mean anything.  if v4_mapped is enabled you want to map the incomming v4
> > address into a v6 address, otherwise, you just want to pass the address to the
> > sctp v4 available method.
> 
> Well, I quickly tested many combinations of v4 and v6 interworking and
> it never covered that if. So if I want to see that the above is
> working properly what do I need to have the test do?
> 
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.

> Also, looking at the whole sequence, I just noticed:
> 
> -               sctp_v6_map_v4(addr);
> +               if (sp && sp->v4mapped)
> +                       sctp_v6_map_v4(addr);
>                 return sctp_get_af_specific(AF_INET)->available(addr, sp);
>         }
> 
> It doesn't look right to call '(AF_INET)->available(addr' when addr is
> an AF_INET6.. Doesn't making the call to sctp_v6_map_v4 conditional
> create that possibility now?
> 
Well, its ok from the standpoint that I don't think I've changed anything from
the previous flow.  The code used to look like this:
/* Support v4-mapped-v6 address. */
        if (ret = IPV6_ADDR_MAPPED) {
                /* 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);
                return sctp_get_af_specific(AF_INET)->addr_valid(addr, sp, skb);
...

As you can see, if sp->v4mapped is set, we call both sctp_v6_map_v4, and the v4
version of addr_valid, which is no different than what we're doing now, save for
the fact that we pass true v4 addresses to addr_valid when v4mapped is disabled,
which should be fine.

thats not to say something isn't mis-coded in sctp_v4_addr_valid, but it looks
like it should work.

> > > > @@ -739,12 +749,16 @@ static void sctp_inet6_event_msgname(struct sctp_ulpevent *event,
> > > > +			} else {
> > > > +				sin->sin_addr.s_addr = addr->v4.sin_addr.s_addr;
> > > > +				sin->sin_family = AF_INET;
> > > 
> > > Missing port?
> 
> > Shoot, I was thinking that was covered by the sin6 port assignment, but they
> > don't overlap.  Can you add that during your testing?
> 
> Sure
> 
Thanks.

> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Ooops with SCTP

[Jason Gunthorpe]

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

Re: Ooops with SCTP

[Neil Horman]

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

Re: Ooops with SCTP

[Neil Horman]

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.


ok, I've looked at this over the weekend, and for the most part I think it looks
ok.  Regarding your questions, I think sctp_id2transport is ok as it is, since
that path is used for both ipv4 and ipv6, making unconditional promotion a bad
idea.  Calling the addr_map method lets us choose the v4 mapping function
(empty), or the v6 mapping function, which only does a map if a v4 address is
passed in.

And I don't think we need to do any demotion of addresses as sctp_v6_addr_v4map
is only called if the socket is AF_INET6, and can handle v6 addresses
transparently.  If the socket was created as AF_INET, we're safe, because that
will reject AF_INET6 addresses anyway.

If you're testing in the interviening week hasn't shown any further problems,
I'd say officially post this with my signed off please.
Neil

Re: Ooops with SCTP

[Jason Gunthorpe]

On Mon, Jul 21, 2014 at 07:15:45AM -0400, Neil Horman wrote:

> ok, I've looked at this over the weekend, and for the most part I think it looks
> ok.  Regarding your questions, I think sctp_id2transport is ok as it is, since
> that path is used for both ipv4 and ipv6, making unconditional promotion a bad
> idea.  Calling the addr_map method lets us choose the v4 mapping function
> (empty), or the v6 mapping function, which only does a map if a v4 address is
> passed in.

Well, v4mapped should only change the presentation of addresses being
returned by syscalls, it should never change the internal behavior of
SCTP.

Hrm.. So sctp_id2transport has this as a trailing call:

	sctp_get_pf_specific(sk->sk_family)->addr_v4map(sctp_sk(sk),
						(union sctp_addr
						*)addr);

Thus it does not alter the operation of sctp_id2transport in any way,
it just changes the presentation of the address for the caller.. So
that is good.

Going through the callers:

sctp_getsockopt_peer_addr_params -
sctp_getsockopt_peer_addr_info -
sctp_getsockopt_peer_addr_params -
sctp_getsockopt_paddr_thresholds
   The address is copied back to user space after the call
   completes

sctp_setsockopt_peer_addr_params -
sctp_setsockopt_primary_addr -
sctp_setsockopt_paddr_thresholds -
  The address is never used again

Basically, if you do a getsockopt with an address then the structure
return'd by the kernel will be passed through addr_v4map.

It is a bit odd that the kernel transforms an input argument when
copying to output, but that doesn't necessarily seem bad...

> And I don't think we need to do any demotion of addresses as sctp_v6_addr_v4map
> is only called if the socket is AF_INET6, and can handle v6 addresses
> transparently.  If the socket was created as AF_INET, we're safe, because that
> will reject AF_INET6 addresses anyway.

sctp_v6_addr_v4map appears to be called on every sockaddr before
returning it to user space. 

So the job of sctp_v6_addr_v4map is to enforce the v4mapped behavior:
 - If v4mapped is set all returns from the kernel should be AF_INET6
 - If v4mapped is unset, all IPv4 addresses should be AF_INET, a V6
   mapped should never be returned.

Right now it only promotes AF_INET to AF_INET6, it doesn't demote
AF_INET6 v4mapped back to AF_INET.

So the question is if v4mapped AF_INET6 addresses can exist inside the
kernel. If yes, we need to demote them in sctp_v6_addr_v4map for
correct presentation to userspace.

Unless you know the design is to keep v4 as AF_INET, then I'm thinking
for robustness it should just demote. Then we know the user API is
going to work properly no matter what changes are made to the
internals.

> If you're testing in the interviening week hasn't shown any further problems,
> I'd say officially post this with my signed off please.

I'll try to get a few mins to verify the above for sctp_v6_addr_v4map
and post the result.

Thanks,
Jason