linux-hyperv.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] swiotlb: check set_memory_decrypted()'s return value
@ 2022-11-21 19:48 Dexuan Cui
  2022-11-21 20:48 ` Robin Murphy
  0 siblings, 1 reply; 3+ messages in thread
From: Dexuan Cui @ 2022-11-21 19:48 UTC (permalink / raw)
  To: hch, m.szyprowski, robin.murphy, iommu
  Cc: linux-kernel, linux-hyperv, Dexuan Cui

swiotlb_update_mem_attributes() is called from a system where decrypted
swiotlb bounce buffers are required. Panic the system if
set_memory_decrypted() fails.

When rmem_swiotlb_device_init() -> set_memory_decrypted(), let's try
to handle it gracefully.

Not sure how to handle the failure in swiotlb_init_late(). Add a WARN().

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 kernel/dma/swiotlb.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 339a990554e7..8e26c09c625c 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -248,12 +248,16 @@ void __init swiotlb_update_mem_attributes(void)
 	struct io_tlb_mem *mem = &io_tlb_default_mem;
 	void *vaddr;
 	unsigned long bytes;
+	int rc;
 
 	if (!mem->nslabs || mem->late_alloc)
 		return;
 	vaddr = phys_to_virt(mem->start);
 	bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
-	set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
+
+	rc = set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
+	if (rc)
+		panic("Failed to decrypt swiotlb bounce buffers (%d)\n", rc);
 
 	mem->vaddr = swiotlb_mem_remap(mem, bytes);
 	if (!mem->vaddr)
@@ -447,8 +451,10 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
 	if (!mem->slots)
 		goto error_slots;
 
-	set_memory_decrypted((unsigned long)vstart,
-			     (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
+	rc = set_memory_decrypted((unsigned long)vstart,
+				  (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
+	WARN(rc, "Failed to decrypt swiotlb bounce buffers (%d)\n", rc);
+
 	swiotlb_init_io_tlb_mem(mem, virt_to_phys(vstart), nslabs, 0, true,
 				default_nareas);
 
@@ -986,6 +992,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
 
 	/* Set Per-device io tlb area to one */
 	unsigned int nareas = 1;
+	int rc = -ENOMEM;
 
 	/*
 	 * Since multiple devices can share the same pool, the private data,
@@ -998,21 +1005,22 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
 			return -ENOMEM;
 
 		mem->slots = kcalloc(nslabs, sizeof(*mem->slots), GFP_KERNEL);
-		if (!mem->slots) {
-			kfree(mem);
-			return -ENOMEM;
-		}
+		if (!mem->slots)
+			goto free_mem;
 
 		mem->areas = kcalloc(nareas, sizeof(*mem->areas),
 				GFP_KERNEL);
-		if (!mem->areas) {
-			kfree(mem->slots);
-			kfree(mem);
-			return -ENOMEM;
+		if (!mem->areas)
+			goto free_slots;
+
+		rc = set_memory_decrypted(
+				(unsigned long)phys_to_virt(rmem->base),
+				rmem->size >> PAGE_SHIFT);
+		if (rc) {
+			pr_err("Failed to decrypt rmem buffer (%d)\n", rc);
+			goto free_areas;
 		}
 
-		set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
-				     rmem->size >> PAGE_SHIFT);
 		swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, SWIOTLB_FORCE,
 					false, nareas);
 		mem->for_alloc = true;
@@ -1025,6 +1033,14 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
 	dev->dma_io_tlb_mem = mem;
 
 	return 0;
+
+free_areas:
+	kfree(mem->areas);
+free_slots:
+	kfree(mem->slots);
+free_mem:
+	kfree(mem);
+	return rc;
 }
 
 static void rmem_swiotlb_device_release(struct reserved_mem *rmem,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] swiotlb: check set_memory_decrypted()'s return value
  2022-11-21 19:48 [PATCH] swiotlb: check set_memory_decrypted()'s return value Dexuan Cui
@ 2022-11-21 20:48 ` Robin Murphy
  2022-11-24  1:51   ` Dexuan Cui
  0 siblings, 1 reply; 3+ messages in thread
From: Robin Murphy @ 2022-11-21 20:48 UTC (permalink / raw)
  To: Dexuan Cui, hch, m.szyprowski, iommu; +Cc: linux-kernel, linux-hyperv

On 2022-11-21 19:48, Dexuan Cui wrote:
> swiotlb_update_mem_attributes() is called from a system where decrypted
> swiotlb bounce buffers are required. Panic the system if
> set_memory_decrypted() fails.
> 
> When rmem_swiotlb_device_init() -> set_memory_decrypted(), let's try
> to handle it gracefully.
> 
> Not sure how to handle the failure in swiotlb_init_late(). Add a WARN().
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>   kernel/dma/swiotlb.c | 42 +++++++++++++++++++++++++++++-------------
>   1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 339a990554e7..8e26c09c625c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -248,12 +248,16 @@ void __init swiotlb_update_mem_attributes(void)
>   	struct io_tlb_mem *mem = &io_tlb_default_mem;
>   	void *vaddr;
>   	unsigned long bytes;
> +	int rc;
>   
>   	if (!mem->nslabs || mem->late_alloc)
>   		return;
>   	vaddr = phys_to_virt(mem->start);
>   	bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
> -	set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> +
> +	rc = set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> +	if (rc)
> +		panic("Failed to decrypt swiotlb bounce buffers (%d)\n", rc);

Aww, I just cleaned up all the panics! AFAICS we could warn and set 
mem->nslabs to 0 here to semi-gracefully disable SWIOTLB (presumably 
decryption failure is sufficiently unexpected that "leaking" the SWIOTLB 
memory is the least of the system's problems). Or indeed just warn and 
do nothing as in the swiotlb_init_late() case below - what's with the 
inconsistency? In either path we have the same expectation that 
decryption succeeds (or does nothing, successfully), so failure is no 
more or less fatal to later SWIOTLB operation depending on when the 
architecture chose to set it up.

>   	mem->vaddr = swiotlb_mem_remap(mem, bytes);
>   	if (!mem->vaddr)
> @@ -447,8 +451,10 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
>   	if (!mem->slots)
>   		goto error_slots;
>   
> -	set_memory_decrypted((unsigned long)vstart,
> -			     (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
> +	rc = set_memory_decrypted((unsigned long)vstart,
> +				  (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
> +	WARN(rc, "Failed to decrypt swiotlb bounce buffers (%d)\n", rc);
> +
>   	swiotlb_init_io_tlb_mem(mem, virt_to_phys(vstart), nslabs, 0, true,
>   				default_nareas);
>   
> @@ -986,6 +992,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
>   
>   	/* Set Per-device io tlb area to one */
>   	unsigned int nareas = 1;
> +	int rc = -ENOMEM;
>   
>   	/*
>   	 * Since multiple devices can share the same pool, the private data,
> @@ -998,21 +1005,22 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
>   			return -ENOMEM;
>   
>   		mem->slots = kcalloc(nslabs, sizeof(*mem->slots), GFP_KERNEL);
> -		if (!mem->slots) {
> -			kfree(mem);
> -			return -ENOMEM;
> -		}
> +		if (!mem->slots)
> +			goto free_mem;
>   
>   		mem->areas = kcalloc(nareas, sizeof(*mem->areas),
>   				GFP_KERNEL);
> -		if (!mem->areas) {
> -			kfree(mem->slots);
> -			kfree(mem);
> -			return -ENOMEM;
> +		if (!mem->areas)
> +			goto free_slots;
> +
> +		rc = set_memory_decrypted(
> +				(unsigned long)phys_to_virt(rmem->base),
> +				rmem->size >> PAGE_SHIFT);
> +		if (rc) {
> +			pr_err("Failed to decrypt rmem buffer (%d)\n", rc);
> +			goto free_areas;

So in 3 init paths we have 3 different outcomes from the same thing :(

1: make the whole system unusable
2: continue with possible data corruption (or at least weird DMA errors) 
if devices still see encrypted memory contents
3: cleanly disable SWIOTLB, thus also all subsequent attempts to use it

(OK, for the rmem case this isn't actually 3 since falling back to 
io_tlb_default_mem might work out more like 2, but hopefully you get my 
point)

Thanks,
Robin.

>   		}
>   
> -		set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
> -				     rmem->size >> PAGE_SHIFT);
>   		swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, SWIOTLB_FORCE,
>   					false, nareas);
>   		mem->for_alloc = true;
> @@ -1025,6 +1033,14 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
>   	dev->dma_io_tlb_mem = mem;
>   
>   	return 0;
> +
> +free_areas:
> +	kfree(mem->areas);
> +free_slots:
> +	kfree(mem->slots);
> +free_mem:
> +	kfree(mem);
> +	return rc;
>   }
>   
>   static void rmem_swiotlb_device_release(struct reserved_mem *rmem,

^ permalink raw reply	[flat|nested] 3+ messages in thread
* RE: [PATCH] swiotlb: check set_memory_decrypted()'s return value
  2022-11-21 20:48 ` Robin Murphy
