From patchwork Mon May 18 15:29:08 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Shradha Shah X-Patchwork-Id: 473425 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id E4C4B1401B5 for ; Tue, 19 May 2015 01:29:41 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754118AbbERP3h (ORCPT ); Mon, 18 May 2015 11:29:37 -0400 Received: from nbfkord-smmo04.seg.att.com ([209.65.160.86]:46148 "EHLO nbfkord-smmo04.seg.att.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753035AbbERP3b (ORCPT ); Mon, 18 May 2015 11:29:31 -0400 Received: from unknown [12.187.104.25] (EHLO webmail.solarflare.com) by nbfkord-smmo04.seg.att.com(mxl_mta-7.2.4-5) with ESMTP id b550a555.2ad0e1e02940.1958525.00-2480.10695613.nbfkord-smmo04.seg.att.com (envelope-from ); Mon, 18 May 2015 15:29:31 +0000 (UTC) X-MXL-Hash: 555a055b46dccbef-848a7bb689f4cf5d50af56d0aa34e842238b493b Received: from unknown [12.187.104.25] (EHLO webmail.solarflare.com) by nbfkord-smmo04.seg.att.com(mxl_mta-7.2.4-5) over TLS secured channel with ESMTP id 7450a555.0.1958452.00-2016.10695286.nbfkord-smmo04.seg.att.com (envelope-from ); Mon, 18 May 2015 15:29:12 +0000 (UTC) X-MXL-Hash: 555a05486940c6d5-7de1f47c70d5b78a38dd1e8c43fbc73566d77fa6 Received: from sshah-desktop.uk.level5networks.com (10.17.20.135) by webmail.SolarFlare.com (10.20.40.31) with Microsoft SMTP Server (TLS) id 14.3.158.1; Mon, 18 May 2015 08:29:10 -0700 Message-ID: <555A0544.6010000@solarflare.com> Date: Mon, 18 May 2015 16:29:08 +0100 From: Shradha Shah User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: David Miller CC: , Subject: [PATCH net-next 07/16] sfc: protect filter table against use-after-free References: <555A044A.4060202@solarflare.com> In-Reply-To: <555A044A.4060202@solarflare.com> X-Originating-IP: [10.17.20.135] X-TM-AS-Product-Ver: SMEX-10.0.0.1412-7.000.1014-21552.005 X-TM-AS-Result: No--8.061300-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-AnalysisOut: [v=2.0 cv=I4vSsqcg c=1 sm=1 a=MkjXnYnS3dyNWGSWLXxFFQ==:17 a] X-AnalysisOut: [=5ZTteq0x3j8A:10 a=3VnyBeAh6Z0A:10 a=BLceEmwcHowA:10 a=N65] X-AnalysisOut: [9UExz7-8A:10 a=zRKbQ67AAAAA:8 a=h1PgugrvaO0A:10 a=l3Zi-YK4] X-AnalysisOut: [vjWrAvqcvEcA:9 a=pILNOxqGKmIA:10] X-Spam: [F=0.2000000000; CM=0.500; S=0.200(2014051901)] X-MAIL-FROM: X-SOURCE-IP: [12.187.104.25] Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Edward Cree If MCDI timeouts are encountered during efx_ef10_filter_table_remove(), an FLR will be queued, but efx->filter_state will still be kfree()d. The queued FLR will then call efx_ef10_filter_table_restore(), which will try to use efx->filter_state. This previously caused a panic. This patch adds an rwsem to protect the existence of efx->filter_state, separately from the spinlock protecting its contents. Users which can race against efx_ef10_filter_table_remove() should down_read this rwsem. Signed-off-by: Shradha Shah --- drivers/net/ethernet/sfc/ef10.c | 19 +++++++++++++++++ drivers/net/ethernet/sfc/efx.c | 39 ++++++++++++++++++++++++++--------- drivers/net/ethernet/sfc/efx.h | 2 ++ drivers/net/ethernet/sfc/ethtool.c | 2 +- drivers/net/ethernet/sfc/net_driver.h | 5 ++++- 5 files changed, 55 insertions(+), 12 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c index 78d3236..9e2e8e1 100644 --- a/drivers/net/ethernet/sfc/ef10.c +++ b/drivers/net/ethernet/sfc/ef10.c @@ -3314,6 +3314,9 @@ fail: return rc; } +/* Caller must hold efx->filter_sem for read if race against + * efx_ef10_filter_table_remove() is possible + */ static void efx_ef10_filter_table_restore(struct efx_nic *efx) { struct efx_ef10_filter_table *table = efx->filter_state; @@ -3323,9 +3326,14 @@ static void efx_ef10_filter_table_restore(struct efx_nic *efx) bool failed = false; int rc; + WARN_ON(!rwsem_is_locked(&efx->filter_sem)); + if (!nic_data->must_restore_filters) return; + if (!table) + return; + spin_lock_bh(&efx->filter_lock); for (filter_idx = 0; filter_idx < HUNT_FILTER_TBL_ROWS; filter_idx++) { @@ -3361,6 +3369,7 @@ static void efx_ef10_filter_table_restore(struct efx_nic *efx) nic_data->must_restore_filters = false; } +/* Caller must hold efx->filter_sem for write */ static void efx_ef10_filter_table_remove(struct efx_nic *efx) { struct efx_ef10_filter_table *table = efx->filter_state; @@ -3369,6 +3378,10 @@ static void efx_ef10_filter_table_remove(struct efx_nic *efx) unsigned int filter_idx; int rc; + efx->filter_state = NULL; + if (!table) + return; + for (filter_idx = 0; filter_idx < HUNT_FILTER_TBL_ROWS; filter_idx++) { spec = efx_ef10_filter_entry_spec(table, filter_idx); if (!spec) @@ -3394,6 +3407,9 @@ static void efx_ef10_filter_table_remove(struct efx_nic *efx) kfree(table); } +/* Caller must hold efx->filter_sem for read if race against + * efx_ef10_filter_table_remove() is possible + */ static void efx_ef10_filter_sync_rx_mode(struct efx_nic *efx) { struct efx_ef10_filter_table *table = efx->filter_state; @@ -3408,6 +3424,9 @@ static void efx_ef10_filter_sync_rx_mode(struct efx_nic *efx) if (!efx_dev_registered(efx)) return; + if (!table) + return; + /* Mark old filters that may need to be removed */ spin_lock_bh(&efx->filter_lock); n = table->dev_uc_count < 0 ? 1 : table->dev_uc_count; diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c index 864c337..50816cd 100644 --- a/drivers/net/ethernet/sfc/efx.c +++ b/drivers/net/ethernet/sfc/efx.c @@ -949,6 +949,16 @@ void efx_link_set_wanted_fc(struct efx_nic *efx, u8 wanted_fc) static void efx_fini_port(struct efx_nic *efx); +/* We assume that efx->type->reconfigure_mac will always try to sync RX + * filters and therefore needs to read-lock the filter table against freeing + */ +void efx_mac_reconfigure(struct efx_nic *efx) +{ + down_read(&efx->filter_sem); + efx->type->reconfigure_mac(efx); + up_read(&efx->filter_sem); +} + /* Push loopback/power/transmit disable settings to the PHY, and reconfigure * the MAC appropriately. All other PHY configuration changes are pushed * through phy_op->set_settings(), and pushed asynchronously to the MAC @@ -1002,7 +1012,7 @@ static void efx_mac_work(struct work_struct *data) mutex_lock(&efx->mac_lock); if (efx->port_enabled) - efx->type->reconfigure_mac(efx); + efx_mac_reconfigure(efx); mutex_unlock(&efx->mac_lock); } @@ -1042,7 +1052,7 @@ static int efx_init_port(struct efx_nic *efx) /* Reconfigure the MAC before creating dma queues (required for * Falcon/A1 where RX_INGR_EN/TX_DRAIN_EN isn't supported) */ - efx->type->reconfigure_mac(efx); + efx_mac_reconfigure(efx); /* Ensure the PHY advertises the correct flow control settings */ rc = efx->phy_op->reconfigure(efx); @@ -1068,7 +1078,7 @@ static void efx_start_port(struct efx_nic *efx) efx->port_enabled = true; /* Ensure MAC ingress/egress is enabled */ - efx->type->reconfigure_mac(efx); + efx_mac_reconfigure(efx); mutex_unlock(&efx->mac_lock); } @@ -1672,10 +1682,11 @@ static int efx_probe_filters(struct efx_nic *efx) int rc; spin_lock_init(&efx->filter_lock); - + init_rwsem(&efx->filter_sem); + down_write(&efx->filter_sem); rc = efx->type->filter_table_probe(efx); if (rc) - return rc; + goto out_unlock; #ifdef CONFIG_RFS_ACCEL if (efx->type->offload_features & NETIF_F_NTUPLE) { @@ -1684,12 +1695,14 @@ static int efx_probe_filters(struct efx_nic *efx) GFP_KERNEL); if (!efx->rps_flow_id) { efx->type->filter_table_remove(efx); - return -ENOMEM; + rc = -ENOMEM; + goto out_unlock; } } #endif - - return 0; +out_unlock: + up_write(&efx->filter_sem); + return rc; } static void efx_remove_filters(struct efx_nic *efx) @@ -1697,12 +1710,16 @@ static void efx_remove_filters(struct efx_nic *efx) #ifdef CONFIG_RFS_ACCEL kfree(efx->rps_flow_id); #endif + down_write(&efx->filter_sem); efx->type->filter_table_remove(efx); + up_write(&efx->filter_sem); } static void efx_restore_filters(struct efx_nic *efx) { + down_read(&efx->filter_sem); efx->type->filter_table_restore(efx); + up_read(&efx->filter_sem); } /************************************************************************** @@ -2183,7 +2200,7 @@ static int efx_change_mtu(struct net_device *net_dev, int new_mtu) mutex_lock(&efx->mac_lock); net_dev->mtu = new_mtu; - efx->type->reconfigure_mac(efx); + efx_mac_reconfigure(efx); mutex_unlock(&efx->mac_lock); efx_start_all(efx); @@ -2219,7 +2236,7 @@ static int efx_set_mac_address(struct net_device *net_dev, void *data) /* Reconfigure the MAC */ mutex_lock(&efx->mac_lock); - efx->type->reconfigure_mac(efx); + efx_mac_reconfigure(efx); mutex_unlock(&efx->mac_lock); return 0; @@ -2464,7 +2481,9 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok) " VFs may not function\n", rc); #endif + down_read(&efx->filter_sem); efx_restore_filters(efx); + up_read(&efx->filter_sem); if (efx->type->sriov_reset) efx->type->sriov_reset(efx); diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h index 9097906..46aee41 100644 --- a/drivers/net/ethernet/sfc/efx.h +++ b/drivers/net/ethernet/sfc/efx.h @@ -74,6 +74,8 @@ void efx_schedule_slow_fill(struct efx_rx_queue *rx_queue); /* Filters */ +void efx_mac_reconfigure(struct efx_nic *efx); + /** * efx_filter_insert_filter - add or replace a filter * @efx: NIC in which to insert the filter diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c index 03829b4..0347976 100644 --- a/drivers/net/ethernet/sfc/ethtool.c +++ b/drivers/net/ethernet/sfc/ethtool.c @@ -734,7 +734,7 @@ static int efx_ethtool_set_pauseparam(struct net_device *net_dev, /* Reconfigure the MAC. The PHY *may* generate a link state change event * if the user just changed the advertised capabilities, but there's no * harm doing this twice */ - efx->type->reconfigure_mac(efx); + efx_mac_reconfigure(efx); out: mutex_unlock(&efx->mac_lock); diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index 55a5f48..f6c4832 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -896,7 +897,8 @@ struct vfdi_status; * @loopback_mode: Loopback status * @loopback_modes: Supported loopback mode bitmask * @loopback_selftest: Offline self-test private state - * @filter_lock: Filter table lock + * @filter_sem: Filter table rw_semaphore, for freeing the table + * @filter_lock: Filter table lock, for mere content changes * @filter_state: Architecture-dependent filter table state * @rps_flow_id: Flow IDs of filters allocated for accelerated RFS, * indexed by filter ID @@ -1038,6 +1040,7 @@ struct efx_nic { void *loopback_selftest; + struct rw_semaphore filter_sem; spinlock_t filter_lock; void *filter_state; #ifdef CONFIG_RFS_ACCEL