Ooops with SCTP

DISCUSSION Jason Gunthorpe 2 participants 18 messages Jul 5, 2014 – Jul 23, 2014

Subsystems (2)

Networking SCTP protocol

lore

Share & tools

lore

Jason Gunthorpe

author

2014-07-05 00:16 UTC #0 lore raw

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

Neil Horman

2014-07-05 13:03 UTC #1 lore raw

Quoted 9 lines · 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..

Do you have the panic backtrace?

Quoted 3 lines

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.

Quoted 23 lines

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

Jason Gunthorpe

author

2014-07-05 16:39 UTC #2 lore raw

In reply to Jason Gunthorpe

Quoted 6 lines · Neil Horman

Quoted 4 lines
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.

Quoted 4 lines

Quoted 3 lines
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

Neil Horman

2014-07-06 12:21 UTC #3 lore raw

In reply to Jason Gunthorpe

Quoted 7 lines · Jason Gunthorpe

Quoted 5 lines · Neil Horman
Quoted 4 lines
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..

Quoted 5 lines

Quoted 1 line
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.

Quoted 24 lines

Quoted 4 lines
Quoted 3 lines
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;
        }
}

Quoted 3 lines

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

Quoted 4 lines

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

Jason Gunthorpe

author

2014-07-07 04:48 UTC #4 lore raw

In reply to Jason Gunthorpe

Quoted 13 lines · Neil Horman

Quoted 13 lines

