: fix multicast list when cloning sockets

PATCH In review Flavio Leitner 3 participants 5 messages Jul 30, 2007 – Aug 25, 2007

Subsystems (4)

Networking Network drivers IPv4/IPv6 TCP

lore

Share & tools

lore

Flavio Leitner

author

2007-07-30 16:12 UTC #0 lore raw

PATCH 1/1 : fix multicast list when cloning sockets

The sock_copy() function uses memcpy() to clone the socket including the struct ip_mc_socklist *mc_list pointer.

The ip_mc_drop_socket() function is called when socket is closed to free these objects leaving the other sockets cloned from the same master socket with invalid pointers.

This patch sets mc_list of cloned socket to NULL.

Signed-off-by Flavio Leitner <[email protected]>

net/ipv4/inet_connection_sock.c +2 -0

index fbe7714..8ee0f54 100644
@@ -506,6 +506,8 @@ struct sock *inet_csk_clone(struct sock *sk, const struct request_sock *req,
 		newicsk->icsk_backoff	  = 0;
 		newicsk->icsk_probes_out  = 0;
 
+		inet_sk(inet)->mc_list = NULL;
+
 		/* Deinitialize accept_queue to trap illegal accesses. */
 		memset(&newicsk->icsk_accept_queue, 0, sizeof(newicsk->icsk_accept_queue));

Signature · 4 lines

1.5.2.4

-- 
Flavio

David Miller

maintainer

2007-07-31 02:01 UTC #1 lore raw

From: Flavio Leitner <[email protected]>
Date: Mon, 30 Jul 2007 13:04:48 -0300

Quoted 11 lines

The sock_copy() function uses memcpy() to clone the socket including the struct ip_mc_socklist *mc_list pointer.
The ip_mc_drop_socket() function is called when socket is closed to free these objects leaving the other sockets cloned from the same master socket with invalid pointers.
This patch sets mc_list of cloned socket to NULL.
Signed-off-by Flavio Leitner <[email protected]>

Allowing non-datagram sockets to end up with a non-NULL inet->mc_list in the first place is a bug.

Multicast subscriptions cannot even be used with TCP and DCCP, which are the only two users of these connection oriented socket functions.

The first thing that TCP and DCCP do, in fact, for input packet processing is drop the packet if it is not unicast.

Therefore the fix really is for the inet layer to reject multicast subscription requests on sockets for which that absolutely does not make sense. There is no reason these functions in inet_connection_sock.c should need to be mindful of multicast state. :-)

Arnaldo Carvalho de Melo

2007-07-31 03:00 UTC #2 lore raw

Quoted 21 lines · David Miller

From: Flavio Leitner <[email protected]>
Date: Mon, 30 Jul 2007 13:04:48 -0300
Quoted 11 lines
The sock_copy() function uses memcpy() to clone the socket including the struct ip_mc_socklist *mc_list pointer.
The ip_mc_drop_socket() function is called when socket is closed to free these objects leaving the other sockets cloned from the same master socket with invalid pointers.
This patch sets mc_list of cloned socket to NULL.
Signed-off-by Flavio Leitner <[email protected]>
Allowing non-datagram sockets to end up with a non-NULL inet->mc_list in the first place is a bug.
Multicast subscriptions cannot even be used with TCP and DCCP, which are the only two users of these connection oriented socket functions.

Quoted 8 lines

The first thing that TCP and DCCP do, in fact, for input packet processing is drop the packet if it is not unicast.
Therefore the fix really is for the inet layer to reject multicast subscription requests on sockets for which that absolutely does not make sense. There is no reason these functions in inet_connection_sock.c should need to be mindful of multicast state. :-)

Well, we can add a BUG_ON there then 8)

Flavio, take a look at do_ip_setsockopt in net/ipv4/ip_sockglue.c, in the IP_{ADD,DROP}_MEMBERSHIP labels.

Don't forget IPV6 (net/ipv6/ipv6_sockglue.c)

