linux-kernel - Re: [RFC PATCH 5/5] staging: vt6655: implement allocation failure handling

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: <YyhAhiUlclk7HxE3@kadam>
Date:   Mon, 19 Sep 2022 13:12:22 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Nam Cao <namcaov@...il.com>
Cc:     forest@...ttletooquiet.net, gregkh@...uxfoundation.org,
        philipp.g.hortmann@...il.com, linux-kernel@...r.kernel.org,
        linux-staging@...ts.linux.dev
Subject: Re: [RFC PATCH 5/5] staging: vt6655: implement allocation failure
 handling

On Thu, Sep 15, 2022 at 10:29:36PM +0200, Nam Cao wrote:
> The function device_rx_srv does not handle allocation failure very well.
> Currently, it performs these steps:
>         - Unmap DMA buffer and hand over the buffer to mac80211

Does the unmapping happens in vnt_receive_frame();?

>         - Allocate and dma-map new buffer

Is the new buffer for the next frame?  So in your patch if we don't
have space for the next frame then we do not process the current frame?
(Rhetorical questions are a bad idea on development lists.  I genuinely
don't know the answers to these questions).

>         - If allocation fails, abort
> 
> The problem is that, it aborts while still marking the buffer as
> OWNED_BY_HOST. So when this function is called again in the future, it
> incorrectly perceives the same buffer as valid and dma-unmap and hand
> over this buffer to mac80211 again.

Where is it supposed to get marked as OWNED_BY_HOST?

> 
> Re-implement this function to do things in a different order:
>         - Allocate and dma-map new buffer
>         - If allocation fails, abort and give up the ownership of the
>           buffer (so that the device can re-use this buffer)
>         - If allocation does not fail: unmap dma buffer and hand over
>           the buffer to mac80211
> 
> Thus, when the driver cannot allocate new buffer, it simply discards the
> received data and re-use the current buffer.
> 
> Signed-off-by: Nam Cao <namcaov@...il.com>
> ---
>  drivers/staging/vt6655/device_main.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index ca6c4266f010..8ae4ecca2ee3 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -826,6 +826,7 @@ static void device_free_td1_ring(struct vnt_private *priv)
>  static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
>  {
>  	struct vnt_rx_desc *rd;
> +	struct vnt_rd_info new_info;
>  	int works = 0;
>  
>  	for (rd = priv->pCurrRD[idx];
> @@ -837,16 +838,18 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
>  		if (!rd->rd_info->skb)
>  			break;
>  
> -		vnt_receive_frame(priv, rd);
> -
> -		if (!device_alloc_rx_buf(priv, rd->rd_info)) {
> +		if (!device_alloc_rx_buf(priv, &new_info)) {
>  			dev_err(&priv->pcid->dev,
>  				"can not allocate rx buf\n");
> +			rd->rd0.owner = OWNED_BY_NIC;
>  			break;
> -		} else {
> -			device_init_rx_desc(priv, rd);
>  		}
>  
> +		vnt_receive_frame(priv, rd);
> +
> +		memcpy(rd->rd_info, &new_info, sizeof(new_info));
> +		device_init_rx_desc(priv, rd);
> +
>  		rd->rd0.owner = OWNED_BY_NIC;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The device_init_rx_desc() function sets it to OWNED_BY_NIC so this line
can be deleted.

regards,
dan carpenter