netdev - Re: [PATCH] ipvlan: fix ipvlan MTU limits

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]

Date:   Tue, 9 Jan 2018 09:21:36 -0800
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     liuqifa@...wei.com
Cc:     David Miller <davem@...emloft.net>, dsahern@...il.com,
        mschiffer@...verse-factory.net, idosch@...lanox.com, fw@...len.de,
        kjlx@...pleofstupid.com, girish.moodalbail@...cle.com,
        sainath.grandhi@...el.com, linux-netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] ipvlan: fix ipvlan MTU limits

On Mon, Jan 8, 2018 at 10:48 PM,  <liuqifa@...wei.com> wrote:
> From: Keefe Liu <liuqifa@...wei.com>
>
> The MTU of ipvlan interface should not bigger than the phy device, When we
> run following scripts, we will find there are some problems.
> Step1:
>         ip link add link eth0 name ipv1 type ipvlan mode l2
>         ip netns add net1
>         ip link set dev ipv1 netns net1
> Step2:
>         ip netns exec net1 ip link set dev ipv1 mtu 1501
>         RTNETLINK answers: Invalid argument
>         dmesg info: "ipv1: Invalid MTU 1501 requested, hw max 1500"
> Step3:
>         ip link set dev eth0 mtu 1600
>         ip netns exec net1 ip link set dev ipv1 mtu 1501
>         RTNETLINK answers: Invalid argument
>         dmesg info: "ipv1: Invalid MTU 1501 requested, hw max 1500"
> Step4:
>         ip link set dev eth0 mtu 1400
>         ip netns exec net1 ip link set dev ipv1 mtu 1500
> The result of Step2 is we expected, but the result of Step3 and Step4
> are not.
>
> This patch set ipvlan's maximum MTU to ETH_MAX_MTU, and when we change
> the ipvlan device's MTU, ipvlan_change_mtu() will make sure the new MTU
> no larger than the phy device's MTU.
>
> Signed-off-by: Keefe Liu <liuqifa@...wei.com>
> ---
>  drivers/net/ipvlan/ipvlan_main.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index 30cb803..84c007d 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -380,12 +380,24 @@ static int ipvlan_get_iflink(const struct net_device *dev)
>         return ipvlan->phy_dev->ifindex;
>  }
>
> +static int ipvlan_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +       struct ipvl_dev *ipvlan = netdev_priv(dev);
> +
> +       if (ipvlan->phy_dev->mtu < new_mtu)
> +               return -EINVAL;
> +
> +       dev->mtu = new_mtu;
> +       return 0;
> +}
> +
>  static const struct net_device_ops ipvlan_netdev_ops = {
>         .ndo_init               = ipvlan_init,
>         .ndo_uninit             = ipvlan_uninit,
>         .ndo_open               = ipvlan_open,
>         .ndo_stop               = ipvlan_stop,
>         .ndo_start_xmit         = ipvlan_start_xmit,
> +       .ndo_change_mtu         = ipvlan_change_mtu,
>         .ndo_fix_features       = ipvlan_fix_features,
>         .ndo_change_rx_flags    = ipvlan_change_rx_flags,
>         .ndo_set_rx_mode        = ipvlan_set_multicast_mac_filter,
> @@ -680,6 +692,8 @@ void ipvlan_link_setup(struct net_device *dev)
>  {
>         ether_setup(dev);
>
> +       dev->min_mtu = 0;
should be ETH_MIN_MTU since we expect the underlying device to be
ETH_ARPHDR and IPvlan deals with IPv4/IPv6.

> +       dev->max_mtu = ETH_MAX_MTU;
>         dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>         dev->priv_flags |= IFF_UNICAST_FLT | IFF_NO_QUEUE;
>         dev->netdev_ops = &ipvlan_netdev_ops;
> --
> 1.8.3.1
>
>
These changes are not sufficient if you want to have different
per-slave mtu settings. One can always change the MTU of the master
device and all  per-slave settings will get wiped. I don't think
that's a desired outcome.