Re: [PATCH net] net: reject dev_weight products that overflow int

[Matthias Tafelmeier <matthias.tafelmeier@gmx.net>]

ACK from my side.

> proc_do_dev_weight() sets the backlog NAPI weight and the qdisc quota
> to dev_weight * dev_weight_rx_bias and dev_weight * dev_weight_tx_bias.
> The three sysctls only have a lower bound of 1, so the products can
> overflow int and end up <= 0, e.g. dev_weight=65536 and
> dev_weight_rx_bias=65536 give a backlog weight of 0.
>
> With a weight <= 0, __napi_poll() never sees work < weight. Once
> process_backlog() drains the queue and completes the NAPI by itself,
> __napi_poll() still asks for a repoll, so net_rx_action() keeps putting
> back a NAPI that is no longer scheduled and NET_RX spins on that CPU
> with nothing to do. Restoring the sysctls does not help, as napi->weight
> is only refreshed by process_backlog(), which is never called again.
>
> After 3 pings over lo on CPU 2, with CONFIG_DEBUG_NET:
>
>    (NULL net_device): NAPI poll function process_backlog+0x0/0x1200 returned 1, exceeding its budget of 0.
>    repoll requested for device (efault) process_backlog but napi is not scheduled.
>    repoll requested for device (efault) process_backlog but napi is not scheduled.
>    ...
>
> and with nothing queued anymore, NET_RX keeps running on CPU 2:
>
>    NET_RX on cpu2: +454190 in 5s, ksoftirqd/2 at ~82% CPU
>
> Reject writes that make either product overflow int, and print the
> values involved.
>
> Fixes: 3d48b53fb2ae ("net: dev_weight: TX/RX orthogonality")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
>   net/core/sysctl_net_core.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index 473c162736d6..4f3796bc8870 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -318,14 +318,23 @@ static int proc_do_dev_weight(const struct ctl_table *table, int write,
>   			   void *buffer, size_t *lenp, loff_t *ppos)
>   {
>   	static DEFINE_MUTEX(dev_weight_mutex);
> -	int ret, weight;
> +	int ret, weight, old, rx, tx;
>   
>   	mutex_lock(&dev_weight_mutex);
> +	old = *(int *)table->data;
>   	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>   	if (!ret && write) {
>   		weight = READ_ONCE(weight_p);
> -		WRITE_ONCE(net_hotdata.dev_rx_weight, weight * dev_weight_rx_bias);
> -		WRITE_ONCE(net_hotdata.dev_tx_weight, weight * dev_weight_tx_bias);
> +		if (check_mul_overflow(weight, dev_weight_rx_bias, &rx) ||
> +		    check_mul_overflow(weight, dev_weight_tx_bias, &tx)) {
> +			pr_warn("dev_weight(%d) * (rx_bias(%d) or tx_bias(%d)) overflows int\n",
> +				weight, dev_weight_rx_bias, dev_weight_tx_bias);
> +			WRITE_ONCE(*(int *)table->data, old);
> +			ret = -EINVAL;
> +		} else {
> +			WRITE_ONCE(net_hotdata.dev_rx_weight, rx);
> +			WRITE_ONCE(net_hotdata.dev_tx_weight, tx);
> +		}
>   	}
>   	mutex_unlock(&dev_weight_mutex);
>