netdev - Re: [PATCH][net-next] bridge: clean up mtu_set_by

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: <780d2dff-c03c-8e0a-f2c0-2b3d5974512d@cumulusnetworks.com>
Date:   Fri, 13 Jul 2018 11:00:56 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Li RongQing <lirongqing@...du.com>, netdev@...r.kernel.org
Subject: Re: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to
 false and comments

On 13/07/18 09:47, Li RongQing wrote:
> Once mtu_set_by_user is set to true, br_mtu_auto_adjust will
> not run, and no chance to clear mtu_set_by_user.
> 
^^
This was by design, there is no error here and no "cleanup" is needed.
If you read the ndo_change_mtu() call you'll see the comment:
/* this flag will be cleared if the MTU was automatically adjusted */

It is the only way we can know that the MTU was automatically adjusted or set
by the user manually in which case we need to _stop_ automatically adjusting
MTU. This was done to be backwards compatible as much as possible but still
give the option to have user-configured MTU which doesn't disappear (and is
not overwritten).

So please next time read the original commit.

>From the original commit 804b854d374e ("net: bridge: disable bridge MTU auto tuning if it was set manually"):
" ...
    Let's improve on that situation and allow for the user to
    set any MTU within ETH_MIN/MAX limits, but once manually configured it
    is the user's responsibility to keep it correct afterwards.
    
    In case the MTU isn't manually set - the behaviour reverts to the
    previous and the bridge follows the minimum MTU.
...
"

> and br_mtu_auto_adjust will run only if mtu_set_by_user is
> false, so not need to set it to false again
> 
> Cc: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> Signed-off-by: Li RongQing <lirongqing@...du.com>
> ---
>  net/bridge/br_device.c | 1 -
>  net/bridge/br_if.c     | 4 ----
>  2 files changed, 5 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index e682a668ce57..c636bc2749c2 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -227,7 +227,6 @@ static int br_change_mtu(struct net_device *dev, int new_mtu)
>  
>  	dev->mtu = new_mtu;
>  
> -	/* this flag will be cleared if the MTU was automatically adjusted */
>  	br->mtu_set_by_user = true;
>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>  	/* remember the MTU in the rtable for PMTU */
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 05e42d86882d..47c65da4b1be 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -450,11 +450,7 @@ void br_mtu_auto_adjust(struct net_bridge *br)
>  	if (br->mtu_set_by_user)
>  		return;
>  
> -	/* change to the minimum MTU and clear the flag which was set by
> -	 * the bridge ndo_change_mtu callback
> -	 */
>  	dev_set_mtu(br->dev, br_mtu_min(br));
> -	br->mtu_set_by_user = false;
>  }
>  
>  static void br_set_gso_limits(struct net_bridge *br)
> 

Nacked-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>