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]

Message-ID: <50b6811dbb53b19385260f6b0dffa1534f8e341e.camel@linux.ibm.com>
Date: Fri, 05 Apr 2024 12:42:02 +0200
From: Gerd Bayer <gbayer@...ux.ibm.com>
To: Christoph Hellwig <hch@....de>, Paolo Abeni <pabeni@...hat.com>
Cc: 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, 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 Fri, 2024-04-05 at 08:49 +0200, Christoph Hellwig wrote:
> On Thu, Apr 04, 2024 at 01:10:20PM +0200, Gerd Bayer wrote:
> > > Why can't you use get_free_pages() (or similar) here? (possibly
> > > rounding up to the relevant page_aligned size). 
> > 
> > Thanks Paolo for your suggestion. However, I wanted to stay as
> > close to the implementation pre [1] - that used to use __GFP_COMP,
> > too. I'd rather avoid to change interfaces from "cpu_addr" to
> > "struct page*" at this point. In the long run, I'd like to drop the
> > requirement for
> 
> The right interface actually is to simply use folio_alloc, which adds
> __GFP_COMP and is a fully supported and understood interface. You can
> just convert the folio to a kernel virtual address using
> folio_address() right after allocating it.

Thanks for pointing me to folios.
After a good night's sleep, I figured that I was thinking too
complicated when I dismissed Paolo's suggestion.

> (get_free_pages also retunrs a kernel virtual address, just awkwardly
> as an unsigned long. In doubt don't use this interface for new
> code..)
> 
> > compound pages entirely, since that *appears* to exist primarily
> > for a
> > simplified handling of the interface to splice_to_pipe() in
> > net/smc/smc_rx.c. And of course there might be performance
> > implications...
> 
> While compounds pages might sound awkward, they are the new normal in
> form of folios.  So just use folios.

With the following fixup, my tests were just as successful.
I'll send that out as a v2.

Thank you, Christoph and Paolo!



diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index 25911b887e5e..affb05521e14 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -14,8 +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 <linux/dma-mapping.h>
+#include <linux/mm.h>
 
 #include "ism.h"
 
@@ -296,7 +296,7 @@ static void ism_free_dmb(struct ism_dev *ism,
struct ism_dmb *dmb)
 	clear_bit(dmb->sba_idx, ism->sba_bitmap);
 	dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
 		       DMA_FROM_DEVICE);
-	kfree(dmb->cpu_addr);
+	folio_put(virt_to_folio(dmb->cpu_addr));
 }
 
 static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
@@ -319,8 +319,11 @@ 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 = kmalloc(dmb->dmb_len, GFP_KERNEL |
__GFP_NOWARN |
-				__GFP_COMP | __GFP_NOMEMALLOC |
__GFP_NORETRY);
+	dmb->cpu_addr =
+		folio_address(folio_alloc(GFP_KERNEL | __GFP_NOWARN |
+					  __GFP_NOMEMALLOC |
__GFP_NORETRY,
+					  get_order(dmb->dmb_len)));
+
 	if (!dmb->cpu_addr) {
 		rc = -ENOMEM;
 		goto out_bit;
-- 
2.44.0