Messages in this thread | | | From | Shay Agroskin <> | Subject | Re: [PATCH net] net: ena: fix shift-out-of-bounds in exponential backoff | Date | Tue, 11 Jul 2023 20:47:32 +0300 |
| |
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
| |