| Message ID | 20221017110630.c6c786d46e74.I7b85b7dd326d2d078dabdd4ae40c35dc4ee1f3bc@changeid |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | um: protect VMA iteration | expand |
On 17/10/2022 10:06, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Looks like this is needed now, otherwise we get RCU > splats from lockdep. But I don't know anything about > this code ... I went through it several times a couple of years ago looking if there is a way to optimize some of the flushes. A lock there will not hurt. I do not think that it would do anything as that bit should be single-threaded (and most invocations are under locks too), but it will not hurt. So if it is needed to make lockdep shut up - let it be. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > arch/um/kernel/tlb.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/um/kernel/tlb.c b/arch/um/kernel/tlb.c > index ad449173a1a1..3cc6b753e579 100644 > --- a/arch/um/kernel/tlb.c > +++ b/arch/um/kernel/tlb.c > @@ -587,8 +587,10 @@ void flush_tlb_mm(struct mm_struct *mm) > struct vm_area_struct *vma; > VMA_ITERATOR(vmi, mm, 0); > > + rcu_read_lock(); > for_each_vma(vmi, vma) > fix_range(mm, vma->vm_start, vma->vm_end, 0); > + rcu_read_unlock(); > } > > void force_flush_all(void) > @@ -597,6 +599,8 @@ void force_flush_all(void) > struct vm_area_struct *vma; > VMA_ITERATOR(vmi, mm, 0); > > + rcu_read_lock(); > for_each_vma(vmi, vma) > fix_range(mm, vma->vm_start, vma->vm_end, 1); > + rcu_read_unlock(); > } Acked-By: Anton Ivanov <anton.ivanov@cambridgegreys.com>
On Mon, Oct 17, 2022 at 11:06:30AM +0200, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Looks like this is needed now, otherwise we get RCU > splats from lockdep. But I don't know anything about > this code ... You're getting lockdep splats now because there was no checking before. I assumed that the locking was correct before when making this change, and lockdep shows it wasn't ;-) So, you were traversing the list of VMAs with no protection against modification (ie a simulataneous call to mmap()/munmap() would lead to a UAF). I imagine you really want a call to mmap_read_lock() / mmap_read_unlock(), and you'll want to backport it as far as you care about. Maybe this RCU locking is good enough; it'll let you walk the VMAs without any locking, but you have to know that (a) fix_range() doesn't sleep and (b) that if new VMAs are added during the walk or old ones are removed, it doesn't matter whether fix_range() is called on them or not. From the tone of the email, I infer that you probably haven't done that depth of analysis ;-) Taking the mmap_lock for read is the safe route. > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > arch/um/kernel/tlb.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/um/kernel/tlb.c b/arch/um/kernel/tlb.c > index ad449173a1a1..3cc6b753e579 100644 > --- a/arch/um/kernel/tlb.c > +++ b/arch/um/kernel/tlb.c > @@ -587,8 +587,10 @@ void flush_tlb_mm(struct mm_struct *mm) > struct vm_area_struct *vma; > VMA_ITERATOR(vmi, mm, 0); > > + rcu_read_lock(); > for_each_vma(vmi, vma) > fix_range(mm, vma->vm_start, vma->vm_end, 0); > + rcu_read_unlock(); > } > > void force_flush_all(void) > @@ -597,6 +599,8 @@ void force_flush_all(void) > struct vm_area_struct *vma; > VMA_ITERATOR(vmi, mm, 0); > > + rcu_read_lock(); > for_each_vma(vmi, vma) > fix_range(mm, vma->vm_start, vma->vm_end, 1); > + rcu_read_unlock(); > } > -- > 2.37.3 >
On Mon, 2022-10-17 at 15:50 +0100, Matthew Wilcox wrote: > On Mon, Oct 17, 2022 at 11:06:30AM +0200, Johannes Berg wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > Looks like this is needed now, otherwise we get RCU > > splats from lockdep. But I don't know anything about > > this code ... > > You're getting lockdep splats now because there was no checking before. Yeah that's what I was assuming, and seeing in your change anyway since it just was doing a manual iteration. > I assumed that the locking was correct before when making this change, > and lockdep shows it wasn't ;-) > :-) > So, you were traversing the list of > VMAs with no protection against modification (ie a simulataneous call to > mmap()/munmap() would lead to a UAF). I imagine you really want a call > to mmap_read_lock() / mmap_read_unlock(), and you'll want to backport > it as far as you care about. Well, to be fair, ARCH=um is !SMP && !PREEMPT, so I don't think anything can actually happen here? At least I don't really see how anything would. Even userspace can't run while here, I believe. > Maybe this RCU locking is good enough; it'll let you walk the VMAs > without any locking, but you have to know that (a) fix_range() doesn't > sleep and (b) that if new VMAs are added during the walk or old ones are > removed, it doesn't matter whether fix_range() is called on them or not. > > From the tone of the email, I infer that you probably haven't done that > depth of analysis ;-) Taking the mmap_lock for read is the safe route. > Indeed, thanks :) johannes
diff --git a/arch/um/kernel/tlb.c b/arch/um/kernel/tlb.c index ad449173a1a1..3cc6b753e579 100644 --- a/arch/um/kernel/tlb.c +++ b/arch/um/kernel/tlb.c @@ -587,8 +587,10 @@ void flush_tlb_mm(struct mm_struct *mm) struct vm_area_struct *vma; VMA_ITERATOR(vmi, mm, 0); + rcu_read_lock(); for_each_vma(vmi, vma) fix_range(mm, vma->vm_start, vma->vm_end, 0); + rcu_read_unlock(); } void force_flush_all(void) @@ -597,6 +599,8 @@ void force_flush_all(void) struct vm_area_struct *vma; VMA_ITERATOR(vmi, mm, 0); + rcu_read_lock(); for_each_vma(vmi, vma) fix_range(mm, vma->vm_start, vma->vm_end, 1); + rcu_read_unlock(); }