netdev - Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190102131657.GE22737@lunn.ch>
Date:   Wed, 2 Jan 2019 14:16:57 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Murali Krishna Policharla <murali.policharla@...adcom.com>
Cc:     Heiner Kallweit <hkallweit1@...il.com>, davem@...emloft.net,
        amritha.nambiar@...el.com, ecree@...arflare.com,
        ktkhai@...tuozzo.com, alexander.h.duyck@...el.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

> Hi Andrew/Heiner
> 
>                                        Thanks for the feedback. This patch
> fixes a case where ndo_change_mtu function is provided but the callback
> function is not storing mtu to netdevice structure.

Hi Murali

At the moment, any driver which implements ndo_change_mtu MUST set
ndev->mtu. It is a nice clean definition, easy for any driver write to
understand.

What you are proposing is that the core will set ndev->mtu. That is
fine, but again we need a nice clean definition. The drivers MUST NOT
set ndev->mtu.

If you plan to make this core change, please also change all the
drivers as well, so we have very clear semantics of what is expected.

It would also be good to extend the comment in include/linux/netdevice.h:

 * int (*ndo_change_mtu)(struct net_device *dev, int new_mtu);
 *      Called when a user wants to change the Maximum Transfer Unit
 *      of a device.

Adding something like: Should only program the hardware with the new MTU.

To give the hint that the core is doing all the validation and
modifying ndev->mtu.

I had one other thought. Please also take a look at how stacked
devices work. I've not looked, but i expect there are cases where a
lower device calls into an upper device to inform it the MTU has
changed. When does this call happen? Does the MTU of the lower device
need to of already changed before this call is made? Are there similar
cases of an upper device calling down to the lower device? You need to
be careful here, you are changing exactly when the ndev->mtu value
changes, and so could be introducing bugs if you don't do a proper
code review.

	  Andrew