Commit a53b59ec authored by Thomas Gleixner's avatar Thomas Gleixner Committed by David S. Miller

net: enic: Cure the enic api locking trainwreck

enic_dev_wait() has a BUG_ON(in_interrupt()).

Chasing the callers of enic_dev_wait() revealed the gems of enic_reset()
and enic_tx_hang_reset() which are both invoked through work queues in
order to be able to call rtnl_lock(). So far so good.

After locking rtnl both functions acquire enic::enic_api_lock which
serializes against the (ab)use from infiniband. This is where the
trainwreck starts.

enic::enic_api_lock is a spin_lock() which implicitly disables preemption,
but both functions invoke a ton of functions under that lock which can
sleep. The BUG_ON(in_interrupt()) does not trigger in that case because it
can't detect the preempt disabled condition.

This clearly has never been tested with any of the mandatory debug options
for 7+ years, which would have caught that for sure.

Cure it by adding a enic_api_busy member to struct enic, which is modified
and evaluated with enic::enic_api_lock held.

If enic_api_devcmd_proxy_by_index() observes enic::enic_api_busy as true,
it drops enic::enic_api_lock and busy waits for enic::enic_api_busy to
become false.

It would be smarter to wait for a completion of that busy period, but
enic_api_devcmd_proxy_by_index() is called with other spin locks held which
obviously can't sleep.

Remove the BUG_ON(in_interrupt()) check as well because it's incomplete and
with proper debugging enabled the problem would have been caught from the
debug checks in schedule_timeout().

Fixes: 0b038566 ("drivers/net: enic: Add an interface for USNIC to interact with firmware")
Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 2ec13cbc
...@@ -169,6 +169,7 @@ struct enic { ...@@ -169,6 +169,7 @@ struct enic {
u16 num_vfs; u16 num_vfs;
#endif #endif
spinlock_t enic_api_lock; spinlock_t enic_api_lock;
bool enic_api_busy;
struct enic_port_profile *pp; struct enic_port_profile *pp;
/* work queue cache line section */ /* work queue cache line section */
......
...@@ -34,6 +34,12 @@ int enic_api_devcmd_proxy_by_index(struct net_device *netdev, int vf, ...@@ -34,6 +34,12 @@ int enic_api_devcmd_proxy_by_index(struct net_device *netdev, int vf,
struct vnic_dev *vdev = enic->vdev; struct vnic_dev *vdev = enic->vdev;
spin_lock(&enic->enic_api_lock); spin_lock(&enic->enic_api_lock);
while (enic->enic_api_busy) {
spin_unlock(&enic->enic_api_lock);
cpu_relax();
spin_lock(&enic->enic_api_lock);
}
spin_lock_bh(&enic->devcmd_lock); spin_lock_bh(&enic->devcmd_lock);
vnic_dev_cmd_proxy_by_index_start(vdev, vf); vnic_dev_cmd_proxy_by_index_start(vdev, vf);
......
...@@ -2107,8 +2107,6 @@ static int enic_dev_wait(struct vnic_dev *vdev, ...@@ -2107,8 +2107,6 @@ static int enic_dev_wait(struct vnic_dev *vdev,
int done; int done;
int err; int err;
BUG_ON(in_interrupt());
err = start(vdev, arg); err = start(vdev, arg);
if (err) if (err)
return err; return err;
...@@ -2297,6 +2295,13 @@ static int enic_set_rss_nic_cfg(struct enic *enic) ...@@ -2297,6 +2295,13 @@ static int enic_set_rss_nic_cfg(struct enic *enic)
rss_hash_bits, rss_base_cpu, rss_enable); rss_hash_bits, rss_base_cpu, rss_enable);
} }
static void enic_set_api_busy(struct enic *enic, bool busy)
{
spin_lock(&enic->enic_api_lock);
enic->enic_api_busy = busy;
spin_unlock(&enic->enic_api_lock);
}
static void enic_reset(struct work_struct *work) static void enic_reset(struct work_struct *work)
{ {
struct enic *enic = container_of(work, struct enic, reset); struct enic *enic = container_of(work, struct enic, reset);
...@@ -2306,7 +2311,9 @@ static void enic_reset(struct work_struct *work) ...@@ -2306,7 +2311,9 @@ static void enic_reset(struct work_struct *work)
rtnl_lock(); rtnl_lock();
spin_lock(&enic->enic_api_lock); /* Stop any activity from infiniband */
enic_set_api_busy(enic, true);
enic_stop(enic->netdev); enic_stop(enic->netdev);
enic_dev_soft_reset(enic); enic_dev_soft_reset(enic);
enic_reset_addr_lists(enic); enic_reset_addr_lists(enic);
...@@ -2314,7 +2321,10 @@ static void enic_reset(struct work_struct *work) ...@@ -2314,7 +2321,10 @@ static void enic_reset(struct work_struct *work)
enic_set_rss_nic_cfg(enic); enic_set_rss_nic_cfg(enic);
enic_dev_set_ig_vlan_rewrite_mode(enic); enic_dev_set_ig_vlan_rewrite_mode(enic);
enic_open(enic->netdev); enic_open(enic->netdev);
spin_unlock(&enic->enic_api_lock);
/* Allow infiniband to fiddle with the device again */
enic_set_api_busy(enic, false);
call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev); call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
rtnl_unlock(); rtnl_unlock();
...@@ -2326,7 +2336,9 @@ static void enic_tx_hang_reset(struct work_struct *work) ...@@ -2326,7 +2336,9 @@ static void enic_tx_hang_reset(struct work_struct *work)
rtnl_lock(); rtnl_lock();
spin_lock(&enic->enic_api_lock); /* Stop any activity from infiniband */
enic_set_api_busy(enic, true);
enic_dev_hang_notify(enic); enic_dev_hang_notify(enic);
enic_stop(enic->netdev); enic_stop(enic->netdev);
enic_dev_hang_reset(enic); enic_dev_hang_reset(enic);
...@@ -2335,7 +2347,10 @@ static void enic_tx_hang_reset(struct work_struct *work) ...@@ -2335,7 +2347,10 @@ static void enic_tx_hang_reset(struct work_struct *work)
enic_set_rss_nic_cfg(enic); enic_set_rss_nic_cfg(enic);
enic_dev_set_ig_vlan_rewrite_mode(enic); enic_dev_set_ig_vlan_rewrite_mode(enic);
enic_open(enic->netdev); enic_open(enic->netdev);
spin_unlock(&enic->enic_api_lock);
/* Allow infiniband to fiddle with the device again */
enic_set_api_busy(enic, false);
call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev); call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
rtnl_unlock(); rtnl_unlock();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment