Skip to content

Commit

Permalink
atlantic: fix deadlock at aq_nic_stop
Browse files Browse the repository at this point in the history 
[ Upstream commit 6960d13 ]

NIC is stopped with rtnl_lock held, and during the stop it cancels the
'service_task' work and free irqs.

However, if CONFIG_MACSEC is set, rtnl_lock is acquired both from
aq_nic_service_task and aq_linkstate_threaded_isr. Then a deadlock
happens if aq_nic_stop tries to cancel/disable them when they've already
started their execution.

As the deadlock is caused by rtnl_lock, it causes many other processes
to stall, not only atlantic related stuff.

Fix it by introducing a mutex that protects each NIC's macsec related
data, and locking it instead of the rtnl_lock from the service task and
the threaded IRQ.

Before this patch, all macsec data was protected with rtnl_lock, but
maybe not all of it needs to be protected. With this new mutex, further
efforts can be made to limit the protected data only to that which
requires it. However, probably it doesn't worth it because all macsec's
data accesses are infrequent, and almost all are done from macsec_ops
or ethtool callbacks, called holding rtnl_lock, so macsec_mutex won't
never be much contended.

The issue appeared repeteadly attaching and deattaching the NIC to a
bond interface. Doing that after this patch I cannot reproduce the bug.

Fixes: 62c1c2e ("net: atlantic: MACSec offload skeleton")
Reported-by: Li Liang <liali@redhat.com>
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
Reviewed-by: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
@ihuguet @gregkh
ihuguet authored and gregkh committed Nov 3, 2022
1 parent 4e2cbc1 commit 6bb2322
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 24 deletions.
96 changes: 72 additions & 24 deletions drivers/net/ethernet/aquantia/atlantic/aq_macsec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1451,26 +1451,57 @@ static void aq_check_txsa_expiration(struct aq_nic_s *nic)
egress_sa_threshold_expired);
}

#define AQ_LOCKED_MDO_DEF(mdo) \
static int aq_locked_mdo_##mdo(struct macsec_context *ctx) \
{ \
struct aq_nic_s *nic = netdev_priv(ctx->netdev); \
int ret; \
mutex_lock(&nic->macsec_mutex); \
ret = aq_mdo_##mdo(ctx); \
mutex_unlock(&nic->macsec_mutex); \
return ret; \
}

AQ_LOCKED_MDO_DEF(dev_open)
AQ_LOCKED_MDO_DEF(dev_stop)
AQ_LOCKED_MDO_DEF(add_secy)
AQ_LOCKED_MDO_DEF(upd_secy)
AQ_LOCKED_MDO_DEF(del_secy)
AQ_LOCKED_MDO_DEF(add_rxsc)
AQ_LOCKED_MDO_DEF(upd_rxsc)
AQ_LOCKED_MDO_DEF(del_rxsc)
AQ_LOCKED_MDO_DEF(add_rxsa)
AQ_LOCKED_MDO_DEF(upd_rxsa)
AQ_LOCKED_MDO_DEF(del_rxsa)
AQ_LOCKED_MDO_DEF(add_txsa)
AQ_LOCKED_MDO_DEF(upd_txsa)
AQ_LOCKED_MDO_DEF(del_txsa)
AQ_LOCKED_MDO_DEF(get_dev_stats)
AQ_LOCKED_MDO_DEF(get_tx_sc_stats)
AQ_LOCKED_MDO_DEF(get_tx_sa_stats)
AQ_LOCKED_MDO_DEF(get_rx_sc_stats)
AQ_LOCKED_MDO_DEF(get_rx_sa_stats)

const struct macsec_ops aq_macsec_ops = {
.mdo_dev_open = aq_mdo_dev_open,
.mdo_dev_stop = aq_mdo_dev_stop,
.mdo_add_secy = aq_mdo_add_secy,
.mdo_upd_secy = aq_mdo_upd_secy,
.mdo_del_secy = aq_mdo_del_secy,
.mdo_add_rxsc = aq_mdo_add_rxsc,
.mdo_upd_rxsc = aq_mdo_upd_rxsc,
.mdo_del_rxsc = aq_mdo_del_rxsc,
.mdo_add_rxsa = aq_mdo_add_rxsa,
.mdo_upd_rxsa = aq_mdo_upd_rxsa,
.mdo_del_rxsa = aq_mdo_del_rxsa,
.mdo_add_txsa = aq_mdo_add_txsa,
.mdo_upd_txsa = aq_mdo_upd_txsa,
.mdo_del_txsa = aq_mdo_del_txsa,
.mdo_get_dev_stats = aq_mdo_get_dev_stats,
.mdo_get_tx_sc_stats = aq_mdo_get_tx_sc_stats,
.mdo_get_tx_sa_stats = aq_mdo_get_tx_sa_stats,
.mdo_get_rx_sc_stats = aq_mdo_get_rx_sc_stats,
.mdo_get_rx_sa_stats = aq_mdo_get_rx_sa_stats,
.mdo_dev_open = aq_locked_mdo_dev_open,
.mdo_dev_stop = aq_locked_mdo_dev_stop,
.mdo_add_secy = aq_locked_mdo_add_secy,
.mdo_upd_secy = aq_locked_mdo_upd_secy,
.mdo_del_secy = aq_locked_mdo_del_secy,
.mdo_add_rxsc = aq_locked_mdo_add_rxsc,
.mdo_upd_rxsc = aq_locked_mdo_upd_rxsc,
.mdo_del_rxsc = aq_locked_mdo_del_rxsc,
.mdo_add_rxsa = aq_locked_mdo_add_rxsa,
.mdo_upd_rxsa = aq_locked_mdo_upd_rxsa,
.mdo_del_rxsa = aq_locked_mdo_del_rxsa,
.mdo_add_txsa = aq_locked_mdo_add_txsa,
.mdo_upd_txsa = aq_locked_mdo_upd_txsa,
.mdo_del_txsa = aq_locked_mdo_del_txsa,
.mdo_get_dev_stats = aq_locked_mdo_get_dev_stats,
.mdo_get_tx_sc_stats = aq_locked_mdo_get_tx_sc_stats,
.mdo_get_tx_sa_stats = aq_locked_mdo_get_tx_sa_stats,
.mdo_get_rx_sc_stats = aq_locked_mdo_get_rx_sc_stats,
.mdo_get_rx_sa_stats = aq_locked_mdo_get_rx_sa_stats,
};

