netdev - Re: [PATCH net 1/1] s390/ism: fix receive message buffer allocation

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]

Date: Thu, 28 Mar 2024 16:59:53 +0100
From: Gerd Bayer <gbayer@...ux.ibm.com>
To: Wenjia Zhang <wenjia@...ux.ibm.com>, Wen Gu <guwen@...ux.alibaba.com>,
        Heiko Carstens <hca@...ux.ibm.com>, pasic@...ux.ibm.com,
        schnelle@...ux.ibm.com, Christoph Hellwig <hch@....de>
Cc: linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        Alexandra Winter
 <wintera@...ux.ibm.com>,
        Thorsten Winkler <twinkler@...ux.ibm.com>,
        Vasily
 Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle
 <svens@...ux.ibm.com>
Subject: Re: [PATCH net 1/1] s390/ism: fix receive message buffer allocation

On Thu, 2024-03-28 at 16:41 +0100, Gerd Bayer wrote:
> Since [1], dma_alloc_coherent() does not accept requests for GFP_COMP
> anymore, even on archs that may be able to fulfill this.
> Functionality that
> relied on the receive buffer being a compound page broke at that
> point:
> The SMC-D protocol, that utilizes the ism device driver, passes
> receive
> buffers to the splice processor in a struct splice_pipe_desc with a
> single entry list of struct pages. As the buffer is no longer a
> compound
> page, the splice processor now rejects requests to handle more than a
> page worth of data.
> 
> Replace dma_alloc_coherent() and allocate a buffer with kmalloc()
> then
> create a DMA map for it with dma_map_page(). Since only receive
> buffers
> on ISM devices use DMA, qualify the mapping as FROM_DEVICE.
> Since ISM devices are available on arch s390, only and on that arch
> all
> DMA is coherent, there is no need to introduce and export some kind
> of
> dma_sync_to_cpu() method to be called by the SMC-D protocol layer.
> 
> Analogously, replace dma_free_coherent by a two step dma_unmap_page,
> then kfree to free the receive buffer.
> 
> [1] https://lore.kernel.org/all/20221113163535.884299-1-hch@lst.de/
> 
> Fixes: c08004eede4b ("s390/ism: don't pass bogus GFP_ flags to
> dma_alloc_coherent")

Late adding Christoph as the "blamed" committer.

> Signed-off-by: Gerd Bayer <gbayer@...ux.ibm.com>
> ---
>  drivers/s390/net/ism_drv.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> index 2c8e964425dc..25911b887e5e 100644
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -14,6 +14,8 @@
>  #include <linux/err.h>
>  #include <linux/ctype.h>
>  #include <linux/processor.h>
> +#include <linux/dma-direction.h>
> +#include <linux/gfp_types.h>
>  
>  #include "ism.h"
>  
> @@ -292,13 +294,15 @@ static int ism_read_local_gid(struct ism_dev
> *ism)
>  static void ism_free_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
>  {
>  	clear_bit(dmb->sba_idx, ism->sba_bitmap);
> -	dma_free_coherent(&ism->pdev->dev, dmb->dmb_len,
> -			  dmb->cpu_addr, dmb->dma_addr);
> +	dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
> +		       DMA_FROM_DEVICE);
> +	kfree(dmb->cpu_addr);
>  }
>  
>  static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
>  {
>  	unsigned long bit;
> +	int rc;
>  
>  	if (PAGE_ALIGN(dmb->dmb_len) > dma_get_max_seg_size(&ism-
> >pdev->dev))
>  		return -EINVAL;
> @@ -315,14 +319,27 @@ static int ism_alloc_dmb(struct ism_dev *ism,
> struct ism_dmb *dmb)
>  	    test_and_set_bit(dmb->sba_idx, ism->sba_bitmap))
>  		return -EINVAL;
>  
> -	dmb->cpu_addr = dma_alloc_coherent(&ism->pdev->dev, dmb-
> >dmb_len,
> -					   &dmb->dma_addr,
> -					   GFP_KERNEL | __GFP_NOWARN
> |
> -					   __GFP_NOMEMALLOC |
> __GFP_NORETRY);
> -	if (!dmb->cpu_addr)
> -		clear_bit(dmb->sba_idx, ism->sba_bitmap);
> +	dmb->cpu_addr = kmalloc(dmb->dmb_len, GFP_KERNEL |
> __GFP_NOWARN |
> +				__GFP_COMP | __GFP_NOMEMALLOC |
> __GFP_NORETRY);
> +	if (!dmb->cpu_addr) {
> +		rc = -ENOMEM;
> +		goto out_bit;
> +	}
> +	dmb->dma_addr = dma_map_page(&ism->pdev->dev,
> +				     virt_to_page(dmb->cpu_addr), 0,
> +				     dmb->dmb_len, DMA_FROM_DEVICE);
> +	if (dma_mapping_error(&ism->pdev->dev, dmb->dma_addr)) {
> +		rc = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	return 0;
>  
> -	return dmb->cpu_addr ? 0 : -ENOMEM;
> +out_free:
> +	kfree(dmb->cpu_addr);
> +out_bit:
> +	clear_bit(dmb->sba_idx, ism->sba_bitmap);
> +	return rc;
>  }
>  
>  int ism_register_dmb(struct ism_dev *ism, struct ism_dmb *dmb,