[PATCH net 1/2] ibmvnic: fix a race between open and reset

[PATCH net 1/2] ibmvnic: fix a race between open and reset

[Sukadev Bhattiprolu]

__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@linux.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;
 
-- 
2.26.2

[PATCH net 2/2] ibmvnic: fix race with multiple open/close

[Sukadev Bhattiprolu]

If two or more instances of 'ip link set' commands race and first one
already brings the interface up (or down), the subsequent instances
can simply return without redoing the up/down operation.

Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
Reported-by: Abdul Haleem <abdhalee@in.ibm.com>
Tested-by: Abdul Haleem <abdhalee@in.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index cb7ddfefb03e..84b772921f35 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1219,6 +1219,13 @@ static int ibmvnic_open(struct net_device *netdev)
 		goto out;
 	}
 
+	/* If adapter is already open, we don't have to do anything. */
+	if (adapter->state == VNIC_OPEN) {
+		netdev_dbg(netdev, "[S:%d] adapter already open\n",
+			   adapter->state);
+		return 0;
+	}
+
 	if (adapter->state != VNIC_CLOSED) {
 		rc = ibmvnic_login(netdev);
 		if (rc)
@@ -1392,6 +1399,12 @@ static int ibmvnic_close(struct net_device *netdev)
 		return 0;
 	}
 
+	/* If adapter is already closed, we don't have to do anything. */
+	if (adapter->state == VNIC_CLOSED) {
+		netdev_dbg(netdev, "[S:%d] adapter already closed\n",
+			   adapter->state);
+		return 0;
+	}
 	rc = __ibmvnic_close(netdev);
 	ibmvnic_cleanup(netdev);
 
-- 
2.26.2

Re: [PATCH net 2/2] ibmvnic: fix race with multiple open/close

[Willem de Bruijn]

On Thu, Jan 28, 2021 at 10:51 PM Sukadev Bhattiprolu
<sukadev@linux.ibm.com> wrote:
>
> If two or more instances of 'ip link set' commands race and first one
> already brings the interface up (or down), the subsequent instances
> can simply return without redoing the up/down operation.
>
> Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
> Reported-by: Abdul Haleem <abdhalee@in.ibm.com>
> Tested-by: Abdul Haleem <abdhalee@in.ibm.com>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>

Isn't this handled in the rtnetlink core based on IFF_UP?

        if ((old_flags ^ flags) & IFF_UP) {
                if (old_flags & IFF_UP)
                        __dev_close(dev);
                else
                        ret = __dev_open(dev, extack);
        }

Re: [PATCH net 1/2] ibmvnic: fix a race between open and reset

[Dany Madden]

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@linux.ibm.com>

Reviewed-by: Dany Madden <drt@linux.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;

Re: [PATCH net 2/2] ibmvnic: fix race with multiple open/close

[Sukadev Bhattiprolu]

Willem de Bruijn [willemdebruijn.kernel@gmail.com] wrote:
> On Thu, Jan 28, 2021 at 10:51 PM Sukadev Bhattiprolu
> <sukadev@linux.ibm.com> wrote:
> >
> > If two or more instances of 'ip link set' commands race and first one
> > already brings the interface up (or down), the subsequent instances
> > can simply return without redoing the up/down operation.
> >
> > Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
> > Reported-by: Abdul Haleem <abdhalee@in.ibm.com>
> > Tested-by: Abdul Haleem <abdhalee@in.ibm.com>
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> 
> Isn't this handled in the rtnetlink core based on IFF_UP?
> 
>         if ((old_flags ^ flags) & IFF_UP) {
>                 if (old_flags & IFF_UP)
>                         __dev_close(dev);
>                 else
>                         ret = __dev_open(dev, extack);
>         }

Good question. During our testing we hit the "adapter already open" debug
message a few times. Without this change, the core layer and dirver disagree
on the state and the adapter becomes unsuable.

I will debug this at the core layer also later this week.  

We are working on rewriting parts of driver surrounding locking/adapter
state and not sure if that will reveal any other cause for this behavior.

Sukadev

Re: [PATCH net 1/2] ibmvnic: fix a race between open and reset

[Jakub Kicinski]

On Thu, 28 Jan 2021 19:47:10 -0800 Sukadev Bhattiprolu wrote:
> +	WARN_ON_ONCE(!rtnl_is_locked());

ASSERT_RTNL() should do nicely here

Re: [PATCH net 1/2] ibmvnic: fix a race between open and reset

[Sukadev Bhattiprolu]

Jakub Kicinski [kuba@kernel.org] wrote:
> On Thu, 28 Jan 2021 19:47:10 -0800 Sukadev Bhattiprolu wrote:
> > +	WARN_ON_ONCE(!rtnl_is_locked());
> 
> ASSERT_RTNL() should do nicely here

Yes, Fixed in v2.

Thanks,

Sukadev