@ 2022-11-24  1:51   ` Dexuan Cui
  0 siblings, 0 replies; 3+ messages in thread
From: Dexuan Cui @ 2022-11-24  1:51 UTC (permalink / raw)
  To: Robin Murphy, hch, m.szyprowski, iommu; +Cc: linux-kernel, linux-hyperv

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Monday, November 21, 2022 12:49 PM
> > [...]
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > @@ -248,12 +248,16 @@ void __init swiotlb_update_mem_attributes(void)
> >   	struct io_tlb_mem *mem = &io_tlb_default_mem;
> >   	void *vaddr;
> >   	unsigned long bytes;
> > +	int rc;
> >
> >   	if (!mem->nslabs || mem->late_alloc)
> >   		return;
> >   	vaddr = phys_to_virt(mem->start);
> >   	bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
> > -	set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> > +
> > +	rc = set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
> > +	if (rc)
> > +		panic("Failed to decrypt swiotlb bounce buffers (%d)\n", rc);
> 
> Aww, I just cleaned up all the panics! AFAICS we could warn and set
Sorry, I didn't know about that... :-)

> mem->nslabs to 0 here to semi-gracefully disable SWIOTLB (presumably
> decryption failure is sufficiently unexpected that "leaking" the SWIOTLB
> memory is the least of the system's problems). Or indeed just warn and
> do nothing as in the swiotlb_init_late() case below - what's with the
> inconsistency? In either path we have the same expectation that
> decryption succeeds (or does nothing, successfully), so failure is no
> more or less fatal to later SWIOTLB operation depending on when the
> architecture chose to set it up.

I agree it's better to print an error message rather than panic here, but
anyway the kernel will panic later, e.g. when an AMD SEV-SNP guest runs
on Hyper-V, in case this set_memory_decrypted() fails, we set
mem->nslabs to 0, and next we'll hit a panic soon when the storage
driver calls swiotlb_tbl_map_single():
"Kernel panic - not syncing: Can not allocate SWIOTLB buffer earlier and 
can't now provide you with the DMA bounce buffer".

> So in 3 init paths we have 3 different outcomes from the same thing :(
> 
> 1: make the whole system unusable
> 2: continue with possible data corruption (or at least weird DMA errors)
> if devices still see encrypted memory contents
> 3: cleanly disable SWIOTLB, thus also all subsequent attempts to use it
> 
> (OK, for the rmem case this isn't actually 3 since falling back to
> io_tlb_default_mem might work out more like 2, but hopefully you get my
> point)
> 
> Thanks,
> Robin.

How do you like this new version: 
1) I removed the panic().
2) For swiotlb_update_mem_attributes() and swiotlb_init_late(), I print
an error message and disable swiotlb: the error seems so bad that IMO
we have to disable swiotlb.
3) No change to rmem_swiotlb_device_init(). The error in this function
doesn't seem fatal to me.

The bottom line is that set_memory_decrypted() should not fail silently
and cause weird issues later... BTW, normally IMO set_memory_decrypted()
doesn't fail, but I did see a failure because we're expectd to retry
(the failure is fixed by
https://lwn.net/ml/linux-kernel/20221121195151.21812-3-decui%40microsoft.com/)

--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -248,12 +248,20 @@ void __init swiotlb_update_mem_attributes(void)
        struct io_tlb_mem *mem = &io_tlb_default_mem;
        void *vaddr;
        unsigned long bytes;
+       int rc;
 
        if (!mem->nslabs || mem->late_alloc)
                return;
        vaddr = phys_to_virt(mem->start);
        bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
-       set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
+
+       rc = set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
+       if (rc) {
+               pr_err("Failed to decrypt swiotlb buffer (%d): disabling swiotlb!\n",
+                      rc);
+               mem->nslabs = 0;
+               return;
+       }
 
        mem->vaddr = swiotlb_mem_remap(mem, bytes);
        if (!mem->vaddr)
@@ -391,7 +399,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
        struct io_tlb_mem *mem = &io_tlb_default_mem;
        unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
        unsigned char *vstart = NULL;
-       unsigned int order, area_order;
+       unsigned int order, area_order, slot_order;
        bool retried = false;
        int rc = 0;
 
@@ -442,19 +450,29 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
        if (!mem->areas)
                goto error_area;
 
+       slot_order = get_order(array_size(sizeof(*mem->slots), nslabs));
        mem->slots = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
-               get_order(array_size(sizeof(*mem->slots), nslabs)));
+               slot_order);
        if (!mem->slots)
                goto error_slots;
 
-       set_memory_decrypted((unsigned long)vstart,
-                            (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
+       rc = set_memory_decrypted((unsigned long)vstart,
+                                 (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
+       if (rc) {
+               pr_err("Failed to decrypt swiotlb buffer (%d): disabling swiotlb!\n",
+                      rc);
+               mem->nslabs = 0;
+               goto error_decrypted;
+       }
+
        swiotlb_init_io_tlb_mem(mem, virt_to_phys(vstart), nslabs, 0, true,
                                default_nareas);
 
        swiotlb_print_info();
        return 0;
 
+error_decrypted:
+       free_pages((unsigned long)mem->slots, slot_order);
 error_slots:
        free_pages((unsigned long)mem->areas, area_order);
 error_area:
@@ -986,6 +1004,7 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
 
        /* Set Per-device io tlb area to one */
        unsigned int nareas = 1;
+       int rc = -ENOMEM;
 
        /*
         * Since multiple devices can share the same pool, the private data,
@@ -998,21 +1017,22 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
                        return -ENOMEM;
 
                mem->slots = kcalloc(nslabs, sizeof(*mem->slots), GFP_KERNEL);
-               if (!mem->slots) {
-                       kfree(mem);
-                       return -ENOMEM;
-               }
+               if (!mem->slots)
+                       goto free_mem;
 
                mem->areas = kcalloc(nareas, sizeof(*mem->areas),
                                GFP_KERNEL);
-               if (!mem->areas) {
-                       kfree(mem->slots);
-                       kfree(mem);
-                       return -ENOMEM;
+               if (!mem->areas)
+                       goto free_slots;
+
+               rc = set_memory_decrypted(
+                               (unsigned long)phys_to_virt(rmem->base),
+                               rmem->size >> PAGE_SHIFT);
+               if (rc) {
+                       pr_err("Failed to decrypt rmem buffer (%d)\n", rc);
+                       goto free_areas;
                }
 
-               set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
-                                    rmem->size >> PAGE_SHIFT);
                swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, SWIOTLB_FORCE,
                                        false, nareas);
                mem->for_alloc = true;
@@ -1025,6 +1045,14 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
        dev->dma_io_tlb_mem = mem;
 
        return 0;
+
+free_areas:
+       kfree(mem->areas);
+free_slots:
+       kfree(mem->slots);
+free_mem:
+       kfree(mem);
+       return rc;
 }

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-24  1:51 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21 19:48 [PATCH] swiotlb: check set_memory_decrypted()'s return value Dexuan Cui
2022-11-21 20:48 ` Robin Murphy
2022-11-24  1:51   ` Dexuan Cui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).