linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] xen/privcmd: misc corrections
@ 2021-09-22 10:14 Jan Beulich
  2021-09-22 10:16 ` [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages Jan Beulich
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Jan Beulich @ 2021-09-22 10:14 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml

The three changes here are largely independent, except for a contextual
dependency between 2 and 3. Note that patch 1 will need actually testing,
on Arm.

1: replace kcalloc() by kvcalloc() when allocating empty pages
2: fix error handling in mmap-resource processing
3: drop "pages" parameter from xen_remap_pfn()

Jan


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages
  2021-09-22 10:14 [PATCH 0/3] xen/privcmd: misc corrections Jan Beulich
@ 2021-09-22 10:16 ` Jan Beulich
  2021-09-23 12:30   ` Juergen Gross
  2021-09-22 10:17 ` [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing Jan Beulich
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-09-22 10:16 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml

Osstest has been suffering test failures for a little while from order-4
allocation failures, resulting from alloc_empty_pages() calling
kcalloc(). As there's no need for physically contiguous space here,
switch to kvcalloc().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: I cannot really test this, as alloc_empty_pages() only gets used in
     the auto-translated case (i.e. on Arm or PVH Dom0, the latter of
     which I'm not trusting enough yet to actually start playing with
     guests).

There are quite a few more kcalloc() where it's not immediately clear
how large the element counts could possibly grow nor whether it would be
fine to replace them (i.e. physically contiguous space not required).

I wasn't sure whether to Cc stable@ here; the issue certainly has been
present for quite some time. But it didn't look to cause issues until
recently.

--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -420,7 +420,7 @@ static int alloc_empty_pages(struct vm_a
 	int rc;
 	struct page **pages;
 
-	pages = kcalloc(numpgs, sizeof(pages[0]), GFP_KERNEL);
+	pages = kvcalloc(numpgs, sizeof(pages[0]), GFP_KERNEL);
 	if (pages == NULL)
 		return -ENOMEM;
 
@@ -428,7 +428,7 @@ static int alloc_empty_pages(struct vm_a
 	if (rc != 0) {
 		pr_warn("%s Could not alloc %d pfns rc:%d\n", __func__,
 			numpgs, rc);
-		kfree(pages);
+		kvfree(pages);
 		return -ENOMEM;
 	}
 	BUG_ON(vma->vm_private_data != NULL);
@@ -912,7 +912,7 @@ static void privcmd_close(struct vm_area
 	else
 		pr_crit("unable to unmap MFN range: leaking %d pages. rc=%d\n",
 			numpgs, rc);
-	kfree(pages);
+	kvfree(pages);
 }
 
 static vm_fault_t privcmd_fault(struct vm_fault *vmf)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing
  2021-09-22 10:14 [PATCH 0/3] xen/privcmd: misc corrections Jan Beulich
  2021-09-22 10:16 ` [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages Jan Beulich
@ 2021-09-22 10:17 ` Jan Beulich
  2021-09-22 13:29   ` Boris Ostrovsky
  2021-09-22 10:18 ` [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn() Jan Beulich
  2021-10-05  6:39 ` [PATCH 0/3] xen/privcmd: misc corrections Juergen Gross
  3 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-09-22 10:17 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml

xen_pfn_t is the same size as int only on 32-bit builds (and not even
on Arm32). Hence pfns[] can't be used directly to read individual error
values returned from xen_remap_domain_mfn_array(); every other error
indicator would be skipped/ignored on 64-bit.

Fixes: 3ad0876554ca ("xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE")
Cc: stable@vger.kernel.org
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -803,11 +803,12 @@ static long privcmd_ioctl_mmap_resource(
 		unsigned int domid =
 			(xdata.flags & XENMEM_rsrc_acq_caller_owned) ?
 			DOMID_SELF : kdata.dom;
-		int num;
+		int num, *errs = (int *)pfns;
 
+		BUILD_BUG_ON(sizeof(*errs) > sizeof(*pfns));
 		num = xen_remap_domain_mfn_array(vma,
 						 kdata.addr & PAGE_MASK,
-						 pfns, kdata.num, (int *)pfns,
+						 pfns, kdata.num, errs,
 						 vma->vm_page_prot,
 						 domid,
 						 vma->vm_private_data);
@@ -817,7 +818,7 @@ static long privcmd_ioctl_mmap_resource(
 			unsigned int i;
 
 			for (i = 0; i < num; i++) {
-				rc = pfns[i];
+				rc = errs[i];
 				if (rc < 0)
 					break;
 			}


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn()
  2021-09-22 10:14 [PATCH 0/3] xen/privcmd: misc corrections Jan Beulich
  2021-09-22 10:16 ` [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages Jan Beulich
  2021-09-22 10:17 ` [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing Jan Beulich
@ 2021-09-22 10:18 ` Jan Beulich
  2021-09-22 14:02   ` Boris Ostrovsky
  2021-10-05  6:39 ` [PATCH 0/3] xen/privcmd: misc corrections Juergen Gross
  3 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-09-22 10:18 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml

The function doesn't use it and all of its callers say in a comment that
their respective arguments are to be non-NULL only in auto-translated
mode. Since xen_remap_domain_mfn_array() isn't supposed to be used by
non-PV, drop the parameter there as well. It was bogusly passed as non-
NULL (PRIV_VMA_LOCKED) by its only caller anyway. For
xen_remap_domain_gfn_range(), otoh, it's not clear at all why this
wouldn't want / might not need to gain auto-translated support down the
road, so the parameter is retained there despite now remaining unused
(and the only caller passing NULL); correct a respective comment as
well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2398,7 +2398,7 @@ static int remap_area_pfn_pte_fn(pte_t *
 
 int xen_remap_pfn(struct vm_area_struct *vma, unsigned long addr,
 		  xen_pfn_t *pfn, int nr, int *err_ptr, pgprot_t prot,
-		  unsigned int domid, bool no_translate, struct page **pages)
+		  unsigned int domid, bool no_translate)
 {
 	int err = 0;
 	struct remap_data rmd;
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -257,7 +257,7 @@ static long privcmd_ioctl_mmap(struct fi
 	LIST_HEAD(pagelist);
 	struct mmap_gfn_state state;
 
-	/* We only support privcmd_ioctl_mmap_batch for auto translated. */
+	/* We only support privcmd_ioctl_mmap_batch for non-auto-translated. */
 	if (xen_feature(XENFEAT_auto_translated_physmap))
 		return -ENOSYS;
 
@@ -810,8 +810,7 @@ static long privcmd_ioctl_mmap_resource(
 						 kdata.addr & PAGE_MASK,
 						 pfns, kdata.num, errs,
 						 vma->vm_page_prot,
-						 domid,
-						 vma->vm_private_data);
+						 domid);
 		if (num < 0)
 			rc = num;
 		else if (num != kdata.num) {
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -64,12 +64,12 @@ static inline void xen_destroy_contiguou
 #if defined(CONFIG_XEN_PV)
 int xen_remap_pfn(struct vm_area_struct *vma, unsigned long addr,
 		  xen_pfn_t *pfn, int nr, int *err_ptr, pgprot_t prot,
-		  unsigned int domid, bool no_translate, struct page **pages);
+		  unsigned int domid, bool no_translate);
 #else
 static inline int xen_remap_pfn(struct vm_area_struct *vma, unsigned long addr,
 				xen_pfn_t *pfn, int nr, int *err_ptr,
 				pgprot_t prot,  unsigned int domid,
-				bool no_translate, struct page **pages)
+				bool no_translate)
 {
 	BUG();
 	return 0;
@@ -146,7 +146,7 @@ static inline int xen_remap_domain_gfn_a
 	 */
 	BUG_ON(err_ptr == NULL);
 	return xen_remap_pfn(vma, addr, gfn, nr, err_ptr, prot, domid,
-			     false, pages);
+			     false);
 }
 
 /*
@@ -158,7 +158,6 @@ static inline int xen_remap_domain_gfn_a
  * @err_ptr: Returns per-MFN error status.
  * @prot:    page protection mask
  * @domid:   Domain owning the pages
- * @pages:   Array of pages if this domain has an auto-translated physmap
  *
  * @mfn and @err_ptr may point to the same buffer, the MFNs will be
  * overwritten by the error codes after they are mapped.
@@ -169,14 +168,13 @@ static inline int xen_remap_domain_gfn_a
 static inline int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
 					     unsigned long addr, xen_pfn_t *mfn,
 					     int nr, int *err_ptr,
-					     pgprot_t prot, unsigned int domid,
-					     struct page **pages)
+					     pgprot_t prot, unsigned int domid)
 {
 	if (xen_feature(XENFEAT_auto_translated_physmap))
 		return -EOPNOTSUPP;
 
 	return xen_remap_pfn(vma, addr, mfn, nr, err_ptr, prot, domid,
-			     true, pages);
+			     true);
 }
 
 /* xen_remap_domain_gfn_range() - map a range of foreign frames
@@ -200,8 +198,7 @@ static inline int xen_remap_domain_gfn_r
 	if (xen_feature(XENFEAT_auto_translated_physmap))
 		return -EOPNOTSUPP;
 
-	return xen_remap_pfn(vma, addr, &gfn, nr, NULL, prot, domid, false,
-			     pages);
+	return xen_remap_pfn(vma, addr, &gfn, nr, NULL, prot, domid, false);
 }
 
 int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing
  2021-09-22 10:17 ` [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing Jan Beulich
@ 2021-09-22 13:29   ` Boris Ostrovsky
  2021-09-22 13:39     ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2021-09-22 13:29 UTC (permalink / raw)
  To: Jan Beulich, Juergen Gross; +Cc: Stefano Stabellini, xen-devel, lkml


On 9/22/21 6:17 AM, Jan Beulich wrote:
> @@ -817,7 +818,7 @@ static long privcmd_ioctl_mmap_resource(
>  			unsigned int i;
>  
>  			for (i = 0; i < num; i++) {
> -				rc = pfns[i];
> +				rc = errs[i];
>  				if (rc < 0)
>  					break;


Can the assignment be moved inside the 'if' statement?


I am also not sure I understand why we need error array at all. Don't we always look at the first error only? In fact, AFAICS this is the only place where we look at the value.


-boris


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing
  2021-09-22 13:29   ` Boris Ostrovsky
@ 2021-09-22 13:39     ` Jan Beulich
  2021-09-22 13:56       ` Boris Ostrovsky
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-09-22 13:39 UTC (permalink / raw)
  To: Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml, Juergen Gross

On 22.09.2021 15:29, Boris Ostrovsky wrote:
> On 9/22/21 6:17 AM, Jan Beulich wrote:
>> @@ -817,7 +818,7 @@ static long privcmd_ioctl_mmap_resource(
>>  			unsigned int i;
>>  
>>  			for (i = 0; i < num; i++) {
>> -				rc = pfns[i];
>> +				rc = errs[i];
>>  				if (rc < 0)
>>  					break;
> 
> 
> Can the assignment be moved inside the 'if' statement?

I wouldn't mind, albeit it's not the purpose of this change. Plus
generally, when I do such elsewhere, I'm frequently told to better
leave things as separate statements. IOW I'm a little surprised by
the request.

> I am also not sure I understand why we need error array at all. Don't we always look at the first error only? In fact, AFAICS this is the only place where we look at the value.

Well, to look at the first error we need to scan the array to find
one. Indeed we bail from here in once we've found a slot which has
failed.

I guess what you're trying to say is that there's room for
improvement. In which case I might agree, but would want to point
out that doing so would mean removing flexibility from the
underlying function(s) (which may or may not be fine depending on
what existing and future requirements there are). And that would
be for another day, if at all.

Jan


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing
  2021-09-22 13:39     ` Jan Beulich
@ 2021-09-22 13:56       ` Boris Ostrovsky
  0 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2021-09-22 13:56 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Stefano Stabellini, xen-devel, lkml, Juergen Gross


On 9/22/21 9:39 AM, Jan Beulich wrote:
> On 22.09.2021 15:29, Boris Ostrovsky wrote:
>> On 9/22/21 6:17 AM, Jan Beulich wrote:
>>> @@ -817,7 +818,7 @@ static long privcmd_ioctl_mmap_resource(
>>>  			unsigned int i;
>>>  
>>>  			for (i = 0; i < num; i++) {
>>> -				rc = pfns[i];
>>> +				rc = errs[i];
>>>  				if (rc < 0)
>>>  					break;
>>
>> Can the assignment be moved inside the 'if' statement?
> I wouldn't mind, albeit it's not the purpose of this change. Plus
> generally, when I do such elsewhere, I'm frequently told to better
> leave things as separate statements. IOW I'm a little surprised by
> the request.


Sure, can be done as a separate patch.


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


>
>> I am also not sure I understand why we need error array at all. Don't we always look at the first error only? In fact, AFAICS this is the only place where we look at the value.
> Well, to look at the first error we need to scan the array to find
> one. Indeed we bail from here in once we've found a slot which has
> failed.
>
> I guess what you're trying to say is that there's room for
> improvement. In which case I might agree, but would want to point
> out that doing so would mean removing flexibility from the
> underlying function(s) (which may or may not be fine depending on
> what existing and future requirements there are).


We haven't needed this for a while and IMO existing code, with overloading the meaning of the pfn array, is rather confusing, unnecessarily complicated and error-prone (thus your patch).


>  And that would
> be for another day, if at all.


True.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn()
  2021-09-22 10:18 ` [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn() Jan Beulich
@ 2021-09-22 14:02   ` Boris Ostrovsky
  0 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2021-09-22 14:02 UTC (permalink / raw)
  To: Jan Beulich, Juergen Gross; +Cc: Stefano Stabellini, xen-devel, lkml


On 9/22/21 6:18 AM, Jan Beulich wrote:
> The function doesn't use it and all of its callers say in a comment that
> their respective arguments are to be non-NULL only in auto-translated
> mode. Since xen_remap_domain_mfn_array() isn't supposed to be used by
> non-PV, drop the parameter there as well. It was bogusly passed as non-
> NULL (PRIV_VMA_LOCKED) by its only caller anyway. For
> xen_remap_domain_gfn_range(), otoh, it's not clear at all why this
> wouldn't want / might not need to gain auto-translated support down the
> road, so the parameter is retained there despite now remaining unused
> (and the only caller passing NULL); correct a respective comment as
> well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages
  2021-09-22 10:16 ` [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages Jan Beulich
@ 2021-09-23 12:30   ` Juergen Gross
  0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2021-09-23 12:30 UTC (permalink / raw)
  To: Jan Beulich, Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml


[-- Attachment #1.1.1: Type: text/plain, Size: 1209 bytes --]

On 22.09.21 12:16, Jan Beulich wrote:
> Osstest has been suffering test failures for a little while from order-4
> allocation failures, resulting from alloc_empty_pages() calling
> kcalloc(). As there's no need for physically contiguous space here,
> switch to kvcalloc().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> RFC: I cannot really test this, as alloc_empty_pages() only gets used in
>       the auto-translated case (i.e. on Arm or PVH Dom0, the latter of
>       which I'm not trusting enough yet to actually start playing with
>       guests).
> 
> There are quite a few more kcalloc() where it's not immediately clear
> how large the element counts could possibly grow nor whether it would be
> fine to replace them (i.e. physically contiguous space not required).

I don't think those are an issue. Per default the sizes seem to be well
below a single page.

> I wasn't sure whether to Cc stable@ here; the issue certainly has been
> present for quite some time. But it didn't look to cause issues until
> recently.

I'd rather add it to stable. Its not as if the patch had a high
complexity.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/3] xen/privcmd: misc corrections
  2021-09-22 10:14 [PATCH 0/3] xen/privcmd: misc corrections Jan Beulich
                   ` (2 preceding siblings ...)
  2021-09-22 10:18 ` [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn() Jan Beulich
@ 2021-10-05  6:39 ` Juergen Gross
  3 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2021-10-05  6:39 UTC (permalink / raw)
  To: Jan Beulich, Boris Ostrovsky; +Cc: Stefano Stabellini, xen-devel, lkml


[-- Attachment #1.1.1: Type: text/plain, Size: 438 bytes --]

On 22.09.21 12:14, Jan Beulich wrote:
> The three changes here are largely independent, except for a contextual
> dependency between 2 and 3. Note that patch 1 will need actually testing,
> on Arm.
> 
> 1: replace kcalloc() by kvcalloc() when allocating empty pages
> 2: fix error handling in mmap-resource processing
> 3: drop "pages" parameter from xen_remap_pfn()

Series pushed to xen/tip.git for-linus-5.15b


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-10-05  6:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 10:14 [PATCH 0/3] xen/privcmd: misc corrections Jan Beulich
2021-09-22 10:16 ` [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages Jan Beulich
2021-09-23 12:30   ` Juergen Gross
2021-09-22 10:17 ` [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing Jan Beulich
2021-09-22 13:29   ` Boris Ostrovsky
2021-09-22 13:39     ` Jan Beulich
2021-09-22 13:56       ` Boris Ostrovsky
2021-09-22 10:18 ` [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn() Jan Beulich
2021-09-22 14:02   ` Boris Ostrovsky
2021-10-05  6:39 ` [PATCH 0/3] xen/privcmd: misc corrections Juergen Gross

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).