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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20160809.151344.1875492869890678715.davem@davemloft.net>
Date:	Tue, 09 Aug 2016 15:13:44 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	tlfalcon@...ux.vnet.ibm.com
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net] ibmveth: Disable tx queue while changing mtu

From: Thomas Falcon <tlfalcon@...ux.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@...hat.com>
> Tested-by: Jan Stancek <jstancek@...hat.com>
> Signed-off-by: Thomas Falcon <tlfalcon@...ux.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.