Re: [net-next,v2,7/9] net: use core MTU range checking in misc drivers

[Sven Eckelmann <sven-KaDOiPu9UxWEi8DpZVb4nw@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

On Donnerstag, 20. Oktober 2016 13:55:22 CEST Jarod Wilson wrote:
[...]
> batman-adv:
> - set max_mtu
> - remove batadv_interface_change_mtu
> - initialization is a little async, not 100% certain that max_mtu is set
>   in the optimal place, don't have hardware to test with

batman-adv is creating a virtual interface - so there are no
hardware requirements (ok, ethernet compatible hardware - even
when only virtual/emulated).

[...]
> diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
> index 49e16b6..112679d 100644
> --- a/net/batman-adv/soft-interface.c
> +++ b/net/batman-adv/soft-interface.c
> @@ -158,17 +158,6 @@ static int batadv_interface_set_mac_addr(struct net_device *dev, void *p)
>  	return 0;
>  }
>  
> -static int batadv_interface_change_mtu(struct net_device *dev, int new_mtu)
> -{
> -	/* check ranges */
> -	if ((new_mtu < 68) || (new_mtu > batadv_hardif_min_mtu(dev)))
> -		return -EINVAL;
> -
> -	dev->mtu = new_mtu;
> -
> -	return 0;
> -}
> -
>  /**
>   * batadv_interface_set_rx_mode - set the rx mode of a device
>   * @dev: registered network device to modify
> @@ -920,7 +909,6 @@ static const struct net_device_ops batadv_netdev_ops = {
>  	.ndo_vlan_rx_add_vid = batadv_interface_add_vid,
>  	.ndo_vlan_rx_kill_vid = batadv_interface_kill_vid,
>  	.ndo_set_mac_address = batadv_interface_set_mac_addr,
> -	.ndo_change_mtu = batadv_interface_change_mtu,
>  	.ndo_set_rx_mode = batadv_interface_set_rx_mode,
>  	.ndo_start_xmit = batadv_interface_tx,
>  	.ndo_validate_addr = eth_validate_addr,
> @@ -987,6 +975,7 @@ struct net_device *batadv_softif_create(struct net *net, const char *name)
>  	dev_net_set(soft_iface, net);
>  
>  	soft_iface->rtnl_link_ops = &batadv_link_ops;
> +	soft_iface->max_mtu = batadv_hardif_min_mtu(soft_iface);
>  
>  	ret = register_netdevice(soft_iface);
>  	if (ret < 0) {

This looks bogus to me. You are now setting max_mtu during initialization of
the virtual interface. But at this time no slave interfaces were added to the
master batman-adv interface. So the batadv_hardif_min_mtu will not return the
correct value here. Especially if you don't have fragmentation enabled.

So this change looks like a bug to me

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]