netdev - Re: [PATCH net-next] net: stmmac: allow changing the MTU while the interface is running

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: <20170405.074932.195620842509788140.davem@davemloft.net>
Date:   Wed, 05 Apr 2017 07:49:32 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     niklas.cassel@...s.com
Cc:     peppe.cavallaro@...com, alexandre.torgue@...com,
        Joao.Pinto@...opsys.com, f.fainelli@...il.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        niklass@...s.com
Subject: Re: [PATCH net-next] net: stmmac: allow changing the MTU while the
 interface is running

From: Niklas Cassel <niklas.cassel@...s.com>
Date: Tue, 4 Apr 2017 14:18:54 +0200

> From: Niklas Cassel <niklas.cassel@...s.com>
> 
> Setting ethtool ops for stmmac is only allowed when the interface is up.
> Setting MTU (a netdev op) for stmmac is only allowed when the interface
> is down.
> 
> It seems that the only reason why MTU cannot be changed when running is
> that we have not bothered to implement a nice way to dealloc/alloc the
> descriptor rings.
> 
> To make it less confusing for the user, call ndo_stop() and ndo_open()
> from ndo_change_mtu(). This is not a nice way to dealloc/alloc the
> descriptor rings, since it will announce that the interface is being
> brought down/up to user space, but there are several other drivers doing
> it this way, and it is arguably better than just returning -EBUSY.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@...s.com>

You can't do this with no error handling.

Instead, you must do this using a "prepare", "commit" sequence.
First making sure you can reallocate all necessary resources,
and make the config change, before actually doing so.

You're not even checking if the re-open fails, meaning that an MTU
change can cause the interface to shut down.  That is simply not
acceptable.