* [PATCH v2 net] packet: Do not call fanout_release from atomic contexts
@ 2017-02-15 16:43 Anoob Soman
2017-02-15 17:15 ` kbuild test robot
2017-02-15 17:41 ` Eric Dumazet
0 siblings, 2 replies; 3+ messages in thread
From: Anoob Soman @ 2017-02-15 16:43 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, Anoob Soman
Commit 6664498280cf ("packet: call fanout_release, while UNREGISTERING a
netdev"), unfortunately, introduced the following issues.
1. calling mutex_lock(&fanout_mutex) (fanout_release()) from inside
rcu_read-side critical section. rcu_read_lock disables preemption, most often,
which prohibits calling sleeping functions.
[ ] include/linux/rcupdate.h:560 Illegal context switch in RCU read-side critical section!
[ ]
[ ] rcu_scheduler_active = 1, debug_locks = 0
[ ] 4 locks held by ovs-vswitchd/1969:
[ ] #0: (cb_lock){++++++}, at: [<ffffffff8158a6c9>] genl_rcv+0x19/0x40
[ ] #1: (ovs_mutex){+.+.+.}, at: [<ffffffffa04878ca>] ovs_vport_cmd_del+0x4a/0x100 [openvswitch]
[ ] #2: (rtnl_mutex){+.+.+.}, at: [<ffffffff81564157>] rtnl_lock+0x17/0x20
[ ] #3: (rcu_read_lock){......}, at: [<ffffffff81614165>] packet_notifier+0x5/0x3f0
[ ]
[ ] Call Trace:
[ ] [<ffffffff813770c1>] dump_stack+0x85/0xc4
[ ] [<ffffffff810c9077>] lockdep_rcu_suspicious+0x107/0x110
[ ] [<ffffffff810a2da7>] ___might_sleep+0x57/0x210
[ ] [<ffffffff810a2fd0>] __might_sleep+0x70/0x90
[ ] [<ffffffff8162e80c>] mutex_lock_nested+0x3c/0x3a0
[ ] [<ffffffff810de93f>] ? vprintk_default+0x1f/0x30
[ ] [<ffffffff81186e88>] ? printk+0x4d/0x4f
[ ] [<ffffffff816106dd>] fanout_release+0x1d/0xe0
[ ] [<ffffffff81614459>] packet_notifier+0x2f9/0x3f0
2. calling mutex_lock(&fanout_mutex) inside spin_lock(&po->bind_lock).
"sleeping function called from invalid context"
[ ] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:620
[ ] in_atomic(): 1, irqs_disabled(): 0, pid: 1969, name: ovs-vswitchd
[ ] INFO: lockdep is turned off.
[ ] Call Trace:
[ ] [<ffffffff813770c1>] dump_stack+0x85/0xc4
[ ] [<ffffffff810a2f52>] ___might_sleep+0x202/0x210
[ ] [<ffffffff810a2fd0>] __might_sleep+0x70/0x90
[ ] [<ffffffff8162e80c>] mutex_lock_nested+0x3c/0x3a0
[ ] [<ffffffff816106dd>] fanout_release+0x1d/0xe0
[ ] [<ffffffff81614459>] packet_notifier+0x2f9/0x3f0
3. calling dev_remove_pack(&fanout->prot_hook), from inside
spin_lock(&po->bind_lock) or rcu_read-side critical-section. dev_remove_pack()
-> synchronize_net(), which might sleep.
[ ] BUG: scheduling while atomic: ovs-vswitchd/1969/0x00000002
[ ] INFO: lockdep is turned off.
[ ] Call Trace:
[ ] [<ffffffff813770c1>] dump_stack+0x85/0xc4
[ ] [<ffffffff81186274>] __schedule_bug+0x64/0x73
[ ] [<ffffffff8162b8cb>] __schedule+0x6b/0xd10
[ ] [<ffffffff8162c5db>] schedule+0x6b/0x80
[ ] [<ffffffff81630b1d>] schedule_timeout+0x38d/0x410
[ ] [<ffffffff810ea3fd>] synchronize_sched_expedited+0x53d/0x810
[ ] [<ffffffff810ea6de>] synchronize_rcu_expedited+0xe/0x10
[ ] [<ffffffff8154eab5>] synchronize_net+0x35/0x50
[ ] [<ffffffff8154eae3>] dev_remove_pack+0x13/0x20
[ ] [<ffffffff8161077e>] fanout_release+0xbe/0xe0
[ ] [<ffffffff81614459>] packet_notifier+0x2f9/0x3f0
4. fanout_release() races with calls from different CPU.
To fix the above problems, remove the call to fanout_release() under
rcu_read_lock(). Instead, call __dev_remove_pack(&fanout->prot_hook) and
netdev_run_todo will be happy that &dev->ptype_specific list is empty. In order
to achieve this, I moved dev_{add,remove}_pack() out of fanout_{add,release} to
__fanout_{link,unlink}. So, call to {,__}unregister_prot_hook() will make sure
fanout->prot_hook is removed as well.
Fixes: 6664498280cf ("packet: call fanout_release, while UNREGISTERING a netdev")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
Changes in v2:
- Incorporated Eric's suggestion to do fanout_release_data() and kfree() after
synchronize_net()
net/packet/af_packet.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index d56ee46..af29510 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1497,6 +1497,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po)
f->arr[f->num_members] = sk;
smp_wmb();
f->num_members++;
+ if (f->num_members == 1)
+ dev_add_pack(&f->prot_hook);
spin_unlock(&f->lock);
}
@@ -1513,6 +1515,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po)
BUG_ON(i >= f->num_members);
f->arr[i] = f->arr[f->num_members - 1];
f->num_members--;
+ if (f->num_members == 0)
+ __dev_remove_pack(&f->prot_hook);
spin_unlock(&f->lock);
}
@@ -1687,7 +1691,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
match->prot_hook.func = packet_rcv_fanout;
match->prot_hook.af_packet_priv = match;
match->prot_hook.id_match = match_fanout_group;
- dev_add_pack(&match->prot_hook);
list_add(&match->list, &fanout_list);
}
err = -EINVAL;
@@ -1712,10 +1715,16 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
return err;
}
-static void fanout_release(struct sock *sk)
+/* If pkt_sk(sk)->fanout->sk_ref is zero, this functuon removes
+ * pkt_sk(sk)->fanout from fanout_list and returns pkt_sk(sk)->fanout.
+ * It is the responsibility of the caller to call fanout_release_data() and
+ * free the returned packet_fanout (after synchronize_net())
+ */
+static struct packet_fanout *fanout_release(struct sock *sk)
{
struct packet_sock *po = pkt_sk(sk);
struct packet_fanout *f;
+ bool ret_fanout = false;
f = po->fanout;
if (!f)
@@ -1726,14 +1735,14 @@ static void fanout_release(struct sock *sk)
if (atomic_dec_and_test(&f->sk_ref)) {
list_del(&f->list);
- dev_remove_pack(&f->prot_hook);
- fanout_release_data(f);
- kfree(f);
+ ret_fanout = true;
}
mutex_unlock(&fanout_mutex);
if (po->rollover)
kfree_rcu(po->rollover, rcu);
+
+ return ret_fanout ? f : NULL;
}
static bool packet_extra_vlan_len_allowed(const struct net_device *dev,
@@ -2907,6 +2916,7 @@ static int packet_release(struct socket *sock)
{
struct sock *sk = sock->sk;
struct packet_sock *po;
+ struct packet_fanout *f;
struct net *net;
union tpacket_req_u req_u;
@@ -2946,9 +2956,15 @@ static int packet_release(struct socket *sock)
packet_set_ring(sk, &req_u, 1, 1);
}
- fanout_release(sk);
+ f = fanout_release(sk);
synchronize_net();
+
+ if (f) {
+ fanout_release_data(f);
+ kfree(f);
+ }
+
/*
* Now the socket is dead. No more input will appear.
*/
@@ -3900,7 +3916,6 @@ static int packet_notifier(struct notifier_block *this,
}
if (msg == NETDEV_UNREGISTER) {
packet_cached_dev_reset(po);
- fanout_release(sk);
po->ifindex = -1;
if (po->prot_hook.dev)
dev_put(po->prot_hook.dev);
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 net] packet: Do not call fanout_release from atomic contexts
2017-02-15 16:43 [PATCH v2 net] packet: Do not call fanout_release from atomic contexts Anoob Soman
@ 2017-02-15 17:15 ` kbuild test robot
2017-02-15 17:41 ` Eric Dumazet
1 sibling, 0 replies; 3+ messages in thread
From: kbuild test robot @ 2017-02-15 17:15 UTC (permalink / raw)
To: Anoob Soman; +Cc: kbuild-all, netdev, davem, eric.dumazet, Anoob Soman
[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]
Hi Anoob,
[auto build test WARNING on net-next/master]
[also build test WARNING on v4.10-rc8]
[cannot apply to net/master next-20170215]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Anoob-Soman/packet-Do-not-call-fanout_release-from-atomic-contexts/20170216-004744
config: x86_64-randconfig-x008-201707 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
net/packet/af_packet.c: In function 'fanout_release':
>> net/packet/af_packet.c:1739:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type]
return;
^~~~~~
net/packet/af_packet.c:1731:30: note: declared here
static struct packet_fanout *fanout_release(struct sock *sk)
^~~~~~~~~~~~~~
vim +/return +1739 net/packet/af_packet.c
dc99f600 David S. Miller 2011-07-05 1723 return err;
dc99f600 David S. Miller 2011-07-05 1724 }
dc99f600 David S. Miller 2011-07-05 1725
53d5c353 Anoob Soman 2017-02-15 1726 /* If pkt_sk(sk)->fanout->sk_ref is zero, this functuon removes
53d5c353 Anoob Soman 2017-02-15 1727 * pkt_sk(sk)->fanout from fanout_list and returns pkt_sk(sk)->fanout.
53d5c353 Anoob Soman 2017-02-15 1728 * It is the responsibility of the caller to call fanout_release_data() and
53d5c353 Anoob Soman 2017-02-15 1729 * free the returned packet_fanout (after synchronize_net())
53d5c353 Anoob Soman 2017-02-15 1730 */
53d5c353 Anoob Soman 2017-02-15 1731 static struct packet_fanout *fanout_release(struct sock *sk)
dc99f600 David S. Miller 2011-07-05 1732 {
dc99f600 David S. Miller 2011-07-05 1733 struct packet_sock *po = pkt_sk(sk);
dc99f600 David S. Miller 2011-07-05 1734 struct packet_fanout *f;
53d5c353 Anoob Soman 2017-02-15 1735 bool ret_fanout = false;
dc99f600 David S. Miller 2011-07-05 1736
dc99f600 David S. Miller 2011-07-05 1737 f = po->fanout;
dc99f600 David S. Miller 2011-07-05 1738 if (!f)
dc99f600 David S. Miller 2011-07-05 @1739 return;
dc99f600 David S. Miller 2011-07-05 1740
fff3321d Pavel Emelyanov 2012-08-16 1741 mutex_lock(&fanout_mutex);
dc99f600 David S. Miller 2011-07-05 1742 po->fanout = NULL;
dc99f600 David S. Miller 2011-07-05 1743
dc99f600 David S. Miller 2011-07-05 1744 if (atomic_dec_and_test(&f->sk_ref)) {
dc99f600 David S. Miller 2011-07-05 1745 list_del(&f->list);
53d5c353 Anoob Soman 2017-02-15 1746 ret_fanout = true;
dc99f600 David S. Miller 2011-07-05 1747 }
:::::: The code at line 1739 was first introduced by commit
:::::: dc99f600698dcac69b8f56dda9a8a00d645c5ffc packet: Add fanout support.
:::::: TO: David S. Miller <davem@davemloft.net>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31218 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 net] packet: Do not call fanout_release from atomic contexts
2017-02-15 16:43 [PATCH v2 net] packet: Do not call fanout_release from atomic contexts Anoob Soman
2017-02-15 17:15 ` kbuild test robot
@ 2017-02-15 17:41 ` Eric Dumazet
1 sibling, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2017-02-15 17:41 UTC (permalink / raw)
To: Anoob Soman; +Cc: netdev, davem
On Wed, 2017-02-15 at 16:43 +0000, Anoob Soman wrote:
>
> net/packet/af_packet.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index d56ee46..af29510 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1497,6 +1497,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po)
> f->arr[f->num_members] = sk;
> smp_wmb();
> f->num_members++;
> + if (f->num_members == 1)
> + dev_add_pack(&f->prot_hook);
> spin_unlock(&f->lock);
> }
>
> @@ -1513,6 +1515,8 @@ static void __fanout_unlink(struct sock *sk, struct packet_sock *po)
> BUG_ON(i >= f->num_members);
> f->arr[i] = f->arr[f->num_members - 1];
> f->num_members--;
> + if (f->num_members == 0)
> + __dev_remove_pack(&f->prot_hook);
> spin_unlock(&f->lock);
> }
>
> @@ -1687,7 +1691,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> match->prot_hook.func = packet_rcv_fanout;
> match->prot_hook.af_packet_priv = match;
> match->prot_hook.id_match = match_fanout_group;
> - dev_add_pack(&match->prot_hook);
> list_add(&match->list, &fanout_list);
> }
> err = -EINVAL;
> @@ -1712,10 +1715,16 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> return err;
> }
>
> -static void fanout_release(struct sock *sk)
> +/* If pkt_sk(sk)->fanout->sk_ref is zero, this functuon removes
> + * pkt_sk(sk)->fanout from fanout_list and returns pkt_sk(sk)->fanout.
> + * It is the responsibility of the caller to call fanout_release_data() and
> + * free the returned packet_fanout (after synchronize_net())
> + */
> +static struct packet_fanout *fanout_release(struct sock *sk)
> {
> struct packet_sock *po = pkt_sk(sk);
> struct packet_fanout *f;
> + bool ret_fanout = false;
No need for this new variable.
>
> f = po->fanout;
> if (!f)
> @@ -1726,14 +1735,14 @@ static void fanout_release(struct sock *sk)
>
> if (atomic_dec_and_test(&f->sk_ref)) {
> list_del(&f->list);
> - dev_remove_pack(&f->prot_hook);
> - fanout_release_data(f);
> - kfree(f);
> + ret_fanout = true;
> }
} else {
f = NULL;
}
> mutex_unlock(&fanout_mutex);
>
> if (po->rollover)
> kfree_rcu(po->rollover, rcu);
> +
> + return ret_fanout ? f : NULL;
return f;
> }
>
>
Otherwise, this look good.
But make sure to respin your patch based on latest net tree.
af_packet.c got a recent fix.
Thanks
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-15 17:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15 16:43 [PATCH v2 net] packet: Do not call fanout_release from atomic contexts Anoob Soman
2017-02-15 17:15 ` kbuild test robot
2017-02-15 17:41 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.