2021-05-10 17:03:07

by Liam R. Howlett

[permalink] [raw]
Subject: [PATCH 01/22] mm: Add vma_lookup()

Many places in the kernel use find_vma() to get a vma and then check the
start address of the vma to ensure the next vma was not returned.

Other places use the find_vma_intersection() call with add, addr + 1 as
the range; looking for just the vma at a specific address.

The third use of find_vma() is by developers who do not know that the
function starts searching at the provided address upwards for the next
vma. This results in a bug that is often overlooked for a long time.

Adding the new vma_lookup() function will allow for cleaner code by
removing the find_vma() calls which check limits, making
find_vma_intersection() calls of a single address to be shorter, and
potentially reduce the incorrect uses of find_vma().

Signed-off-by: Liam R. Howlett <[email protected]>
---
include/linux/mm.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 25b9041f9925..5f2a15e702ff 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2689,6 +2689,24 @@ static inline struct vm_area_struct * find_vma_intersection(struct mm_struct * m
return vma;
}

+/**
+ * vma_lookup() - Find a VMA at a specific address
+ * @mm: The process address space.
+ * @addr: The user address.
+ *
+ * Return: The vm_area_struct at the given address, %NULL otherwise.
+ */
+static inline
+struct vm_area_struct *vma_lookup(struct mm_struct *mm, unsigned long addr)
+{
+ struct vm_area_struct * vma = find_vma(mm, addr);
+
+ if (vma && addr < vma->vm_start)
+ vma = NULL;
+
+ return vma;
+}
+
static inline unsigned long vm_start_gap(struct vm_area_struct *vma)
{
unsigned long vm_start = vma->vm_start;
--
2.30.2


2021-05-14 10:42:32

by David Hildenbrand

[permalink] [raw]
Subject: Re: [PATCH 01/22] mm: Add vma_lookup()

On 10.05.21 18:58, Liam Howlett wrote:
> he third use of find_vma() is by developers who do not know that the
> function starts searching at the provided address upwards for the next
> vma. This results in a bug that is often overlooked for a long time.
>
> Adding the new vma_lookup() function will allow for cleaner code by

Sounds helpful to me

Acked-by: David Hildenbrand <[email protected]>

--
Thanks,

David / dhildenb


2021-05-21 20:06:43

by Davidlohr Bueso

[permalink] [raw]
Subject: Re: [PATCH 01/22] mm: Add vma_lookup()

On Mon, 10 May 2021, Liam Howlett wrote:

>Many places in the kernel use find_vma() to get a vma and then check the
>start address of the vma to ensure the next vma was not returned.
>
>Other places use the find_vma_intersection() call with add, addr + 1 as
>the range; looking for just the vma at a specific address.
>
>The third use of find_vma() is by developers who do not know that the
>function starts searching at the provided address upwards for the next
>vma. This results in a bug that is often overlooked for a long time.
>
>Adding the new vma_lookup() function will allow for cleaner code by
>removing the find_vma() calls which check limits, making
>find_vma_intersection() calls of a single address to be shorter, and
>potentially reduce the incorrect uses of find_vma().
>
>Signed-off-by: Liam R. Howlett <[email protected]>
>---
> include/linux/mm.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>diff --git a/include/linux/mm.h b/include/linux/mm.h
>index 25b9041f9925..5f2a15e702ff 100644
>--- a/include/linux/mm.h
>+++ b/include/linux/mm.h
>@@ -2689,6 +2689,24 @@ static inline struct vm_area_struct * find_vma_intersection(struct mm_struct * m
> return vma;
> }

While at it can we clean up find_vma_intersection? I'm not particularly
user/fan of checkpatch.pl, but this one is kind of ridiculous.

Thanks,
Davidlohr

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c274f75efcf9..16eddedf783f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2678,9 +2678,14 @@ extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long add
extern struct vm_area_struct * find_vma_prev(struct mm_struct * mm, unsigned long addr,
struct vm_area_struct **pprev);

-/* Look up the first VMA which intersects the interval start_addr..end_addr-1,
- NULL if none. Assume start_addr < end_addr. */
-static inline struct vm_area_struct * find_vma_intersection(struct mm_struct * mm, unsigned long start_addr, unsigned long end_addr)
+/*
+ * Look up the first VMA which intersects the interval start_addr..end_addr-1,
+ * NULL if none. Assume start_addr < end_addr.
+ */
+static inline
+struct vm_area_struct *find_vma_intersection(struct mm_struct * mm,
+ unsigned long start_addr,
+ unsigned long end_addr)
{
struct vm_area_struct * vma = find_vma(mm,start_addr);

2021-05-21 20:18:49

by Liam R. Howlett

[permalink] [raw]
Subject: Re: [PATCH 01/22] mm: Add vma_lookup()

* Davidlohr Bueso <[email protected]> [210520 23:32]:
> On Mon, 10 May 2021, Liam Howlett wrote:
>
> > Many places in the kernel use find_vma() to get a vma and then check the
> > start address of the vma to ensure the next vma was not returned.
> >
> > Other places use the find_vma_intersection() call with add, addr + 1 as
> > the range; looking for just the vma at a specific address.
> >
> > The third use of find_vma() is by developers who do not know that the
> > function starts searching at the provided address upwards for the next
> > vma. This results in a bug that is often overlooked for a long time.
> >
> > Adding the new vma_lookup() function will allow for cleaner code by
> > removing the find_vma() calls which check limits, making
> > find_vma_intersection() calls of a single address to be shorter, and
> > potentially reduce the incorrect uses of find_vma().
> >
> > Signed-off-by: Liam R. Howlett <[email protected]>
> > ---
> > include/linux/mm.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 25b9041f9925..5f2a15e702ff 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2689,6 +2689,24 @@ static inline struct vm_area_struct * find_vma_intersection(struct mm_struct * m
> > return vma;
> > }
>
> While at it can we clean up find_vma_intersection? I'm not particularly
> user/fan of checkpatch.pl, but this one is kind of ridiculous.

Agreed. This addition is worth re-spin. I will change the comment into
a kernel documentation style comment at the same time.

>
> Thanks,
> Davidlohr
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c274f75efcf9..16eddedf783f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2678,9 +2678,14 @@ extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long add
> extern struct vm_area_struct * find_vma_prev(struct mm_struct * mm, unsigned long addr,
> struct vm_area_struct **pprev);
>
> -/* Look up the first VMA which intersects the interval start_addr..end_addr-1,
> - NULL if none. Assume start_addr < end_addr. */
> -static inline struct vm_area_struct * find_vma_intersection(struct mm_struct * mm, unsigned long start_addr, unsigned long end_addr)
> +/*
> + * Look up the first VMA which intersects the interval start_addr..end_addr-1,
> + * NULL if none. Assume start_addr < end_addr.
> + */
> +static inline
> +struct vm_area_struct *find_vma_intersection(struct mm_struct * mm,
> + unsigned long start_addr,
> + unsigned long end_addr)
> {
> struct vm_area_struct * vma = find_vma(mm,start_addr);