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
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 <[email protected]>
---
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)
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: [email protected]
Signed-off-by: Jan Beulich <[email protected]>
--- 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;
}
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 <[email protected]>
--- 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,
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
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
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 <[email protected]>
>
>> 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.
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 <[email protected]>
Reviewed-by: Boris Ostrovsky <[email protected]>
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 <[email protected]>
Reviewed-by: Juergen Gross <[email protected]>
> ---
> 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
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