| Message ID | 20241206225036.273103-1-lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: enforce __must_check on VMA merge and split | expand |
Andrew - I'm guessing this will hit mm-unstable alongside [0], but to be clear this patch relies on that one, as that one fixes a case this check would have highlighted :) Thanks! [0]:https://lore.kernel.org/linux-mm/20241206215229.244413-1-lorenzo.stoakes@oracle.com/ On Fri, Dec 06, 2024 at 10:50:36PM +0000, Lorenzo Stoakes wrote: > It is of critical importance to check the return results on VMA merge (and > split), failure to do so can result in use-after-free's. This bug has > recurred, so have the compiler enforce this check to prevent any future > repetition. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/vma.c | 8 +++++--- > mm/vma.h | 26 +++++++++++++++----------- > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index a06747845cac..543c102b4062 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -447,8 +447,9 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > * has already been checked or doesn't make sense to fail. > * VMA Iterator will point to the original VMA. > */ > -static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > - unsigned long addr, int new_below) > +static __must_check int > +__split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > + unsigned long addr, int new_below) > { > struct vma_prepare vp; > struct vm_area_struct *new; > @@ -710,7 +711,8 @@ static bool can_merge_remove_vma(struct vm_area_struct *vma) > * - The caller must hold a WRITE lock on the mm_struct->mmap_lock. > * - vmi must be positioned within [@vmg->vma->vm_start, @vmg->vma->vm_end). > */ > -static struct vm_area_struct *vma_merge_existing_range(struct vma_merge_struct *vmg) > +static __must_check struct vm_area_struct *vma_merge_existing_range( > + struct vma_merge_struct *vmg) > { > struct vm_area_struct *vma = vmg->vma; > struct vm_area_struct *prev = vmg->prev; > diff --git a/mm/vma.h b/mm/vma.h > index 295d44ea54db..61ed044b6145 100644 > --- a/mm/vma.h > +++ b/mm/vma.h > @@ -139,9 +139,10 @@ void validate_mm(struct mm_struct *mm); > #define validate_mm(mm) do { } while (0) > #endif > > -int vma_expand(struct vma_merge_struct *vmg); > -int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, > - unsigned long start, unsigned long end, pgoff_t pgoff); > +__must_check int vma_expand(struct vma_merge_struct *vmg); > +__must_check int vma_shrink(struct vma_iterator *vmi, > + struct vm_area_struct *vma, > + unsigned long start, unsigned long end, pgoff_t pgoff); > > static inline int vma_iter_store_gfp(struct vma_iterator *vmi, > struct vm_area_struct *vma, gfp_t gfp) > @@ -174,13 +175,14 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > struct vm_area_struct *prev, struct vm_area_struct *next); > > /* We are about to modify the VMA's flags. */ > -struct vm_area_struct *vma_modify_flags(struct vma_iterator *vmi, > +__must_check struct vm_area_struct > +*vma_modify_flags(struct vma_iterator *vmi, > struct vm_area_struct *prev, struct vm_area_struct *vma, > unsigned long start, unsigned long end, > unsigned long new_flags); > > /* We are about to modify the VMA's flags and/or anon_name. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_flags_name(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -190,7 +192,7 @@ struct vm_area_struct > struct anon_vma_name *new_name); > > /* We are about to modify the VMA's memory policy. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_policy(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -198,7 +200,7 @@ struct vm_area_struct > struct mempolicy *new_pol); > > /* We are about to modify the VMA's flags and/or uffd context. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_flags_uffd(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -206,11 +208,13 @@ struct vm_area_struct > unsigned long new_flags, > struct vm_userfaultfd_ctx new_ctx); > > -struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg); > +__must_check struct vm_area_struct > +*vma_merge_new_range(struct vma_merge_struct *vmg); > > -struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi, > - struct vm_area_struct *vma, > - unsigned long delta); > +__must_check struct vm_area_struct > +*vma_merge_extend(struct vma_iterator *vmi, > + struct vm_area_struct *vma, > + unsigned long delta); > > void unlink_file_vma_batch_init(struct unlink_vma_file_batch *vb); > > -- > 2.47.1 >
On 12/6/24 23:50, Lorenzo Stoakes wrote: > It is of critical importance to check the return results on VMA merge (and > split), failure to do so can result in use-after-free's. This bug has > recurred, so have the compiler enforce this check to prevent any future > repetition. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/vma.c | 8 +++++--- > mm/vma.h | 26 +++++++++++++++----------- > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index a06747845cac..543c102b4062 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -447,8 +447,9 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > * has already been checked or doesn't make sense to fail. > * VMA Iterator will point to the original VMA. > */ > -static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > - unsigned long addr, int new_below) > +static __must_check int > +__split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > + unsigned long addr, int new_below) > { > struct vma_prepare vp; > struct vm_area_struct *new; > @@ -710,7 +711,8 @@ static bool can_merge_remove_vma(struct vm_area_struct *vma) > * - The caller must hold a WRITE lock on the mm_struct->mmap_lock. > * - vmi must be positioned within [@vmg->vma->vm_start, @vmg->vma->vm_end). > */ > -static struct vm_area_struct *vma_merge_existing_range(struct vma_merge_struct *vmg) > +static __must_check struct vm_area_struct *vma_merge_existing_range( > + struct vma_merge_struct *vmg) > { > struct vm_area_struct *vma = vmg->vma; > struct vm_area_struct *prev = vmg->prev; > diff --git a/mm/vma.h b/mm/vma.h > index 295d44ea54db..61ed044b6145 100644 > --- a/mm/vma.h > +++ b/mm/vma.h > @@ -139,9 +139,10 @@ void validate_mm(struct mm_struct *mm); > #define validate_mm(mm) do { } while (0) > #endif > > -int vma_expand(struct vma_merge_struct *vmg); > -int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, > - unsigned long start, unsigned long end, pgoff_t pgoff); > +__must_check int vma_expand(struct vma_merge_struct *vmg); > +__must_check int vma_shrink(struct vma_iterator *vmi, > + struct vm_area_struct *vma, > + unsigned long start, unsigned long end, pgoff_t pgoff); > > static inline int vma_iter_store_gfp(struct vma_iterator *vmi, > struct vm_area_struct *vma, gfp_t gfp) > @@ -174,13 +175,14 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > struct vm_area_struct *prev, struct vm_area_struct *next); > > /* We are about to modify the VMA's flags. */ > -struct vm_area_struct *vma_modify_flags(struct vma_iterator *vmi, > +__must_check struct vm_area_struct > +*vma_modify_flags(struct vma_iterator *vmi, > struct vm_area_struct *prev, struct vm_area_struct *vma, > unsigned long start, unsigned long end, > unsigned long new_flags); > > /* We are about to modify the VMA's flags and/or anon_name. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_flags_name(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -190,7 +192,7 @@ struct vm_area_struct > struct anon_vma_name *new_name); > > /* We are about to modify the VMA's memory policy. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_policy(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -198,7 +200,7 @@ struct vm_area_struct > struct mempolicy *new_pol); > > /* We are about to modify the VMA's flags and/or uffd context. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_flags_uffd(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -206,11 +208,13 @@ struct vm_area_struct > unsigned long new_flags, > struct vm_userfaultfd_ctx new_ctx); > > -struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg); > +__must_check struct vm_area_struct > +*vma_merge_new_range(struct vma_merge_struct *vmg); > > -struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi, > - struct vm_area_struct *vma, > - unsigned long delta); > +__must_check struct vm_area_struct > +*vma_merge_extend(struct vma_iterator *vmi, > + struct vm_area_struct *vma, > + unsigned long delta); > > void unlink_file_vma_batch_init(struct unlink_vma_file_batch *vb); >
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241206 17:50]: > It is of critical importance to check the return results on VMA merge (and > split), failure to do so can result in use-after-free's. This bug has > recurred, so have the compiler enforce this check to prevent any future > repetition. Reviewed-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/vma.c | 8 +++++--- > mm/vma.h | 26 +++++++++++++++----------- > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index a06747845cac..543c102b4062 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -447,8 +447,9 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > * has already been checked or doesn't make sense to fail. > * VMA Iterator will point to the original VMA. > */ > -static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > - unsigned long addr, int new_below) > +static __must_check int > +__split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, > + unsigned long addr, int new_below) > { > struct vma_prepare vp; > struct vm_area_struct *new; > @@ -710,7 +711,8 @@ static bool can_merge_remove_vma(struct vm_area_struct *vma) > * - The caller must hold a WRITE lock on the mm_struct->mmap_lock. > * - vmi must be positioned within [@vmg->vma->vm_start, @vmg->vma->vm_end). > */ > -static struct vm_area_struct *vma_merge_existing_range(struct vma_merge_struct *vmg) > +static __must_check struct vm_area_struct *vma_merge_existing_range( > + struct vma_merge_struct *vmg) > { > struct vm_area_struct *vma = vmg->vma; > struct vm_area_struct *prev = vmg->prev; > diff --git a/mm/vma.h b/mm/vma.h > index 295d44ea54db..61ed044b6145 100644 > --- a/mm/vma.h > +++ b/mm/vma.h > @@ -139,9 +139,10 @@ void validate_mm(struct mm_struct *mm); > #define validate_mm(mm) do { } while (0) > #endif > > -int vma_expand(struct vma_merge_struct *vmg); > -int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, > - unsigned long start, unsigned long end, pgoff_t pgoff); > +__must_check int vma_expand(struct vma_merge_struct *vmg); > +__must_check int vma_shrink(struct vma_iterator *vmi, > + struct vm_area_struct *vma, > + unsigned long start, unsigned long end, pgoff_t pgoff); > > static inline int vma_iter_store_gfp(struct vma_iterator *vmi, > struct vm_area_struct *vma, gfp_t gfp) > @@ -174,13 +175,14 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > struct vm_area_struct *prev, struct vm_area_struct *next); > > /* We are about to modify the VMA's flags. */ > -struct vm_area_struct *vma_modify_flags(struct vma_iterator *vmi, > +__must_check struct vm_area_struct > +*vma_modify_flags(struct vma_iterator *vmi, > struct vm_area_struct *prev, struct vm_area_struct *vma, > unsigned long start, unsigned long end, > unsigned long new_flags); > > /* We are about to modify the VMA's flags and/or anon_name. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_flags_name(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -190,7 +192,7 @@ struct vm_area_struct > struct anon_vma_name *new_name); > > /* We are about to modify the VMA's memory policy. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_policy(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -198,7 +200,7 @@ struct vm_area_struct > struct mempolicy *new_pol); > > /* We are about to modify the VMA's flags and/or uffd context. */ > -struct vm_area_struct > +__must_check struct vm_area_struct > *vma_modify_flags_uffd(struct vma_iterator *vmi, > struct vm_area_struct *prev, > struct vm_area_struct *vma, > @@ -206,11 +208,13 @@ struct vm_area_struct > unsigned long new_flags, > struct vm_userfaultfd_ctx new_ctx); > > -struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg); > +__must_check struct vm_area_struct > +*vma_merge_new_range(struct vma_merge_struct *vmg); > > -struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi, > - struct vm_area_struct *vma, > - unsigned long delta); > +__must_check struct vm_area_struct > +*vma_merge_extend(struct vma_iterator *vmi, > + struct vm_area_struct *vma, > + unsigned long delta); > > void unlink_file_vma_batch_init(struct unlink_vma_file_batch *vb); > > -- > 2.47.1 >
diff --git a/mm/vma.c b/mm/vma.c index a06747845cac..543c102b4062 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -447,8 +447,9 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, * has already been checked or doesn't make sense to fail. * VMA Iterator will point to the original VMA. */ -static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, - unsigned long addr, int new_below) +static __must_check int +__split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, + unsigned long addr, int new_below) { struct vma_prepare vp; struct vm_area_struct *new; @@ -710,7 +711,8 @@ static bool can_merge_remove_vma(struct vm_area_struct *vma) * - The caller must hold a WRITE lock on the mm_struct->mmap_lock. * - vmi must be positioned within [@vmg->vma->vm_start, @vmg->vma->vm_end). */ -static struct vm_area_struct *vma_merge_existing_range(struct vma_merge_struct *vmg) +static __must_check struct vm_area_struct *vma_merge_existing_range( + struct vma_merge_struct *vmg) { struct vm_area_struct *vma = vmg->vma; struct vm_area_struct *prev = vmg->prev; diff --git a/mm/vma.h b/mm/vma.h index 295d44ea54db..61ed044b6145 100644 --- a/mm/vma.h +++ b/mm/vma.h @@ -139,9 +139,10 @@ void validate_mm(struct mm_struct *mm); #define validate_mm(mm) do { } while (0) #endif -int vma_expand(struct vma_merge_struct *vmg); -int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, - unsigned long start, unsigned long end, pgoff_t pgoff); +__must_check int vma_expand(struct vma_merge_struct *vmg); +__must_check int vma_shrink(struct vma_iterator *vmi, + struct vm_area_struct *vma, + unsigned long start, unsigned long end, pgoff_t pgoff); static inline int vma_iter_store_gfp(struct vma_iterator *vmi, struct vm_area_struct *vma, gfp_t gfp) @@ -174,13 +175,14 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, struct vm_area_struct *prev, struct vm_area_struct *next); /* We are about to modify the VMA's flags. */ -struct vm_area_struct *vma_modify_flags(struct vma_iterator *vmi, +__must_check struct vm_area_struct +*vma_modify_flags(struct vma_iterator *vmi, struct vm_area_struct *prev, struct vm_area_struct *vma, unsigned long start, unsigned long end, unsigned long new_flags); /* We are about to modify the VMA's flags and/or anon_name. */ -struct vm_area_struct +__must_check struct vm_area_struct *vma_modify_flags_name(struct vma_iterator *vmi, struct vm_area_struct *prev, struct vm_area_struct *vma, @@ -190,7 +192,7 @@ struct vm_area_struct struct anon_vma_name *new_name); /* We are about to modify the VMA's memory policy. */ -struct vm_area_struct +__must_check struct vm_area_struct *vma_modify_policy(struct vma_iterator *vmi, struct vm_area_struct *prev, struct vm_area_struct *vma, @@ -198,7 +200,7 @@ struct vm_area_struct struct mempolicy *new_pol); /* We are about to modify the VMA's flags and/or uffd context. */ -struct vm_area_struct +__must_check struct vm_area_struct *vma_modify_flags_uffd(struct vma_iterator *vmi, struct vm_area_struct *prev, struct vm_area_struct *vma, @@ -206,11 +208,13 @@ struct vm_area_struct unsigned long new_flags, struct vm_userfaultfd_ctx new_ctx); -struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg); +__must_check struct vm_area_struct +*vma_merge_new_range(struct vma_merge_struct *vmg); -struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi, - struct vm_area_struct *vma, - unsigned long delta); +__must_check struct vm_area_struct +*vma_merge_extend(struct vma_iterator *vmi, + struct vm_area_struct *vma, + unsigned long delta); void unlink_file_vma_batch_init(struct unlink_vma_file_batch *vb);
It is of critical importance to check the return results on VMA merge (and split), failure to do so can result in use-after-free's. This bug has recurred, so have the compiler enforce this check to prevent any future repetition. Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- mm/vma.c | 8 +++++--- mm/vma.h | 26 +++++++++++++++----------- 2 files changed, 20 insertions(+), 14 deletions(-)