We can use the mmap insert pages functionality provided for use in
mmap_prepare to insert the kcov pages as required.
This does necessitate an allocation, but since it's in the mmap path this
doesn't seem egregious. The allocation/freeing of the pages array is
handled automatically by vma_desc_set_mixedmap_pages() and the mapping
logic.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
kernel/kcov.c | 42 ++++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 1d85597057e1..2bcf403e5f6f 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -484,31 +484,41 @@ void kcov_task_exit(struct task_struct *t)
kcov_put(kcov);
}
-static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
+static int kcov_mmap_error(int err)
+{
+ pr_warn_once("kcov: vm_insert_page() failed\n");
+ return err;
+}
+
+static int kcov_mmap_prepare(struct vm_area_desc *desc)
{
int res = 0;
- struct kcov *kcov = vma->vm_file->private_data;
- unsigned long size, off;
- struct page *page;
+ struct kcov *kcov = desc->file->private_data;
+ unsigned long size, nr_pages, i;
+ struct page **pages;
unsigned long flags;
spin_lock_irqsave(&kcov->lock, flags);
size = kcov->size * sizeof(unsigned long);
- if (kcov->area == NULL || vma->vm_pgoff != 0 ||
- vma->vm_end - vma->vm_start != size) {
+ if (kcov->area == NULL || desc->pgoff != 0 ||
+ vma_desc_size(desc) != size) {
res = -EINVAL;
goto exit;
}
spin_unlock_irqrestore(&kcov->lock, flags);
- vm_flags_set(vma, VM_DONTEXPAND);
- for (off = 0; off < size; off += PAGE_SIZE) {
- page = vmalloc_to_page(kcov->area + off);
- res = vm_insert_page(vma, vma->vm_start + off, page);
- if (res) {
- pr_warn_once("kcov: vm_insert_page() failed\n");
- return res;
- }
- }
+
+ desc->vm_flags |= VM_DONTEXPAND;
+ nr_pages = size >> PAGE_SHIFT;
+
+ pages = mmap_action_mixedmap_pages(&desc->action, desc->start,
+ nr_pages);
+ if (!pages)
+ return -ENOMEM;
+
+ for (i = 0; i < nr_pages; i++)
+ pages[i] = vmalloc_to_page(kcov->area + i * PAGE_SIZE);
+ desc->action.error_hook = kcov_mmap_error;
+
return 0;
exit:
spin_unlock_irqrestore(&kcov->lock, flags);
@@ -761,7 +771,7 @@ static const struct file_operations kcov_fops = {
.open = kcov_open,
.unlocked_ioctl = kcov_ioctl,
.compat_ioctl = kcov_ioctl,
- .mmap = kcov_mmap,
+ .mmap_prepare = kcov_mmap_prepare,
.release = kcov_close,
};
--
2.51.0On Wed, 10 Sep 2025 21:22:11 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> We can use the mmap insert pages functionality provided for use in
> mmap_prepare to insert the kcov pages as required.
>
> This does necessitate an allocation, but since it's in the mmap path this
> doesn't seem egregious. The allocation/freeing of the pages array is
> handled automatically by vma_desc_set_mixedmap_pages() and the mapping
> logic.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> kernel/kcov.c | 42 ++++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 1d85597057e1..2bcf403e5f6f 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -484,31 +484,41 @@ void kcov_task_exit(struct task_struct *t)
> kcov_put(kcov);
> }
>
> -static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> +static int kcov_mmap_error(int err)
> +{
> + pr_warn_once("kcov: vm_insert_page() failed\n");
> + return err;
> +}
> +
> +static int kcov_mmap_prepare(struct vm_area_desc *desc)
> {
> int res = 0;
> - struct kcov *kcov = vma->vm_file->private_data;
> - unsigned long size, off;
> - struct page *page;
> + struct kcov *kcov = desc->file->private_data;
> + unsigned long size, nr_pages, i;
> + struct page **pages;
> unsigned long flags;
>
> spin_lock_irqsave(&kcov->lock, flags);
> size = kcov->size * sizeof(unsigned long);
> - if (kcov->area == NULL || vma->vm_pgoff != 0 ||
> - vma->vm_end - vma->vm_start != size) {
> + if (kcov->area == NULL || desc->pgoff != 0 ||
> + vma_desc_size(desc) != size) {
> res = -EINVAL;
> goto exit;
> }
> spin_unlock_irqrestore(&kcov->lock, flags);
> - vm_flags_set(vma, VM_DONTEXPAND);
> - for (off = 0; off < size; off += PAGE_SIZE) {
> - page = vmalloc_to_page(kcov->area + off);
> - res = vm_insert_page(vma, vma->vm_start + off, page);
> - if (res) {
> - pr_warn_once("kcov: vm_insert_page() failed\n");
> - return res;
> - }
> - }
> +
> + desc->vm_flags |= VM_DONTEXPAND;
> + nr_pages = size >> PAGE_SHIFT;
> +
> + pages = mmap_action_mixedmap_pages(&desc->action, desc->start,
> + nr_pages);
Hi Lorenzo,
Not sure if it belongs here before the EINVAL tests, but it looks like
kcov->size doesn't have any page alignment. I think size could be
4000 bytes other unaligned values, so nr_pages should round up.
-chris
On Thu, Sep 18, 2025 at 12:45:38PM -0700, Chris Mason wrote:
> On Wed, 10 Sep 2025 21:22:11 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> > We can use the mmap insert pages functionality provided for use in
> > mmap_prepare to insert the kcov pages as required.
> >
> > This does necessitate an allocation, but since it's in the mmap path this
> > doesn't seem egregious. The allocation/freeing of the pages array is
> > handled automatically by vma_desc_set_mixedmap_pages() and the mapping
> > logic.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > kernel/kcov.c | 42 ++++++++++++++++++++++++++----------------
> > 1 file changed, 26 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index 1d85597057e1..2bcf403e5f6f 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -484,31 +484,41 @@ void kcov_task_exit(struct task_struct *t)
> > kcov_put(kcov);
> > }
> >
> > -static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> > +static int kcov_mmap_error(int err)
> > +{
> > + pr_warn_once("kcov: vm_insert_page() failed\n");
> > + return err;
> > +}
> > +
> > +static int kcov_mmap_prepare(struct vm_area_desc *desc)
> > {
> > int res = 0;
> > - struct kcov *kcov = vma->vm_file->private_data;
> > - unsigned long size, off;
> > - struct page *page;
> > + struct kcov *kcov = desc->file->private_data;
> > + unsigned long size, nr_pages, i;
> > + struct page **pages;
> > unsigned long flags;
> >
> > spin_lock_irqsave(&kcov->lock, flags);
> > size = kcov->size * sizeof(unsigned long);
> > - if (kcov->area == NULL || vma->vm_pgoff != 0 ||
> > - vma->vm_end - vma->vm_start != size) {
> > + if (kcov->area == NULL || desc->pgoff != 0 ||
> > + vma_desc_size(desc) != size) {
> > res = -EINVAL;
> > goto exit;
> > }
> > spin_unlock_irqrestore(&kcov->lock, flags);
> > - vm_flags_set(vma, VM_DONTEXPAND);
> > - for (off = 0; off < size; off += PAGE_SIZE) {
> > - page = vmalloc_to_page(kcov->area + off);
> > - res = vm_insert_page(vma, vma->vm_start + off, page);
> > - if (res) {
> > - pr_warn_once("kcov: vm_insert_page() failed\n");
> > - return res;
> > - }
> > - }
> > +
> > + desc->vm_flags |= VM_DONTEXPAND;
> > + nr_pages = size >> PAGE_SHIFT;
> > +
> > + pages = mmap_action_mixedmap_pages(&desc->action, desc->start,
> > + nr_pages);
>
> Hi Lorenzo,
>
> Not sure if it belongs here before the EINVAL tests, but it looks like
> kcov->size doesn't have any page alignment. I think size could be
> 4000 bytes other unaligned values, so nr_pages should round up.
Thanks, you may well be right, but but this series has been respun and I no
longer touch kcov. :)
Am at v4 now -
https://lore.kernel.org/linux-mm/cover.1758135681.git.lorenzo.stoakes@oracle.com/
- apologies for the quick turnaround but going to kernel recipes soon and then
on vacation so wanted to get this wrapped up!
>
> -chris
Cheers, Lorenzo
On Wed, Sep 10, 2025 at 09:22:11PM +0100, Lorenzo Stoakes wrote:
> +static int kcov_mmap_prepare(struct vm_area_desc *desc)
> {
> int res = 0;
> - struct kcov *kcov = vma->vm_file->private_data;
> - unsigned long size, off;
> - struct page *page;
> + struct kcov *kcov = desc->file->private_data;
> + unsigned long size, nr_pages, i;
> + struct page **pages;
> unsigned long flags;
>
> spin_lock_irqsave(&kcov->lock, flags);
> size = kcov->size * sizeof(unsigned long);
> - if (kcov->area == NULL || vma->vm_pgoff != 0 ||
> - vma->vm_end - vma->vm_start != size) {
> + if (kcov->area == NULL || desc->pgoff != 0 ||
> + vma_desc_size(desc) != size) {
IMHO these range checks should be cleaned up into a helper:
/* Returns true if the VMA falls within starting_pgoff to
starting_pgoff + ROUND_DOWN(length_bytes, PAGE_SIZE))
Is careful to avoid any arithmetic overflow.
*/
vma_desc_check_range(desc, starting_pgoff=0, length_bytes=size);
> + desc->vm_flags |= VM_DONTEXPAND;
> + nr_pages = size >> PAGE_SHIFT;
> +
> + pages = mmap_action_mixedmap_pages(&desc->action, desc->start,
> + nr_pages);
> + if (!pages)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_pages; i++)
> + pages[i] = vmalloc_to_page(kcov->area + i * PAGE_SIZE);
This is not a mixed map.
All the memory comes from vmalloc_user() which makes them normal
struct pages with refcounts.
If anything the action should be called mmap_action_vmalloc_user() to
match how the memory was allocated instead of open coding something.
Jason
On Mon, Sep 15, 2025 at 09:16:17AM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 10, 2025 at 09:22:11PM +0100, Lorenzo Stoakes wrote:
> > +static int kcov_mmap_prepare(struct vm_area_desc *desc)
> > {
> > int res = 0;
> > - struct kcov *kcov = vma->vm_file->private_data;
> > - unsigned long size, off;
> > - struct page *page;
> > + struct kcov *kcov = desc->file->private_data;
> > + unsigned long size, nr_pages, i;
> > + struct page **pages;
> > unsigned long flags;
> >
> > spin_lock_irqsave(&kcov->lock, flags);
> > size = kcov->size * sizeof(unsigned long);
> > - if (kcov->area == NULL || vma->vm_pgoff != 0 ||
> > - vma->vm_end - vma->vm_start != size) {
> > + if (kcov->area == NULL || desc->pgoff != 0 ||
> > + vma_desc_size(desc) != size) {
>
> IMHO these range checks should be cleaned up into a helper:
>
> /* Returns true if the VMA falls within starting_pgoff to
> starting_pgoff + ROUND_DOWN(length_bytes, PAGE_SIZE))
> Is careful to avoid any arithmetic overflow.
> */
Right, but I can't refactor every driver I touch, it's not really tractable. I'd
like to get this change done before I retire :)
> vma_desc_check_range(desc, starting_pgoff=0, length_bytes=size);
>
> > + desc->vm_flags |= VM_DONTEXPAND;
> > + nr_pages = size >> PAGE_SHIFT;
> > +
> > + pages = mmap_action_mixedmap_pages(&desc->action, desc->start,
> > + nr_pages);
> > + if (!pages)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < nr_pages; i++)
> > + pages[i] = vmalloc_to_page(kcov->area + i * PAGE_SIZE);
>
> This is not a mixed map.
>
> All the memory comes from vmalloc_user() which makes them normal
> struct pages with refcounts.
>
> If anything the action should be called mmap_action_vmalloc_user() to
> match how the memory was allocated instead of open coding something.
Again we're getting into the same issue - my workload doesn't really permit
me to refactor every user of .mmap beyond converting sensibly to the new
scheme.
I think this kind of change is out of scope for the series.
I'd rather make this as apples-to-apples as possible for now so it can be
done vaguely mechanically.
Of course we can follow up with improvements later.
>
> Jason
Cheers, Lorenzo
On Mon, Sep 15, 2025 at 01:43:50PM +0100, Lorenzo Stoakes wrote:
> > > + if (kcov->area == NULL || desc->pgoff != 0 ||
> > > + vma_desc_size(desc) != size) {
> >
> > IMHO these range checks should be cleaned up into a helper:
> >
> > /* Returns true if the VMA falls within starting_pgoff to
> > starting_pgoff + ROUND_DOWN(length_bytes, PAGE_SIZE))
> > Is careful to avoid any arithmetic overflow.
> > */
>
> Right, but I can't refactor every driver I touch, it's not really tractable. I'd
> like to get this change done before I retire :)
I don't think it is a big deal, and these helpers should be part of
the new api. You are reading and touching anyhow.
> > If anything the action should be called mmap_action_vmalloc_user() to
> > match how the memory was allocated instead of open coding something.
>
> Again we're getting into the same issue - my workload doesn't really permit
> me to refactor every user of .mmap beyond converting sensibly to the new
> scheme.
If you are adding this explicit action concept then it should be a
sane set of actions. Using a mixed map action to insert a vmalloc_user
is not a reasonable thing to do.
Jason
On Mon, Sep 15, 2025 at 09:48:01AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 15, 2025 at 01:43:50PM +0100, Lorenzo Stoakes wrote:
> > > > + if (kcov->area == NULL || desc->pgoff != 0 ||
> > > > + vma_desc_size(desc) != size) {
> > >
> > > IMHO these range checks should be cleaned up into a helper:
> > >
> > > /* Returns true if the VMA falls within starting_pgoff to
> > > starting_pgoff + ROUND_DOWN(length_bytes, PAGE_SIZE))
> > > Is careful to avoid any arithmetic overflow.
> > > */
> >
> > Right, but I can't refactor every driver I touch, it's not really tractable. I'd
> > like to get this change done before I retire :)
>
> I don't think it is a big deal, and these helpers should be part of
> the new api. You are reading and touching anyhow.
x ~230 becomes a big deal.
>
> > > If anything the action should be called mmap_action_vmalloc_user() to
> > > match how the memory was allocated instead of open coding something.
> >
> > Again we're getting into the same issue - my workload doesn't really permit
> > me to refactor every user of .mmap beyond converting sensibly to the new
> > scheme.
>
> If you are adding this explicit action concept then it should be a
> sane set of actions. Using a mixed map action to insert a vmalloc_user
> is not a reasonable thing to do.
Right I'm obviously intending there to be a sane interface.
And there are users who use mixed map to insert actual mixed map pages, so
having an interface for _that_ isn't crazy. So it's not like this is
compromising that.
(I mean an aside is we need to clean up a lot there anyway, it's a mess, but
that's out of scope here.)
>
> Jason
Anwyay, for the sake of getting this series in since you seem adament, I'll
go ahead and refactor in this case. But it's really not reasonable to
expect me to do this in each instance.
I will obviously try my best to ensure the API is as good as it can be, and
adapted to what mmap users need. That bit I am trying to get as right as I
can...
But in each individual driver's case, we have to be pragmatic.
Cheers, Lorenzo
© 2016 - 2025 Red Hat, Inc.