- Arnaldo

Flavio Leitner

author

2007-07-31 18:32 UTC #3 lore raw

Quoted 16 lines · Arnaldo Carvalho de Melo

Quoted 6 lines · David Miller
Allowing non-datagram sockets to end up with a non-NULL inet->mc_list in the first place is a bug.
Multicast subscriptions cannot even be used with TCP and DCCP, which are the only two users of these connection oriented socket functions.
Quoted 8 lines
The first thing that TCP and DCCP do, in fact, for input packet processing is drop the packet if it is not unicast.
Therefore the fix really is for the inet layer to reject multicast subscription requests on sockets for which that absolutely does not make sense. There is no reason these functions in inet_connection_sock.c should need to be mindful of multicast state. :-)

Quoted 6 lines

Well, we can add a BUG_ON there then 8)
Flavio, take a look at do_ip_setsockopt in net/ipv4/ip_sockglue.c, in the IP_{ADD,DROP}_MEMBERSHIP labels.
Don't forget IPV6 (net/ipv6/ipv6_sockglue.c)

yes, right. What about the one below?

[NET]: Fix IP_ADD/DROP_MEMBERSHIP to handle only connectionless

Fix IP[V6]_ADD_MEMBERSHIP and IP[V6]_DROP_MEMBERSHIP to return -EPROTO for connection oriented sockets.

Signed-off-by Flavio Leitner <[email protected]>

net/ipv4/ip_sockglue.c +4 -0

index 4d54457..6b420ae 100644
@@ -625,6 +625,10 @@ static int do_ip_setsockopt(struct sock *sk, int level,
 	{
 		struct ip_mreqn mreq;
 
+		err = -EPROTO;
+		if (inet_sk(sk)->is_icsk)
+			break;
+
 		if (optlen < sizeof(struct ip_mreq))
 			goto e_inval;
 		err = -EFAULT;

net/ipv6/ipv6_sockglue.c +4 -0

index d684639..350e584 100644
@@ -554,6 +554,10 @@ done:
 	{
 		struct ipv6_mreq mreq;
 
+		retv = -EPROTO;
+		if (inet_sk(sk)->is_icsk)
+			break;
+
 		retv = -EFAULT;
 		if (copy_from_user(&mreq, optval, sizeof(struct ipv6_mreq)))
 			break;

Signature · 4 lines

1.5.2.4

-- 
Flavio

David Miller

maintainer

2007-08-25 05:16 UTC #4 lore raw

From: Flavio Leitner <[email protected]>
Date: Tue, 31 Jul 2007 15:29:40 -0300

Quoted 24 lines · Arnaldo Carvalho de Melo

Quoted 16 lines · David Miller
Quoted 6 lines
Allowing non-datagram sockets to end up with a non-NULL inet->mc_list in the first place is a bug.
Multicast subscriptions cannot even be used with TCP and DCCP, which are the only two users of these connection oriented socket functions.
Quoted 8 lines
The first thing that TCP and DCCP do, in fact, for input packet processing is drop the packet if it is not unicast.
Therefore the fix really is for the inet layer to reject multicast subscription requests on sockets for which that absolutely does not make sense. There is no reason these functions in inet_connection_sock.c should need to be mindful of multicast state. :-)
Quoted 6 lines
Well, we can add a BUG_ON there then 8)
Flavio, take a look at do_ip_setsockopt in net/ipv4/ip_sockglue.c, in the IP_{ADD,DROP}_MEMBERSHIP labels.
Don't forget IPV6 (net/ipv6/ipv6_sockglue.c)

Quoted 8 lines

yes, right. What about the one below?
[NET]: Fix IP_ADD/DROP_MEMBERSHIP to handle only connectionless
Fix IP[V6]_ADD_MEMBERSHIP and IP[V6]_DROP_MEMBERSHIP to return -EPROTO for connection oriented sockets.
Signed-off-by Flavio Leitner <[email protected]>

This looks great, patch applied.

Thanks!