2022-09-15 09:14:16

by Jürgen Groß

[permalink] [raw]
Subject: [PATCH] xen/privcmd: remove privcmd_ioctl_mmap()

The IOCTL_PRIVCMD_MMAP isn't in use by Xen since at least Xen 4.0.

Remove it from the privcmd driver.

Signed-off-by: Juergen Gross <[email protected]>
---
drivers/xen/privcmd.c | 138 +------------------------------------
include/uapi/xen/privcmd.h | 2 -
include/xen/xen-ops.h | 24 -------
3 files changed, 1 insertion(+), 163 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index e88e8f6f0a33..5136644f3008 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -147,42 +147,7 @@ static int gather_array(struct list_head *pagelist,
/*
* Call function "fn" on each element of the array fragmented
* over a list of pages.
- */
-static int traverse_pages(unsigned nelem, size_t size,
- struct list_head *pos,
- int (*fn)(void *data, void *state),
- void *state)
-{
- void *pagedata;
- unsigned pageidx;
- int ret = 0;
-
- BUG_ON(size > PAGE_SIZE);
-
- pageidx = PAGE_SIZE;
- pagedata = NULL; /* hush, gcc */
-
- while (nelem--) {
- if (pageidx > PAGE_SIZE-size) {
- struct page *page;
- pos = pos->next;
- page = list_entry(pos, struct page, lru);
- pagedata = page_address(page);
- pageidx = 0;
- }
-
- ret = (*fn)(pagedata + pageidx, state);
- if (ret)
- break;
- pageidx += size;
- }
-
- return ret;
-}
-
-/*
- * Similar to traverse_pages, but use each page as a "block" of
- * data to be processed as one unit.
+ * Use each page as a "block" of data to be processed as one unit.
*/
static int traverse_pages_block(unsigned nelem, size_t size,
struct list_head *pos,
@@ -211,103 +176,6 @@ static int traverse_pages_block(unsigned nelem, size_t size,
return ret;
}

-struct mmap_gfn_state {
- unsigned long va;
- struct vm_area_struct *vma;
- domid_t domain;
-};
-
-static int mmap_gfn_range(void *data, void *state)
-{
- struct privcmd_mmap_entry *msg = data;
- struct mmap_gfn_state *st = state;
- struct vm_area_struct *vma = st->vma;
- int rc;
-
- /* Do not allow range to wrap the address space. */
- if ((msg->npages > (LONG_MAX >> PAGE_SHIFT)) ||
- ((unsigned long)(msg->npages << PAGE_SHIFT) >= -st->va))
- return -EINVAL;
-
- /* Range chunks must be contiguous in va space. */
- if ((msg->va != st->va) ||
- ((msg->va+(msg->npages<<PAGE_SHIFT)) > vma->vm_end))
- return -EINVAL;
-
- rc = xen_remap_domain_gfn_range(vma,
- msg->va & PAGE_MASK,
- msg->mfn, msg->npages,
- vma->vm_page_prot,
- st->domain, NULL);
- if (rc < 0)
- return rc;
-
- st->va += msg->npages << PAGE_SHIFT;
-
- return 0;
-}
-
-static long privcmd_ioctl_mmap(struct file *file, void __user *udata)
-{
- struct privcmd_data *data = file->private_data;
- struct privcmd_mmap mmapcmd;
- struct mm_struct *mm = current->mm;
- struct vm_area_struct *vma;
- int rc;
- LIST_HEAD(pagelist);
- struct mmap_gfn_state state;
-
- /* We only support privcmd_ioctl_mmap_batch for non-auto-translated. */
- if (xen_feature(XENFEAT_auto_translated_physmap))
- return -ENOSYS;
-
- if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
- return -EFAULT;
-
- /* If restriction is in place, check the domid matches */
- if (data->domid != DOMID_INVALID && data->domid != mmapcmd.dom)
- return -EPERM;
-
- rc = gather_array(&pagelist,
- mmapcmd.num, sizeof(struct privcmd_mmap_entry),
- mmapcmd.entry);
-
- if (rc || list_empty(&pagelist))
- goto out;
-
- mmap_write_lock(mm);
-
- {
- struct page *page = list_first_entry(&pagelist,
- struct page, lru);
- struct privcmd_mmap_entry *msg = page_address(page);
-
- vma = find_vma(mm, msg->va);
- rc = -EINVAL;
-
- if (!vma || (msg->va != vma->vm_start) || vma->vm_private_data)
- goto out_up;
- vma->vm_private_data = PRIV_VMA_LOCKED;
- }
-
- state.va = vma->vm_start;
- state.vma = vma;
- state.domain = mmapcmd.dom;
-
- rc = traverse_pages(mmapcmd.num, sizeof(struct privcmd_mmap_entry),
- &pagelist,
- mmap_gfn_range, &state);
-
-
-out_up:
- mmap_write_unlock(mm);
-
-out:
- free_page_list(&pagelist);
-
- return rc;
-}
-
struct mmap_batch_state {
domid_t domain;
unsigned long va;
@@ -844,10 +712,6 @@ static long privcmd_ioctl(struct file *file,
ret = privcmd_ioctl_hypercall(file, udata);
break;

- case IOCTL_PRIVCMD_MMAP:
- ret = privcmd_ioctl_mmap(file, udata);
- break;
-
case IOCTL_PRIVCMD_MMAPBATCH:
ret = privcmd_ioctl_mmap_batch(file, udata, 1);
break;
diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h
index d2029556083e..6101d1566238 100644
--- a/include/uapi/xen/privcmd.h
+++ b/include/uapi/xen/privcmd.h
@@ -113,8 +113,6 @@ struct privcmd_mmap_resource {
*/
#define IOCTL_PRIVCMD_HYPERCALL \
_IOC(_IOC_NONE, 'P', 0, sizeof(struct privcmd_hypercall))
-#define IOCTL_PRIVCMD_MMAP \
- _IOC(_IOC_NONE, 'P', 2, sizeof(struct privcmd_mmap))
#define IOCTL_PRIVCMD_MMAPBATCH \
_IOC(_IOC_NONE, 'P', 3, sizeof(struct privcmd_mmapbatch))
#define IOCTL_PRIVCMD_MMAPBATCH_V2 \
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index dae0f350c678..a07e422be09a 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -159,30 +159,6 @@ static inline int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
true);
}

-/* xen_remap_domain_gfn_range() - map a range of foreign frames
- * @vma: VMA to map the pages into
- * @addr: Address at which to map the pages
- * @gfn: First GFN to map.
- * @nr: Number frames to map
- * @prot: page protection mask
- * @domid: Domain owning the pages
- * @pages: Array of pages if this domain has an auto-translated physmap
- *
- * Returns the number of successfully mapped frames, or a -ve error
- * code.
- */
-static inline int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
- unsigned long addr,
- xen_pfn_t gfn, int nr,
- pgprot_t prot, unsigned int domid,
- struct page **pages)
-{
- if (xen_feature(XENFEAT_auto_translated_physmap))
- return -EOPNOTSUPP;
-
- return xen_remap_pfn(vma, addr, &gfn, nr, NULL, prot, domid, false);
-}
-
int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
int numpgs, struct page **pages);

--
2.35.3


2022-09-15 09:42:54

by Jan Beulich

[permalink] [raw]
Subject: Re: [PATCH] xen/privcmd: remove privcmd_ioctl_mmap()

On 15.09.2022 10:39, Juergen Gross wrote:
> The IOCTL_PRIVCMD_MMAP isn't in use by Xen since at least Xen 4.0.
>
> Remove it from the privcmd driver.
>
> Signed-off-by: Juergen Gross <[email protected]>

Can we reasonably remove an IOCTL, without being entirely certain that
no users exist outside of xen.git? Even if so, shouldn't there be a
staged deprecation process?

Jan

2022-09-15 11:11:58

by Jürgen Groß

[permalink] [raw]
Subject: Re: [PATCH] xen/privcmd: remove privcmd_ioctl_mmap()

On 15.09.22 11:32, Jan Beulich wrote:
> On 15.09.2022 10:39, Juergen Gross wrote:
>> The IOCTL_PRIVCMD_MMAP isn't in use by Xen since at least Xen 4.0.
>>
>> Remove it from the privcmd driver.
>>
>> Signed-off-by: Juergen Gross <[email protected]>
>
> Can we reasonably remove an IOCTL, without being entirely certain that
> no users exist outside of xen.git?

This is a valid question. I'm not sure how probable it is that such a user
is existing. Are there any Xen tool stacks not using the Xen libraries?

If so, why? Do we want to support those use cases?

> Even if so, shouldn't there be a
> staged deprecation process?

Depends on the answer to above questions.


Juergen


Attachments:
OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key
OpenPGP_signature (505.00 B)
OpenPGP digital signature
Download all attachments

2022-09-15 12:27:55

by Jan Beulich

[permalink] [raw]
Subject: Re: [PATCH] xen/privcmd: remove privcmd_ioctl_mmap()

On 15.09.2022 12:20, Juergen Gross wrote:
> On 15.09.22 11:32, Jan Beulich wrote:
>> On 15.09.2022 10:39, Juergen Gross wrote:
>>> The IOCTL_PRIVCMD_MMAP isn't in use by Xen since at least Xen 4.0.
>>>
>>> Remove it from the privcmd driver.
>>>
>>> Signed-off-by: Juergen Gross <[email protected]>
>>
>> Can we reasonably remove an IOCTL, without being entirely certain that
>> no users exist outside of xen.git?
>
> This is a valid question. I'm not sure how probable it is that such a user
> is existing. Are there any Xen tool stacks not using the Xen libraries?
>
> If so, why? Do we want to support those use cases?

I'm afraid I have no answers to these questions, and hence would generally
want to be conservative with removal of functionality.

Jan

>> Even if so, shouldn't there be a
>> staged deprecation process?
>
> Depends on the answer to above questions.
>
>
> Juergen

2022-09-16 00:55:59

by Stefano Stabellini

[permalink] [raw]
Subject: Re: [PATCH] xen/privcmd: remove privcmd_ioctl_mmap()

On Thu, 15 Sep 2022, Jan Beulich wrote:
> On 15.09.2022 12:20, Juergen Gross wrote:
> > On 15.09.22 11:32, Jan Beulich wrote:
> >> On 15.09.2022 10:39, Juergen Gross wrote:
> >>> The IOCTL_PRIVCMD_MMAP isn't in use by Xen since at least Xen 4.0.
> >>>
> >>> Remove it from the privcmd driver.
> >>>
> >>> Signed-off-by: Juergen Gross <[email protected]>
> >>
> >> Can we reasonably remove an IOCTL, without being entirely certain that
> >> no users exist outside of xen.git?
> >
> > This is a valid question. I'm not sure how probable it is that such a user
> > is existing. Are there any Xen tool stacks not using the Xen libraries?
> >
> > If so, why? Do we want to support those use cases?
>
> I'm afraid I have no answers to these questions, and hence would generally
> want to be conservative with removal of functionality.

I don't know either, but maybe we could at least mark IOCTL_PRIVCMD_MMAP
as deprecated in include/uapi/xen/privcmd.h ?