int aq_macsec_init(struct aq_nic_s *nic)
Expand All @@ -1492,6 +1523,7 @@ int aq_macsec_init(struct aq_nic_s *nic)

nic->ndev->features |= NETIF_F_HW_MACSEC;
nic->ndev->macsec_ops = &aq_macsec_ops;
mutex_init(&nic->macsec_mutex);

return 0;
}
Expand All @@ -1515,7 +1547,7 @@ int aq_macsec_enable(struct aq_nic_s *nic)
if (!nic->macsec_cfg)
return 0;

rtnl_lock();
mutex_lock(&nic->macsec_mutex);

if (nic->aq_fw_ops->send_macsec_req) {
struct macsec_cfg_request cfg = { 0 };
Expand Down Expand Up @@ -1564,7 +1596,7 @@ int aq_macsec_enable(struct aq_nic_s *nic)
ret = aq_apply_macsec_cfg(nic);

unlock:
rtnl_unlock();
mutex_unlock(&nic->macsec_mutex);
return ret;
}

Expand All @@ -1576,9 +1608,9 @@ void aq_macsec_work(struct aq_nic_s *nic)
if (!netif_carrier_ok(nic->ndev))
return;

rtnl_lock();
mutex_lock(&nic->macsec_mutex);
aq_check_txsa_expiration(nic);
rtnl_unlock();
mutex_unlock(&nic->macsec_mutex);
}

int aq_macsec_rx_sa_cnt(struct aq_nic_s *nic)
Expand All @@ -1589,21 +1621,30 @@ int aq_macsec_rx_sa_cnt(struct aq_nic_s *nic)
if (!cfg)
return 0;

mutex_lock(&nic->macsec_mutex);

for (i = 0; i < AQ_MACSEC_MAX_SC; i++) {
if (!test_bit(i, &cfg->rxsc_idx_busy))
continue;
cnt += hweight_long(cfg->aq_rxsc[i].rx_sa_idx_busy);
}

mutex_unlock(&nic->macsec_mutex);
return cnt;
}

int aq_macsec_tx_sc_cnt(struct aq_nic_s *nic)
{
int cnt;

if (!nic->macsec_cfg)
return 0;

return hweight_long(nic->macsec_cfg->txsc_idx_busy);
mutex_lock(&nic->macsec_mutex);
cnt = hweight_long(nic->macsec_cfg->txsc_idx_busy);
mutex_unlock(&nic->macsec_mutex);

return cnt;
}

int aq_macsec_tx_sa_cnt(struct aq_nic_s *nic)
Expand All @@ -1614,12 +1655,15 @@ int aq_macsec_tx_sa_cnt(struct aq_nic_s *nic)
if (!cfg)
return 0;

mutex_lock(&nic->macsec_mutex);

for (i = 0; i < AQ_MACSEC_MAX_SC; i++) {
if (!test_bit(i, &cfg->txsc_idx_busy))
continue;
cnt += hweight_long(cfg->aq_txsc[i].tx_sa_idx_busy);
}

mutex_unlock(&nic->macsec_mutex);
return cnt;
}

Expand Down Expand Up @@ -1691,6 +1735,8 @@ u64 *aq_macsec_get_stats(struct aq_nic_s *nic, u64 *data)
if (!cfg)
return data;

mutex_lock(&nic->macsec_mutex);

aq_macsec_update_stats(nic);

common_stats = &cfg->stats;
Expand Down Expand Up @@ -1773,5 +1819,7 @@ u64 *aq_macsec_get_stats(struct aq_nic_s *nic, u64 *data)

data += i;

mutex_unlock(&nic->macsec_mutex);

return data;
}
2 changes: 2 additions & 0 deletions drivers/net/ethernet/aquantia/atlantic/aq_nic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ struct aq_nic_s {
struct mutex fwreq_mutex;
#if IS_ENABLED(CONFIG_MACSEC)
struct aq_macsec_cfg *macsec_cfg;
/* mutex to protect data in macsec_cfg */
struct mutex macsec_mutex;
#endif
/* PTP support */
struct aq_ptp_s *aq_ptp;
Expand Down

0 comments on commit 6bb2322

Please sign in to comment.