linux-kernel - Re: [PATCH net] 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] [day] [month] [year] [list]

Message-ID: <ba514498-3706-413b-a09f-f577861eef28@redhat.com>
Date: Tue, 10 Sep 2024 13:39:08 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Aleksandr Mishin <amishin@...rgos.ru>,
 Veerasenareddy Burru <vburru@...vell.com>
Cc: Sathesh Edara <sedara@...vell.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, 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] octeon_ep: Add SKB allocation failures handling in
 __octep_oq_process_rx()

On 9/6/24 08:39, Aleksandr Mishin wrote:
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> index 4746a6b258f0..e92afd1a372a 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> @@ -394,32 +394,32 @@ static int __octep_oq_process_rx(struct octep_device *oct,
>   			data_offset = OCTEP_OQ_RESP_HW_SIZE;
>   			rx_ol_flags = 0;
>   		}
> +
> +		skb = build_skb((void *)resp_hw, PAGE_SIZE);
> +		if (skb)
> +			skb_reserve(skb, data_offset);
> +		else
> +			oq->stats.alloc_failures++;
>   		rx_bytes += buff_info->len;

The packet is dropped, we should not increase 'rx_bytes'

> +		read_idx++;
> +		desc_used++;
> +		if (read_idx == oq->max_count)
> +			read_idx = 0;
>   
>   		if (buff_info->len <= oq->max_single_buffer_size) {
> -			skb = build_skb((void *)resp_hw, PAGE_SIZE);
> -			skb_reserve(skb, data_offset);
> -			skb_put(skb, buff_info->len);
> -			read_idx++;
> -			desc_used++;
> -			if (read_idx == oq->max_count)
> -				read_idx = 0;
> +			if (skb)
> +				skb_put(skb, buff_info->len);
>   		} else {
>   			struct skb_shared_info *shinfo;
>   			u16 data_len;
>   
> -			skb = build_skb((void *)resp_hw, PAGE_SIZE);
> -			skb_reserve(skb, data_offset);
>   			/* Head fragment includes response header(s);
>   			 * subsequent fragments contains only data.
>   			 */
> -			skb_put(skb, oq->max_single_buffer_size);
> -			read_idx++;
> -			desc_used++;
> -			if (read_idx == oq->max_count)
> -				read_idx = 0;
> -
> -			shinfo = skb_shinfo(skb);
> +			if (skb) {
> +				skb_put(skb, oq->max_single_buffer_size);
> +				shinfo = skb_shinfo(skb);
> +			}
>   			data_len = buff_info->len - oq->max_single_buffer_size;
>   			while (data_len) {
>   				dma_unmap_page(oq->dev, oq->desc_ring[read_idx].buffer_ptr,
> @@ -434,10 +434,11 @@ static int __octep_oq_process_rx(struct octep_device *oct,
>   					data_len -= oq->buffer_size;
>   				}
>   
> -				skb_add_rx_frag(skb, shinfo->nr_frags,
> -						buff_info->page, 0,
> -						buff_info->len,
> -						buff_info->len);
> +				if (skb)
> +					skb_add_rx_frag(skb, shinfo->nr_frags,
> +							buff_info->page, 0,
> +							buff_info->len,
> +							buff_info->len);
>   				buff_info->page = NULL;
>   				read_idx++;
>   				desc_used++;
> @@ -446,6 +447,9 @@ static int __octep_oq_process_rx(struct octep_device *oct,
>   			}
>   		}
>   
> +		if (!skb)
> +			continue;

Instead of adding multiple checks for !skb, I think it would be better 
to implement an helper to unmmap/flush all the fragment buffers used by 
the dropped packet, call such helper at skb allocation failure and 
continue looping earlier/at that point.

Thanks,

Paolo