* [Patch net v2] ipv4: fix a potential deadlock in mcast getsockopt() path
@ 2015-11-03 23:41 Cong Wang
2015-11-04 19:04 ` Marcelo Ricardo Leitner
2015-11-05 2:30 ` David Miller
0 siblings, 2 replies; 3+ messages in thread
From: Cong Wang @ 2015-11-03 23:41 UTC (permalink / raw)
To: netdev; +Cc: sasha.levin, Cong Wang, Marcelo Ricardo Leitner
Sasha reported the following lockdep warning:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(sk_lock-AF_INET);
lock(rtnl_mutex);
lock(sk_lock-AF_INET);
lock(rtnl_mutex);
This is due to that for IP_MSFILTER and MCAST_MSFILTER, we take
rtnl lock before the socket lock in setsockopt() path, but take
the socket lock before rtnl lock in getsockopt() path. All the
rest optnames are setsockopt()-only.
Fix this by aligning the getsockopt() path with the setsockopt()
path, so that all mcast socket path would be locked in the same
order.
Note, IPv6 part is different where rtnl lock is not held.
Fixes: 54ff9ef36bdf ("ipv4, ipv6: kill ip_mc_{join, leave}_group and ipv6_sock_mc_{join, drop}")
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv4/igmp.c | 12 ++++--------
net/ipv4/ip_sockglue.c | 45 ++++++++++++++++++++++++++++++---------------
2 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index d38b8b6..a2429b7 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -2392,11 +2392,11 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
struct ip_sf_socklist *psl;
struct net *net = sock_net(sk);
+ ASSERT_RTNL();
+
if (!ipv4_is_multicast(addr))
return -EINVAL;
- rtnl_lock();
-
imr.imr_multiaddr.s_addr = msf->imsf_multiaddr;
imr.imr_address.s_addr = msf->imsf_interface;
imr.imr_ifindex = 0;
@@ -2417,7 +2417,6 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
goto done;
msf->imsf_fmode = pmc->sfmode;
psl = rtnl_dereference(pmc->sflist);
- rtnl_unlock();
if (!psl) {
len = 0;
count = 0;
@@ -2436,7 +2435,6 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
return -EFAULT;
return 0;
done:
- rtnl_unlock();
return err;
}
@@ -2450,6 +2448,8 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
struct inet_sock *inet = inet_sk(sk);
struct ip_sf_socklist *psl;
+ ASSERT_RTNL();
+
psin = (struct sockaddr_in *)&gsf->gf_group;
if (psin->sin_family != AF_INET)
return -EINVAL;
@@ -2457,8 +2457,6 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
if (!ipv4_is_multicast(addr))
return -EINVAL;
- rtnl_lock();
-
err = -EADDRNOTAVAIL;
for_each_pmc_rtnl(inet, pmc) {
@@ -2470,7 +2468,6 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
goto done;
gsf->gf_fmode = pmc->sfmode;
psl = rtnl_dereference(pmc->sflist);
- rtnl_unlock();
count = psl ? psl->sl_count : 0;
copycount = count < gsf->gf_numsrc ? count : gsf->gf_numsrc;
gsf->gf_numsrc = count;
@@ -2490,7 +2487,6 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
}
return 0;
done:
- rtnl_unlock();
return err;
}
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index c3c359a..5f73a7c 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -1251,11 +1251,22 @@ EXPORT_SYMBOL(compat_ip_setsockopt);
* the _received_ ones. The set sets the _sent_ ones.
*/
+static bool getsockopt_needs_rtnl(int optname)
+{
+ switch (optname) {
+ case IP_MSFILTER:
+ case MCAST_MSFILTER:
+ return true;
+ }
+ return false;
+}
+
static int do_ip_getsockopt(struct sock *sk, int level, int optname,
char __user *optval, int __user *optlen, unsigned int flags)
{
struct inet_sock *inet = inet_sk(sk);
- int val;
+ bool needs_rtnl = getsockopt_needs_rtnl(optname);
+ int val, err = 0;
int len;
if (level != SOL_IP)
@@ -1269,6 +1280,8 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
if (len < 0)
return -EINVAL;
+ if (needs_rtnl)
+ rtnl_lock();
lock_sock(sk);
switch (optname) {
@@ -1386,39 +1399,35 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
case IP_MSFILTER:
{
struct ip_msfilter msf;
- int err;
if (len < IP_MSFILTER_SIZE(0)) {
- release_sock(sk);
- return -EINVAL;
+ err = -EINVAL;
+ goto out;
}
if (copy_from_user(&msf, optval, IP_MSFILTER_SIZE(0))) {
- release_sock(sk);
- return -EFAULT;
+ err = -EFAULT;
+ goto out;
}
err = ip_mc_msfget(sk, &msf,
(struct ip_msfilter __user *)optval, optlen);
- release_sock(sk);
- return err;
+ goto out;
}
case MCAST_MSFILTER:
{
struct group_filter gsf;
- int err;
if (len < GROUP_FILTER_SIZE(0)) {
- release_sock(sk);
- return -EINVAL;
+ err = -EINVAL;
+ goto out;
}
if (copy_from_user(&gsf, optval, GROUP_FILTER_SIZE(0))) {
- release_sock(sk);
- return -EFAULT;
+ err = -EFAULT;
+ goto out;
}
err = ip_mc_gsfget(sk, &gsf,
(struct group_filter __user *)optval,
optlen);
- release_sock(sk);
- return err;
+ goto out;
}
case IP_MULTICAST_ALL:
val = inet->mc_all;
@@ -1485,6 +1494,12 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
return -EFAULT;
}
return 0;
+
+out:
+ release_sock(sk);
+ if (needs_rtnl)
+ rtnl_unlock();
+ return err;
}
int ip_getsockopt(struct sock *sk, int level,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Patch net v2] ipv4: fix a potential deadlock in mcast getsockopt() path
2015-11-03 23:41 [Patch net v2] ipv4: fix a potential deadlock in mcast getsockopt() path Cong Wang
@ 2015-11-04 19:04 ` Marcelo Ricardo Leitner
2015-11-05 2:30 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: Marcelo Ricardo Leitner @ 2015-11-04 19:04 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: sasha.levin
Em 03-11-2015 21:41, Cong Wang escreveu:
> Sasha reported the following lockdep warning:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(sk_lock-AF_INET);
> lock(rtnl_mutex);
> lock(sk_lock-AF_INET);
> lock(rtnl_mutex);
>
> This is due to that for IP_MSFILTER and MCAST_MSFILTER, we take
> rtnl lock before the socket lock in setsockopt() path, but take
> the socket lock before rtnl lock in getsockopt() path. All the
> rest optnames are setsockopt()-only.
>
> Fix this by aligning the getsockopt() path with the setsockopt()
> path, so that all mcast socket path would be locked in the same
> order.
>
> Note, IPv6 part is different where rtnl lock is not held.
>
> Fixes: 54ff9ef36bdf ("ipv4, ipv6: kill ip_mc_{join, leave}_group and ipv6_sock_mc_{join, drop}")
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Thanks
> ---
> net/ipv4/igmp.c | 12 ++++--------
> net/ipv4/ip_sockglue.c | 45 ++++++++++++++++++++++++++++++---------------
> 2 files changed, 34 insertions(+), 23 deletions(-)
>
> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index d38b8b6..a2429b7 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -2392,11 +2392,11 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
> struct ip_sf_socklist *psl;
> struct net *net = sock_net(sk);
>
> + ASSERT_RTNL();
> +
> if (!ipv4_is_multicast(addr))
> return -EINVAL;
>
> - rtnl_lock();
> -
> imr.imr_multiaddr.s_addr = msf->imsf_multiaddr;
> imr.imr_address.s_addr = msf->imsf_interface;
> imr.imr_ifindex = 0;
> @@ -2417,7 +2417,6 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
> goto done;
> msf->imsf_fmode = pmc->sfmode;
> psl = rtnl_dereference(pmc->sflist);
> - rtnl_unlock();
> if (!psl) {
> len = 0;
> count = 0;
> @@ -2436,7 +2435,6 @@ int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf,
> return -EFAULT;
> return 0;
> done:
> - rtnl_unlock();
> return err;
> }
>
> @@ -2450,6 +2448,8 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
> struct inet_sock *inet = inet_sk(sk);
> struct ip_sf_socklist *psl;
>
> + ASSERT_RTNL();
> +
> psin = (struct sockaddr_in *)&gsf->gf_group;
> if (psin->sin_family != AF_INET)
> return -EINVAL;
> @@ -2457,8 +2457,6 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
> if (!ipv4_is_multicast(addr))
> return -EINVAL;
>
> - rtnl_lock();
> -
> err = -EADDRNOTAVAIL;
>
> for_each_pmc_rtnl(inet, pmc) {
> @@ -2470,7 +2468,6 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
> goto done;
> gsf->gf_fmode = pmc->sfmode;
> psl = rtnl_dereference(pmc->sflist);
> - rtnl_unlock();
> count = psl ? psl->sl_count : 0;
> copycount = count < gsf->gf_numsrc ? count : gsf->gf_numsrc;
> gsf->gf_numsrc = count;
> @@ -2490,7 +2487,6 @@ int ip_mc_gsfget(struct sock *sk, struct group_filter *gsf,
> }
> return 0;
> done:
> - rtnl_unlock();
> return err;
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index c3c359a..5f73a7c 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -1251,11 +1251,22 @@ EXPORT_SYMBOL(compat_ip_setsockopt);
> * the _received_ ones. The set sets the _sent_ ones.
> */
>
> +static bool getsockopt_needs_rtnl(int optname)
> +{
> + switch (optname) {
> + case IP_MSFILTER:
> + case MCAST_MSFILTER:
> + return true;
> + }
> + return false;
> +}
> +
> static int do_ip_getsockopt(struct sock *sk, int level, int optname,
> char __user *optval, int __user *optlen, unsigned int flags)
> {
> struct inet_sock *inet = inet_sk(sk);
> - int val;
> + bool needs_rtnl = getsockopt_needs_rtnl(optname);
> + int val, err = 0;
> int len;
>
> if (level != SOL_IP)
> @@ -1269,6 +1280,8 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
> if (len < 0)
> return -EINVAL;
>
> + if (needs_rtnl)
> + rtnl_lock();
> lock_sock(sk);
>
> switch (optname) {
> @@ -1386,39 +1399,35 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
> case IP_MSFILTER:
> {
> struct ip_msfilter msf;
> - int err;
>
> if (len < IP_MSFILTER_SIZE(0)) {
> - release_sock(sk);
> - return -EINVAL;
> + err = -EINVAL;
> + goto out;
> }
> if (copy_from_user(&msf, optval, IP_MSFILTER_SIZE(0))) {
> - release_sock(sk);
> - return -EFAULT;
> + err = -EFAULT;
> + goto out;
> }
> err = ip_mc_msfget(sk, &msf,
> (struct ip_msfilter __user *)optval, optlen);
> - release_sock(sk);
> - return err;
> + goto out;
> }
> case MCAST_MSFILTER:
> {
> struct group_filter gsf;
> - int err;
>
> if (len < GROUP_FILTER_SIZE(0)) {
> - release_sock(sk);
> - return -EINVAL;
> + err = -EINVAL;
> + goto out;
> }
> if (copy_from_user(&gsf, optval, GROUP_FILTER_SIZE(0))) {
> - release_sock(sk);
> - return -EFAULT;
> + err = -EFAULT;
> + goto out;
> }
> err = ip_mc_gsfget(sk, &gsf,
> (struct group_filter __user *)optval,
> optlen);
> - release_sock(sk);
> - return err;
> + goto out;
> }
> case IP_MULTICAST_ALL:
> val = inet->mc_all;
> @@ -1485,6 +1494,12 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
> return -EFAULT;
> }
> return 0;
> +
> +out:
> + release_sock(sk);
> + if (needs_rtnl)
> + rtnl_unlock();
> + return err;
> }
>
> int ip_getsockopt(struct sock *sk, int level,
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Patch net v2] ipv4: fix a potential deadlock in mcast getsockopt() path
2015-11-03 23:41 [Patch net v2] ipv4: fix a potential deadlock in mcast getsockopt() path Cong Wang
2015-11-04 19:04 ` Marcelo Ricardo Leitner
@ 2015-11-05 2:30 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2015-11-05 2:30 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, sasha.levin, marcelo.leitner
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 3 Nov 2015 15:41:16 -0800
> Sasha reported the following lockdep warning:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(sk_lock-AF_INET);
> lock(rtnl_mutex);
> lock(sk_lock-AF_INET);
> lock(rtnl_mutex);
>
> This is due to that for IP_MSFILTER and MCAST_MSFILTER, we take
> rtnl lock before the socket lock in setsockopt() path, but take
> the socket lock before rtnl lock in getsockopt() path. All the
> rest optnames are setsockopt()-only.
>
> Fix this by aligning the getsockopt() path with the setsockopt()
> path, so that all mcast socket path would be locked in the same
> order.
>
> Note, IPv6 part is different where rtnl lock is not held.
>
> Fixes: 54ff9ef36bdf ("ipv4, ipv6: kill ip_mc_{join, leave}_group and ipv6_sock_mc_{join, drop}")
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
I don't like conditional locking, but I can't think of a better
way to suggest fixing this, so applied.
And queued up for -stable as well.
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-11-05 2:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-03 23:41 [Patch net v2] ipv4: fix a potential deadlock in mcast getsockopt() path Cong Wang
2015-11-04 19:04 ` Marcelo Ricardo Leitner
2015-11-05 2:30 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).