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

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

[Jiayuan Chen]

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);
 
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Matthias Tafelmeier]

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);
>