Re: [PATCH net] net: sched: taprio: Avoid division by zero on invalid link speed

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
To: Vladimir Oltean <olteanv@gmail.com>, davem@davemloft.net
Cc: jhs@mojatatu.com, xiyou.wangcong@gmail.com,
	netdev@vger.kernel.org, Vladimir Oltean <olteanv@gmail.com>
Subject: Re: [PATCH net] net: sched: taprio: Avoid division by zero on invalid link speed
Date: Mon, 30 Sep 2019 09:39:02 -0700	[thread overview]
Message-ID: <871rvx91sp.fsf@linux.intel.com> (raw)
In-Reply-To: <20190928233722.15054-1-olteanv@gmail.com>

Vladimir Oltean <olteanv@gmail.com> writes:

> The check in taprio_set_picos_per_byte is currently not robust enough
> and will trigger this division by zero, due to e.g. PHYLINK not setting
> kset->base.speed when there is no PHY connected:
>
> [   27.109992] Division by zero in kernel.
> [   27.113842] CPU: 1 PID: 198 Comm: tc Not tainted 5.3.0-rc5-01246-gc4006b8c2637-dirty #212
> [   27.121974] Hardware name: Freescale LS1021A
> [   27.126234] [<c03132e0>] (unwind_backtrace) from [<c030d8b8>] (show_stack+0x10/0x14)
> [   27.133938] [<c030d8b8>] (show_stack) from [<c10b21b0>] (dump_stack+0xb0/0xc4)
> [   27.141124] [<c10b21b0>] (dump_stack) from [<c10af97c>] (Ldiv0_64+0x8/0x18)
> [   27.148052] [<c10af97c>] (Ldiv0_64) from [<c0700260>] (div64_u64+0xcc/0xf0)
> [   27.154978] [<c0700260>] (div64_u64) from [<c07002d0>] (div64_s64+0x4c/0x68)
> [   27.161993] [<c07002d0>] (div64_s64) from [<c0f3d890>] (taprio_set_picos_per_byte+0xe8/0xf4)
> [   27.170388] [<c0f3d890>] (taprio_set_picos_per_byte) from [<c0f3f614>] (taprio_change+0x668/0xcec)
> [   27.179302] [<c0f3f614>] (taprio_change) from [<c0f2bc24>] (qdisc_create+0x1fc/0x4f4)
> [   27.187091] [<c0f2bc24>] (qdisc_create) from [<c0f2c0c8>] (tc_modify_qdisc+0x1ac/0x6f8)
> [   27.195055] [<c0f2c0c8>] (tc_modify_qdisc) from [<c0ee9604>] (rtnetlink_rcv_msg+0x268/0x2dc)
> [   27.203449] [<c0ee9604>] (rtnetlink_rcv_msg) from [<c0f4fef0>] (netlink_rcv_skb+0xe0/0x114)
> [   27.211756] [<c0f4fef0>] (netlink_rcv_skb) from [<c0f4f6cc>] (netlink_unicast+0x1b4/0x22c)
> [   27.219977] [<c0f4f6cc>] (netlink_unicast) from [<c0f4fa84>] (netlink_sendmsg+0x284/0x340)
> [   27.228198] [<c0f4fa84>] (netlink_sendmsg) from [<c0eae5fc>] (sock_sendmsg+0x14/0x24)
> [   27.235988] [<c0eae5fc>] (sock_sendmsg) from [<c0eaedf8>] (___sys_sendmsg+0x214/0x228)
> [   27.243863] [<c0eaedf8>] (___sys_sendmsg) from [<c0eb015c>] (__sys_sendmsg+0x50/0x8c)
> [   27.251652] [<c0eb015c>] (__sys_sendmsg) from [<c0301000>] (ret_fast_syscall+0x0/0x54)
> [   27.259524] Exception stack(0xe8045fa8 to 0xe8045ff0)
> [   27.264546] 5fa0:                   b6f608c8 000000f8 00000003 bed7e2f0 00000000 00000000
> [   27.272681] 5fc0: b6f608c8 000000f8 004ce54c 00000128 5d3ce8c7 00000000 00000026 00505c9c
> [   27.280812] 5fe0: 00000070 bed7e298 004ddd64 b6dd1e64
>
> Russell King points out that the ethtool API says zero is a valid return
> value of __ethtool_get_link_ksettings:
>
>    * If it is enabled then they are read-only; if the link
>    * is up they represent the negotiated link mode; if the link is down,
>    * the speed is 0, %SPEED_UNKNOWN or the highest enabled speed and
>    * @duplex is %DUPLEX_UNKNOWN or the best enabled duplex mode.
>
>   So, it seems that taprio is not following the API... I'd suggest either
>   fixing taprio, or getting agreement to change the ethtool API.
>
> The chosen path was to fix taprio.
>
> Fixes: 7b9eba7ba0c1 ("net/sched: taprio: fix picos_per_byte miscalculation")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>