Re: [PATCH net] ibmveth: Disable tx queue while changing mtu

[David Miller <davem@davemloft.net>]

From: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
Date: Tue,  9 Aug 2016 12:47:37 -0500

> If the device is running while the MTU is changed, ibmveth
> is closed and the bounce buffer is freed. If a transmission
> is sent before ibmveth can be reopened, ibmveth_start_xmit
> tries to copy to the null bounce buffer, leading to a kernel
> oops. The proposed solution disables the tx queue until
> ibmveth is restarted.
> 
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Tested-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>

The bugs in the patch show clearly why this kind of non-unwindable behavior
is so undesirable.

> @@ -1378,14 +1379,18 @@ static int ibmveth_change_mtu(struct net_device *dev, int new_mtu)
>  						ibmveth_get_desired_dma
>  						(viodev));
>  			if (need_restart) {
> -				return ibmveth_open(adapter->netdev);
> +				rc = ibmveth_open(adapter->netdev);
> +				netif_wake_queue(dev);
> +				return rc;

If the open fails, the last thing in the world you should do is wake the
TX queue.

Furthermore, ibmveth_open() does netif_start_queue() so this call should
be completely unnecessary.

But fundamentally here the real problem, the whole operation should be
done in a "prepare, commit" style transaction.  So that if we can't
make the MTU change for whatever reason, the original MTU
configuration is retained and the interface stays up and operational.

The error recovery mechanism here in this function is unacceptable,
and needs to be rewritten from scratch.