Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230711013621.GE1926@templeofstupid.com>
User-agent: mu4e 1.10.3; emacs 28.2
From:   Shay Agroskin <shayagr@amazon.com>
To:     Krister Johansen <kjlx@templeofstupid.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        "Arthur Kiyanovski" <akiyano@amazon.com>,
        David Arinzon <darinzon@amazon.com>,
        "Noam Dagan" <ndagan@amazon.com>,
        Saeed Bishara <saeedb@amazon.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH net] net: ena: fix shift-out-of-bounds in exponential
 backoff
Date:   Tue, 11 Jul 2023 20:47:32 +0300
In-Reply-To: <20230711013621.GE1926@templeofstupid.com>
Message-ID: <pj41zllefmpbw7.fsf@u95c7fd9b18a35b.ant.amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed
Precedence: bulk


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