/* 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;
        }
}

Quoted 8 lines

Quoted 2 lines
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

Quoted 4 lines

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

Neil Horman

2014-07-07 12:44 UTC #5 lore raw

In reply to Jason Gunthorpe

Quoted 27 lines · Jason Gunthorpe

Quoted 13 lines · Neil Horman
Quoted 13 lines
/* 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;
        }
}
Quoted 8 lines
Quoted 2 lines
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.

Quoted 7 lines

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?

Quoted 8 lines

Quoted 4 lines
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.

Quoted 3 lines

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.

net/sctp/ipv6.c +14 -5

index 1999592..2e24d8f 100644
@@ -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);

Jason Gunthorpe

author Tested-by

2014-07-07 17:45 UTC #6 lore raw

In reply to Jason Gunthorpe

Quoted 22 lines · Neil Horman

Quoted 9 lines
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.

Quoted 5 lines

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 <[email protected]>
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 <[email protected]>

Tested-by Jason Gunthorpe <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

---
 net/sctp/ipv6.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

net/sctp/ipv6.c +25 ~~-10~~

index 7567e6f1a920..bf18661cfaeb 100644
@@ -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);

Signature · 1 line

1.9.1

Neil Horman

2014-07-07 18:22 UTC #7 lore raw

In reply to Jason Gunthorpe

Quoted 30 lines · Jason Gunthorpe

Quoted 22 lines · Neil Horman
Quoted 9 lines
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.
Quoted 5 lines
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?

Quoted 5 lines

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

Quoted 116 lines

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 <[email protected]>
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 <[email protected]>
Tested-by Jason Gunthorpe <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
---
 net/sctp/ipv6.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)
net/sctp/ipv6.c +25 ~~-10~~
index 7567e6f1a920..bf18661cfaeb 100644
@@ -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);
Signature · 1 line
1.9.1

Jason Gunthorpe

author

2014-07-07 19:39 UTC #8 lore raw

In reply to Jason Gunthorpe

Quoted 1 line · Neil Horman

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

Neil Horman

2014-07-09 15:50 UTC #9 lore raw

In reply to Jason Gunthorpe

Quoted 7 lines · Jason Gunthorpe

Quoted 1 line · Neil Horman
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.

Quoted 7 lines

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
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 <[email protected]>
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 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 <[email protected]>

net/sctp/ipv6.c +47 ~~-26~~

index 1999592..5261ac5 100644
@@ -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);

Jason Gunthorpe

author

2014-07-09 16:28 UTC #10 lore raw

In reply to Jason Gunthorpe

Quoted 4 lines · Neil Horman

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

Quoted 9 lines

net/sctp/ipv6.c +1 -2

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;

net/sctp/ipv6.c static void sctp_v6_from_sk(union sctp_addr *addr, struct sock *sk) · line 443

Quoted 10 lines

net/sctp/ipv6.c +7 -3

-		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?

net/sctp/ipv6.c static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp) · line 572

Quoted 11 lines

net/sctp/ipv6.c +2 -3

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

net/sctp/ipv6.c static int sctp_v6_addr_valid(union sctp_addr *addr, · line 597

Quoted 4 lines

net/sctp/ipv6.c +2 -1

@@ -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?

net/sctp/ipv6.c static void sctp_inet6_event_msgname(struct sctp_ulpevent *event, · line 751

Quoted 4 lines

net/sctp/ipv6.c +3 -0

@@ -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

Neil Horman

2014-07-09 18:27 UTC #11 lore raw

In reply to Jason Gunthorpe

Quoted 23 lines · Jason Gunthorpe

Quoted 4 lines · Neil Horman
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..
Quoted 14 lines
net/sctp/ipv6.c +3 -5
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;

Quoted 9 lines

net/sctp/ipv6.c static void sctp_v6_from_sk(union sctp_addr *addr, struct sock *sk) · line 443
Quoted 5 lines
net/sctp/ipv6.c +5 -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.

Quoted 6 lines

net/sctp/ipv6.c +0 -2

@@ -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;

Quoted 9 lines

net/sctp/ipv6.c static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp) · line 572
Quoted 5 lines
net/sctp/ipv6.c +2 -1
 		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.

Quoted 7 lines

net/sctp/ipv6.c static int sctp_v6_addr_valid(union sctp_addr *addr, · line 597
Quoted 4 lines
net/sctp/ipv6.c +2 -1
@@ -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.

Quoted 6 lines

net/sctp/ipv6.c static void sctp_inet6_event_msgname(struct sctp_ulpevent *event, · line 751
Quoted 4 lines
net/sctp/ipv6.c +3 -0
@@ -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?

Quoted 5 lines

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

Jason Gunthorpe

author

2014-07-09 18:51 UTC #12 lore raw

In reply to Jason Gunthorpe

Quoted 4 lines · Neil Horman

Quoted 3 lines
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

Quoted 15 lines

Quoted 6 lines
Quoted 6 lines
(quoted diff) +0 -2
@@ -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;
Quoted 8 lines
Review static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp) · line 572
Quoted 5 lines
(quoted diff) +2 -1
 		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) ..

Quoted 4 lines

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?

Quoted 9 lines

Quoted 6 lines
Review static void sctp_inet6_event_msgname(struct sctp_ulpevent *event, · line 751
Quoted 4 lines
(quoted diff) +3 -0
@@ -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

Neil Horman

2014-07-10 11:33 UTC #13 lore raw

In reply to Jason Gunthorpe

Quoted 9 lines · Jason Gunthorpe

Quoted 4 lines · Neil Horman
Quoted 3 lines
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).

Quoted 15 lines

Quoted 14 lines
Quoted 14 lines
Review static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp) · line 572
Quoted 11 lines
(quoted diff) +2 -3
@@ -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) ..

Quoted 9 lines

Quoted 4 lines
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.

Quoted 8 lines

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

(quoted diff) +2 -1

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

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines
Quoted 6 lines
Review static void sctp_inet6_event_msgname(struct sctp_ulpevent *event, · line 751
Quoted 4 lines
(quoted diff) +3 -0
@@ -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?

Quoted 5 lines

Quoted 2 lines
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.

Quoted 6 lines

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

Jason Gunthorpe

author Tested-by

2014-07-10 19:58 UTC #14 lore raw

In reply to Jason Gunthorpe

Quoted 6 lines · Neil Horman

Quoted 5 lines · Jason Gunthorpe
Quoted 3 lines · Neil Horman
Quoted 3 lines
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?

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 5 lines

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 <[email protected]>
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 <[email protected]>

Tested-by Jason Gunthorpe <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

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

include/net/sctp/structs.h +4 -4

index f38588bf3462..a5704e597c9a 100644
@@ -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;
 };

net/sctp/ipv6.c +51 ~~-22~~

162 lines not on this page · raw message

net/sctp/protocol.c +2 -2

index 6789d785e698..db48bf739f5f 100644
@@ -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,

net/sctp/socket.c +6 -7

index 429899689408..fc5ec3e698a6 100644
@@ -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.

net/sctp/transport.c +2 -2

index 7dd672fa651f..cf6c267a119c 100644
@@ -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;
 }

Signature · 1 line

1.9.1

Neil Horman

2014-07-10 20:14 UTC #15 lore raw

In reply to Jason Gunthorpe

Quoted 440 lines · Jason Gunthorpe

Quoted 6 lines · Neil Horman

Quoted 5 lines · Jason Gunthorpe
Quoted 3 lines · Neil Horman
Quoted 3 lines
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?

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 5 lines

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

1) Bind should not be sensitive to v4mapped or not, so lets drop this test:

(quoted diff) +0 -3

@@ -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

(quoted diff) +0 -2

@@ -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);
 	}