| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eddd62ab4e7f2a91b5544df709ecc0d3@imap.linux.ibm.com>
Date: Mon, 01 Feb 2021 09:39:29 -0800
From: Dany Madden <drt@...ux.ibm.com>
To: Sukadev Bhattiprolu <sukadev@...ux.ibm.com>
Cc: netdev@...r.kernel.org, Lijun Pan <ljp@...ux.ibm.com>,
Rick Lindsley <ricklind@...ux.ibm.com>, abdhalee@...ibm.com
Subject: Re: [PATCH net 1/2] ibmvnic: fix a race between open and reset
On 2021-01-28 19:47, Sukadev Bhattiprolu wrote:
> __ibmvnic_reset() currently reads the adapter->state before getting the
> rtnl and saves that state as the "target state" for the reset. If this
> read occurs when adapter is in PROBED state, the target state would be
> PROBED.
>
> Just after the target state is saved, and before the actual reset
> process
> is started (i.e before rtnl is acquired) if we get an ibmvnic_open()
> call
> we would move the adapter to OPEN state.
>
> But when the reset is processed (after ibmvnic_open()) drops the rtnl),
> it will leave the adapter in PROBED state even though we already moved
> it to OPEN.
>
> To fix this, use the RTNL to improve the serialization when
> reading/updating
> the adapter state. i.e determine the target state of a reset only after
> getting the RTNL. And if a reset is in progress during an open, simply
> set the target state of the adapter and let the reset code finish the
> open (like we currently do if failover is pending).
>
> One twist to this serialization is if the adapter state changes when we
> drop the RTNL to update the link state. Account for this by checking if
> there was an intervening open and update the target state for the reset
> accordingly (see new comments in the code). Note that only the reset
> functions and ibmvnic_open() can set the adapter to OPEN state and this
> must happen under rtnl.
>
> Fixes: 7d7195a026ba ("ibmvnic: Do not process device remove during
> device reset")
> Signed-off-by: Sukadev Bhattiprolu <sukadev@...ux.ibm.com>
Reviewed-by: Dany Madden <drt@...ux.ibm.com>
> ---
> drivers/net/ethernet/ibm/ibmvnic.c | 72 +++++++++++++++++++++++++++---
> 1 file changed, 65 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index 8820c98ea891..cb7ddfefb03e 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1197,12 +1197,26 @@ static int ibmvnic_open(struct net_device
> *netdev)
> struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> int rc;
>
> - /* If device failover is pending, just set device state and return.
> - * Device operation will be handled by reset routine.
> + WARN_ON_ONCE(!rtnl_is_locked());
> +
> + /**
> + * If device failover is pending or we are about to reset, just set
> + * device state and return. Device operation will be handled by reset
> + * routine.
> + *
> + * It should be safe to overwrite the adapter->state here. Since
> + * we hold the rtnl, either the reset has not actually started or
> + * the rtnl got dropped during the set_link_state() in do_reset().
> + * In the former case, no one else is changing the state (again we
> + * have the rtnl) and in the latter case, do_reset() will detect and
> + * honor our setting below.
> */
> - if (adapter->failover_pending) {
> + if (adapter->failover_pending || (test_bit(0, &adapter->resetting)))
> {
> + netdev_dbg(netdev, "[S:%d FOP:%d] Resetting, deferring open\n",
> + adapter->state, adapter->failover_pending);
> adapter->state = VNIC_OPEN;
> - return 0;
> + rc = 0;
> + goto out;
> }
>
> if (adapter->state != VNIC_CLOSED) {
> @@ -1222,10 +1236,12 @@ static int ibmvnic_open(struct net_device
> *netdev)
>
> out:
> /*
> - * If open fails due to a pending failover, set device state and
> - * return. Device operation will be handled by reset routine.
> + * If open failed and there is a pending failover or in-progress
> reset,
> + * set device state and return. Device operation will be handled by
> + * reset routine. See also comments above regarding rtnl.
> */
> - if (rc && adapter->failover_pending) {
> + if (rc &&
> + (adapter->failover_pending || (test_bit(0,
> &adapter->resetting)))) {
> adapter->state = VNIC_OPEN;
> rc = 0;
> }
> @@ -1939,6 +1955,14 @@ static int do_change_param_reset(struct
> ibmvnic_adapter *adapter,
> netdev_dbg(adapter->netdev, "Change param resetting driver (%d)\n",
> rwi->reset_reason);
>
> + /* read the state and check (again) after getting rtnl */
> + reset_state = adapter->state;
> +
> + if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {
> + rc = -EBUSY;
> + goto out;
> + }
> +
> netif_carrier_off(netdev);
> adapter->reset_reason = rwi->reset_reason;
>
> @@ -2037,6 +2061,14 @@ static int do_reset(struct ibmvnic_adapter
> *adapter,
> if (rwi->reset_reason == VNIC_RESET_FAILOVER)
> adapter->failover_pending = false;
>
> + /* read the state and check (again) after getting rtnl */
> + reset_state = adapter->state;
> +
> + if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {
> + rc = -EBUSY;
> + goto out;
> + }
> +
> netif_carrier_off(netdev);
> adapter->reset_reason = rwi->reset_reason;
>
> @@ -2063,7 +2095,25 @@ static int do_reset(struct ibmvnic_adapter
> *adapter,
> if (rc)
> goto out;
>
> + if (adapter->state == VNIC_OPEN) {
> + /**
> + * When we dropped rtnl, ibmvnic_open() got it and
> + * noticed that we are resetting and set the adapter
> + * state to OPEN. Update our new "target" state,
> + * and resume the reset from VNIC_CLOSING state.
> + */
> + netdev_dbg(netdev,
> + "Open changed state from %d, updating.\n",
> + reset_state);
> + reset_state = VNIC_OPEN;
> + adapter->state = VNIC_CLOSING;
> + }
> +
> if (adapter->state != VNIC_CLOSING) {
> + /**
> + * If someone else changed the adapter state
> + * when we dropped the rtnl, fail the reset
> + */
> rc = -1;
> goto out;
> }
> @@ -2197,6 +2247,14 @@ static int do_hard_reset(struct ibmvnic_adapter
> *adapter,
> netdev_dbg(adapter->netdev, "Hard resetting driver (%d)\n",
> rwi->reset_reason);
>
> + /* read the state and check (again) after getting rtnl */
> + reset_state = adapter->state;
> +
> + if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {
> + rc = -EBUSY;
> + goto out;
> + }
> +
> netif_carrier_off(netdev);
> adapter->reset_reason = rwi->reset_reason;
Powered by blists - more mailing lists