netdev - Re: [PATCH net-next 1/2] ipv6: annotate data-races around np->mcast

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 8 Dec 2023 09:06:43 -0700
From: David Ahern <dsahern@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>, "David S . Miller"
 <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 1/2] ipv6: annotate data-races around
 np->mcast_oif

On 12/8/23 3:12 AM, Eric Dumazet wrote:
> np->mcast_oif is read locklessly in some contexts.
> 
> Make all accesses to this field lockless, adding appropriate
> annotations.
> 
> This also makes setsockopt( IPV6_MULTICAST_IF ) lockless.
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  net/dccp/ipv6.c                 |  2 +-
>  net/ipv6/datagram.c             |  4 +-
>  net/ipv6/icmp.c                 |  4 +-
>  net/ipv6/ipv6_sockglue.c        | 74 +++++++++++++++++----------------
>  net/ipv6/ping.c                 |  4 +-
>  net/ipv6/raw.c                  |  2 +-
>  net/ipv6/tcp_ipv6.c             |  2 +-
>  net/ipv6/udp.c                  |  2 +-
>  net/l2tp/l2tp_ip6.c             |  2 +-
>  net/netfilter/ipvs/ip_vs_sync.c |  2 +-
>  net/rds/tcp_listen.c            |  2 +-
>  11 files changed, 51 insertions(+), 49 deletions(-)
> 


> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 7d661735cb9d519ab4691979f30365acda0a28c3..fe7e96e69960c013e84b48242e309525f7f618da 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -509,6 +509,34 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>  		if (optlen < sizeof(int))
>  			return -EINVAL;
>  		return ip6_sock_set_addr_preferences(sk, val);
> +	case IPV6_MULTICAST_IF:
> +		if (sk->sk_type == SOCK_STREAM)
> +			return -ENOPROTOOPT;
> +		if (optlen < sizeof(int))
> +			return -EINVAL;
> +		if (val) {
> +			struct net_device *dev;
> +			int bound_dev_if, midx;
> +
> +			rcu_read_lock();
> +
> +			dev = dev_get_by_index_rcu(net, val);
> +			if (!dev) {
> +				rcu_read_unlock();
> +				return -ENODEV;
> +			}
> +			midx = l3mdev_master_ifindex_rcu(dev);
> +
> +			rcu_read_unlock();
> +
> +			bound_dev_if = READ_ONCE(sk->sk_bound_dev_if);

you snuck in an extra change with that code move.

Reviewed-by: David Ahern <dsahern@...nel.org>


> +			if (bound_dev_if &&
> +			    bound_dev_if != val &&
> +			    (!midx || midx != bound_dev_if))
> +				return -EINVAL;
> +		}
> +		WRITE_ONCE(np->mcast_oif, val);
> +		return 0;
>  	}
>  	if (needs_rtnl)
>  		rtnl_lock();
> @@ -860,36 +888,6 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>  		break;
>  	}
>  
> -	case IPV6_MULTICAST_IF:
> -		if (sk->sk_type == SOCK_STREAM)
> -			break;
> -		if (optlen < sizeof(int))
> -			goto e_inval;
> -
> -		if (val) {
> -			struct net_device *dev;
> -			int midx;
> -
> -			rcu_read_lock();
> -
> -			dev = dev_get_by_index_rcu(net, val);
> -			if (!dev) {
> -				rcu_read_unlock();
> -				retv = -ENODEV;
> -				break;
> -			}
> -			midx = l3mdev_master_ifindex_rcu(dev);
> -
> -			rcu_read_unlock();
> -
> -			if (sk->sk_bound_dev_if &&
> -			    sk->sk_bound_dev_if != val &&
> -			    (!midx || midx != sk->sk_bound_dev_if))
> -				goto e_inval;
> -		}
> -		np->mcast_oif = val;
> -		retv = 0;
> -		break;
>  	case IPV6_ADD_MEMBERSHIP:
>  	case IPV6_DROP_MEMBERSHIP:
>  	{