Re: [RFC PATCH] fs/splice: allow for a way to block splice() with read-only files

[Pedro Falcato <pfalcato@suse.de>]

On Sun, May 17, 2026 at 12:07:05AM +0100, Matthew Wilcox wrote:
> On Sat, May 16, 2026 at 07:21:26PM +0100, Pedro Falcato wrote:
> > +static bool may_write_to_page(struct page *page, struct address_space **plast)
> > +{
> > +	struct folio *folio = page_folio(page);
> > +	struct address_space *mapping, *last = *plast;
> > +	struct inode *inode;
> > +	bool may = false;
> > +
> > +	if (!READ_ONCE(sysctl_splice_needs_write))
> > +		return true;
> > +	/*
> > +	 * Always fine to write to anon folios.
> > +	 */
> > +	if (folio_test_anon(folio))
> > +		return true;
> 
> What about KSM?  It's not something we've seen attacked yet, but it'd be
> pretty nasty to be able to change a KSM page in another process.

It's my understanding that only anon pages can be KSM'd, and KSM still keeps
the FOLIO_MAPPING_ANON bit set. So folio_test_anon() should still test true
for those.

> 
> I just got off a flight, so hopefully I'm semicoherent.
> 
> > +	mapping = READ_ONCE(folio->mapping);
> > +	WARN_ON((unsigned long) mapping & FOLIO_MAPPING_FLAGS);
> > +
> > +	/* If it is the same (locklessly), then LGTM, proceed. */
> > +	if (mapping == last)
> > +		return true;
> > +	/*
> > +	 * Else we have to recheck with the folio lock held, for mapping
> > +	 * stability. TODO: killable?
> 
> I wouldn't've thought that'd be necessary.  The folio can't be being
> read because it's mapped, and we won't map a folio until it's uptodate.

Makes sense, I'll avoid the trouble then.

> 
> > +	 */
> > +	folio_lock(folio);
> > +	mapping = folio_mapping(folio);
> 
> I think you're safe to just look at folio->mapping here.  You have a
> refcount on the folio so it can't be freed, and I'm not sure there's a
> way to transition from page cache folio to anon folio without taking a
> trip through the page allocator.

Yep, makes sense. I don't think there is either. The worst that can happen is
that the folio could be truncated out while we have a reference but not the lock.
I think I just used the helper for the sake of using the helper, so I'll replace
it with ->mapping.

> 
> > +	/* May have been truncated, etc */
> > +	if (!mapping)
> > +		goto out_lock;
> 
> typically we call this "out_unlock".

ACK

> 
> > +	inode = mapping->host;
> > +	may = inode_owner_or_capable(&nop_mnt_idmap, inode) ||
> > +	      inode_permission(&nop_mnt_idmap, inode, MAY_WRITE) == 0;
> > +	if (likely(may))
> > +		*plast = mapping;
> > +out_lock:
> > +	folio_unlock(folio);
> > +	return may;
> > +}
> 
> I don't have a problem with the idea, other than it's really sad we have
> to do this.

Indeed :/

-- 
Pedro