lkml.org 
[lkml]   [2023]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH net] net: ena: fix shift-out-of-bounds in exponential backoff
Date

Krister Johansen <kjlx@templeofstupid.com> writes:

> The ENA adapters on our instances occasionally reset. Once
> recently
> logged a UBSAN failure to console in the process:
>
> UBSAN: shift-out-of-bounds in
> build/linux/drivers/net/ethernet/amazon/ena/ena_com.c:540:13
> shift exponent 32 is too large for 32-bit type 'unsigned int'
> CPU: 28 PID: 70012 Comm: kworker/u72:2 Kdump: loaded not
> tainted 5.15.117
> Hardware name: Amazon EC2 c5d.9xlarge/, BIOS 1.0 10/16/2017
> Workqueue: ena ena_fw_reset_device [ena]
> Call Trace:
> <TASK>
> dump_stack_lvl+0x4a/0x63
> dump_stack+0x10/0x16
> ubsan_epilogue+0x9/0x36
> __ubsan_handle_shift_out_of_bounds.cold+0x61/0x10e
> ? __const_udelay+0x43/0x50
> ena_delay_exponential_backoff_us.cold+0x16/0x1e [ena]
> wait_for_reset_state+0x54/0xa0 [ena]
> ena_com_dev_reset+0xc8/0x110 [ena]
> ena_down+0x3fe/0x480 [ena]
> ena_destroy_device+0xeb/0xf0 [ena]
> ena_fw_reset_device+0x30/0x50 [ena]
> process_one_work+0x22b/0x3d0
> worker_thread+0x4d/0x3f0
> ? process_one_work+0x3d0/0x3d0
> kthread+0x12a/0x150
> ? set_kthread_struct+0x50/0x50
> ret_from_fork+0x22/0x30
> </TASK>
>
> Apparently, the reset delays are getting so large they can
> trigger a
> UBSAN panic.
>
> Looking at the code, the current timeout is capped at 5000us.
> Using a
> base value of 100us, the current code will overflow after
> (1<<29). Even
> at values before 32, this function wraps around, perhaps
> unintentionally.
>
> Cap the value of the exponent used for this backoff at (1<<16)
> which is
> larger than currently necessary, but large enough to support
> bigger
> values in the future.
>
> Cc: stable@vger.kernel.org
> Fixes: 4bb7f4cf60e3 ("net: ena: reduce driver load time")
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> ---
> drivers/net/ethernet/amazon/ena/ena_com.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 451c3a1b6255..633b321d7fdd 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -35,6 +35,8 @@
>
> #define ENA_REGS_ADMIN_INTR_MASK 1
>
> +#define ENA_MAX_BACKOFF_DELAY_EXP 16U
> +
> #define ENA_MIN_ADMIN_POLL_US 100
>
> #define ENA_MAX_ADMIN_POLL_US 5000
> @@ -536,6 +538,7 @@ static int
> ena_com_comp_status_to_errno(struct ena_com_admin_queue
> *admin_queue,
>
> static void ena_delay_exponential_backoff_us(u32 exp, u32
> delay_us)
> {
> + exp = min_t(u32, exp, ENA_MAX_BACKOFF_DELAY_EXP);
> delay_us = max_t(u32, ENA_MIN_ADMIN_POLL_US, delay_us);
> delay_us = min_t(u32, delay_us * (1U << exp),
> ENA_MAX_ADMIN_POLL_US);
> usleep_range(delay_us, 2 * delay_us);

Hi, thanks for submitting this patch (:

Going over the logic here, the driver sleeps for `delay_us`
micro-seconds in each iteration that this function gets called.

For an exp = 14 it'd sleep (I added units notation)
delay_us * (2 ^ exp) us = 100 * (2 ^ 14) us = (10 * (2 ^ 14)) /
(1000000) s = 1.6 s

For an exp = 15 it'd sleep
(10 * (2 ^ 15)) / (1000000) = 3.2s

To even get close to an overflow value, say exp=29 the driver
would sleep in a single iteration
53687 s = 14.9 hours.

The driver should stop trying to get a response from the device
after a timeout period received from the device which is 3 seconds
by default.

The point being, it seems very unlikely to hit this overflow. Did
you experience it or was the issue discovered by a static analyzer
?

Regarding the patch itself, I don't mind adding it since exp=16
limit should be more than enough to wait for the device's
response.
Reviewed-by: Shay Agroskin <shayagr@amazon.com>

Thanks,
Shay

\
 
 \ /
  Last update: 2023-07-11 20:01    [W:0.063 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site