Re: Misuse of mapping_set_folio_order_range()

On Mon 27-07-26 21:25:01, Matthew Wilcox wrote:
> While reviewing an unrelated change, Sashiko seems to have found a bug
> in ext4:
> 
> > --- Patch [9]: [PATCH v6 09/12] filemap: Remove checks in mapping_set_folio_order_range() ---
> > commit c0859bbb9afcf450bfdee34a144cba981c50e605
> > Author: Matthew Wilcox (Oracle) <[email protected]>
> > 
> > filemap: Remove checks in mapping_set_folio_order_range()
> > 
> > This commit removes the maximum order checks in mapping_set_folio_order_range()
> > to allow its use in hugetlb.
> > 
> > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > > index 2c3718d592d6d..968b791cfd148 100644
> > > --- a/include/linux/pagemap.h
> > > +++ b/include/linux/pagemap.h
> > > @@ -405,29 +405,23 @@ static inline size_t mapping_max_folio_size_supported(void)
> > 
> > [ ... ]
> > 
> > >  static inline void mapping_set_folio_order_range(struct address_space *mapping,
> > > -						 unsigned int min,
> > > -						 unsigned int max)
> > > +		unsigned int min, unsigned int max)
> > >  {
> > > -	if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
> > > +	if (!IS_ENABLED(CONFIG_LARGE_FOLIO))
> > >  		return;
> > > -
> > > -	if (min > MAX_PAGECACHE_ORDER)
> > > -		min = MAX_PAGECACHE_ORDER;
> > > -
> > > -	if (max > MAX_PAGECACHE_ORDER)
> > > -		max = MAX_PAGECACHE_ORDER;
> > > -
> > >  	if (max < min)
> > >  		max = min;
> > >  
> > >  	mapping->flags = (mapping->flags & ~AS_FOLIO_ORDER_MASK) |
> > >  		(min << AS_FOLIO_ORDER_MIN) | (max << AS_FOLIO_ORDER_MAX);
> > 
> > This is a pre-existing issue, but can this non-atomic read-modify-write of
> > mapping->flags cause lost updates to concurrently set atomic bit flags?
> > 
> > While the documentation above this function states it should not be called
> > while the inode is active, ext4_change_inode_journal_flag() dynamically
> > invokes it on an active inode during the EXT4_IOC_SETFLAGS ioctl:
> > 
> > fs/ext4/inode.c:ext4_change_inode_journal_flag() {
> >     ...
> >     ext4_set_inode_mapping_order(inode);
> >     ...
> > }
> > 
> > If a privileged user toggles the ext4 journal flag, could this non-atomic
> > update race with mm_take_all_locks() (which can be triggered by userfaultfd
> > or KVM) modifying AS_MM_ALL_LOCKS via atomic bitops?
> > 
> > Since they do not share any locks (ext4_change_inode_journal_flag() holds
> > inode_lock and invalidate_lock, while mm_take_all_locks() holds mmap_lock
> > and i_mmap_rwsem), losing the AS_MM_ALL_LOCKS bit could cause
> > vm_unlock_mapping() to skip unlocking mapping->i_mmap_rwsem, resulting in a
> > permanent deadlock.
> 
> Making mapping_set_folio_order_range() atomic would be a pain -- it'd
> involve a cmpxchg() loop which would be utterly useless for most
> callers.  Can you figure out how to fix this in ext4?

What we need to do in ext4_change_inode_journal_flag() is that we want to
disable / enable large folios when enabling / disabling data journalling
for an inode (because writeback of large folios would strain the journal
too much when journalling data). We are actually pretty careful in
ext4_change_inode_journal_flag() - we lock the inode_lock, invalidate_lock,
writeout everything, prune all the page cache, make sure to synchronize
with writeback path and then we switch adress_space_operations and allowed
folio orders.

As far as I can tell AS_MM_ALL_LOCKS flags bit manipulation is the only
thing we can realistically race with. But I don't see how we could
avoid that race from ext4 :-|. Maybe we could have
mapping_set_folio_order_range_atomic() helper to avoid the pointless
overhead for everybody?

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR