netdev - Re: [PATCH net v3] octeon_ep: Add SKB allocation failures handling in __octep_oq_process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240930162622.GF1310185@kernel.org>
Date: Mon, 30 Sep 2024 17:26:22 +0100
From: Simon Horman <horms@...nel.org>
To: Aleksandr Mishin <amishin@...rgos.ru>
Cc: Veerasenareddy Burru <vburru@...vell.com>,
	Sathesh Edara <sedara@...vell.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Abhijit Ayarekar <aayarekar@...vell.com>,
	Satananda Burla <sburla@...vell.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH net v3] octeon_ep: Add SKB allocation failures handling
 in __octep_oq_process_rx()

On Mon, Sep 30, 2024 at 08:33:28AM +0300, Aleksandr Mishin wrote:
> build_skb() returns NULL in case of a memory allocation failure so handle
> it inside __octep_oq_process_rx() to avoid NULL pointer dereference.
> 
> __octep_oq_process_rx() is called during NAPI polling by the driver. If
> skb allocation fails, keep on pulling packets out of the Rx DMA queue: we
> shouldn't break the polling immediately and thus falsely indicate to the
> octep_napi_poll() that the Rx pressure is going down. As there is no
> associated skb in this case, don't process the packets and don't push them
> up the network stack - they are skipped.
> 
> The common code with skb and some index manipulations is extracted to make
> the fix more readable and avoid code duplication. Also helper function is
> implemented to unmmap/flush all the fragment buffers used by the dropped
> packet. 'alloc_failures' counter is incremented to mark the skb allocation
> error in driver statistics.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> Suggested-by: Paolo Abeni <pabeni@...hat.com>
> Suggested-by: Simon Horman <horms@...nel.org>
> Signed-off-by: Aleksandr Mishin <amishin@...rgos.ru>
> ---
> A similar situation is present in the __octep_vf_oq_process_rx() of the
> Octeon VF driver. First we want to try the fix on __octep_oq_process_rx().

One step at a time :)

> Compile tested only.
> 
> v3:
>   - Implement helper which frees current packet resources and increase
>     index and descriptor as suggested by Simon
>     (https://lore.kernel.org/all/20240919134812.GB1571683@kernel.org/)
>   - Optimize helper as suggested by Paolo
>     (https://lore.kernel.org/all/b9ae8575-f903-425f-aa42-0c2a7605aa94@redhat.com/)	

Thanks for the revision.
This version looks good to me.

Reviewed-by: Simon Horman <horms@...nel.org>

> v2: https://lore.kernel.org/all/20240916060212.12393-1-amishin@t-argos.ru/
>   - Implement helper instead of adding multiple checks for '!skb' and
>     remove 'rx_bytes' increasing in case of packet dropping as suggested
>     by Paolo
>     (https://lore.kernel.org/all/ba514498-3706-413b-a09f-f577861eef28@redhat.com/)
> v1: https://lore.kernel.org/all/20240906063907.9591-1-amishin@t-argos.ru/
> 
>  .../net/ethernet/marvell/octeon_ep/octep_rx.c | 82 +++++++++++++------
>  1 file changed, 59 insertions(+), 23 deletions(-)

...