Re: [PATCH v3 01/33] jfs: free uncommitted anonymous tlocks when evicting an inode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] jfs: false WARN_ON_ONCE() in jfs_evict_inode() on read-write
  mounts
- [Medium] jfs: txAbortAnon() after jfs_free_zero_link() leaks metapages
  and tlocks
- [Medium] jfs: race between jfs_evict_inode(), txAbortAnon(), and
  txQuiesce()
--

commit a15655dff32c48b09c331d3e03e198e2173008c1
Author: Christian Brauner <brauner@kernel.org>

jfs: free uncommitted anonymous tlocks when evicting an inode

This commit adds txAbortAnon() and calls it from jfs_evict_inode() to
release uncommitted anonymous transaction locks and metapage pins when an
inode is evicted before writeback commits them.

> diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
> index c7914dbc91ed9..6390d915c7f11 100644
> --- a/fs/jfs/inode.c
> +++ b/fs/jfs/inode.c
> @@ -172,6 +172,15 @@ void jfs_evict_inode(struct inode *inode)
>  	clear_inode(inode);
>  	dquot_drop(inode);
>  
> +	/*
> +	 * Writeback commits the anonymous transactions of allocations. If the
> +	 * filesystem went read-only without a sync, e.g. after an error,
> +	 * nothing did.
> +	 */
> +	if (ji->atlhead) {
> +		WARN_ON_ONCE(!sb_rdonly(inode->i_sb));

[Severity: Medium]
Can WARN_ON_ONCE(!sb_rdonly(inode->i_sb)) fire on a read-write mount when
ji->atlhead is left populated without setting COMMIT_Dirty?

In extAlloc(), xtExtend() and xtInsert() acquire anonymous tlocks on
non-root xtree metapages via txLock() before splitting pages in
xtSplitUp().  When xtSplitUp() fails with -ENOSPC or -EDQUOT, extAlloc()
frees the data blocks and returns before calling mark_inode_dirty(ip):

fs/jfs/jfs_extent.c:extAlloc() {
	...
	if (xaddr && xaddr == nxaddr)
		rc = xtExtend(0, ip, xoff, (int) nxlen, 0);
	else
		rc = xtInsert(0, ip, xflag, xoff, (int) nxlen, &nxaddr, 0);

	/* if the extend or insert failed,
	 * free the newly allocated blocks and return the error.
	 */
	if (rc) {
		dbFree(ip, nxaddr, nxlen);
		dquot_free_block(ip, nxlen);
		mutex_unlock(&JFS_IP(ip)->commit_mutex);
		return (rc);
	}
	...
	mark_inode_dirty(ip);
}

Because COMMIT_Dirty is never set on the inode, jfs_write_inode() does not
commit the anonymous tlocks, and evicting the inode triggers the warning on
a read-write filesystem.  The warning also triggers after a filesystem
error when mounted with errors=continue, where jfs_handle_error() marks the
superblock dirty without setting SB_RDONLY, or via extRecord() ->
xtUpdate(), which is fixed in a later patch in this series (commit "jfs:
don't set SB_RDONLY after filesystem errors").

> +		txAbortAnon(inode);
> +	}

[Severity: Medium]
Does calling txAbortAnon() after jfs_free_zero_link() leak metapages and
linelocks when an unlinked inode with COMMIT_Freewmap is evicted after an
error?

Earlier in jfs_evict_inode(), an unlinked inode with COMMIT_Freewmap calls
jfs_free_zero_link() -> xtTruncate(..., COMMIT_WMAP).  For each non-root
xtree metapage, xtTruncate() marks mp->lid with tlckFREELOCK, calls
discard_metapage(mp), and finishes with txFreelock(ip):

fs/jfs/jfs_txnmgr.c:txFreelock() {
	...
	while ((lid = xtlck->next) != 0) {
		tlck = lid_to_tlock(lid);
		if (tlck->flag & tlckFREELOCK) {
			xtlck->next = tlck->next;
			txLockFree(lid);
		} else {
	...
}

If the transaction in jfs_unlink() or jfs_rename() aborted after
commitZeroLink() set COMMIT_Freewmap, ji->atlhead still holds anonymous
tlckPAGELOCK tlocks when jfs_evict_inode() runs.  txFreelock() unlinks
those tlocks from ji->atlhead and frees lid before txAbortAnon() runs,
without dropping mp->nohomeok, clearing mp->lid, or freeing chained
linelocks.  Calling txAbortAnon() before jfs_free_zero_link() would release
the metapage pins and linelocks before xtTruncate() discards the pages.

> diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
> index 083dbbb0c3268..85192dd4461a4 100644
> --- a/fs/jfs/jfs_txnmgr.c
> +++ b/fs/jfs/jfs_txnmgr.c
> @@ -2581,6 +2581,74 @@ void txFreelock(struct inode *ip)
>  	TXN_UNLOCK();
>  }
>  
> +/*
> + *	txAbortAnon()
> + *
> + * function:	free the anonymous tlocks of an inode that is evicted
> + *		before writeback committed them, e.g. because an error made
> + *		the filesystem read-only.
> + *
> + * A page that carries nothing but uncommitted changes isn't written home.
> + * A page that is also waiting to go home with committed changes (mp->lsn)
> + * goes home with both, after an error the superblock is marked dirty.
> + */
> +void txAbortAnon(struct inode *ip)
> +{
> +	struct jfs_inode_info *jfs_ip = JFS_IP(ip);
> +	struct linelock *linelock;
> +	struct metapage *mp;
> +	struct tlock *tlck;
> +	lid_t lid, next, llid, k;
> +
> +	/* txQuiesce() and jfs_sync() commit under the commit_mutex */
> +	mutex_lock(&jfs_ip->commit_mutex);
> +
> +	TXN_LOCK();
> +	lid = jfs_ip->atlhead;
> +	jfs_ip->atlhead = jfs_ip->atltail = 0;
> +	list_del_init(&jfs_ip->anon_inode_list);
> +	TXN_UNLOCK();

[Severity: Medium]
Can jfs_evict_inode() and txAbortAnon() race with txQuiesce() walking the
global TxAnchor.anon_list?

txQuiesce() walks TxAnchor.anon_list (shared across all JFS mounts) under
TXN_LOCK() and drops TXN_LOCK() without pinning the inode before taking
commit_mutex:

CPU0                              CPU1
txQuiesce()                       jfs_evict_inode()
  TXN_LOCK()                        txAbortAnon()
  jfs_ip = list_entry(...)            mutex_lock(&jfs_ip->commit_mutex)
  TXN_UNLOCK()                        TXN_LOCK()
                                      list_del_init(&jfs_ip->anon_inode_list)
                                      TXN_UNLOCK()
                                      ...
                                      mutex_unlock(&jfs_ip->commit_mutex)
                                    ...
                                  destroy_inode(ip)
  txBegin(ip->i_sb, ...)
  mutex_lock(&jfs_ip->commit_mutex)  <-- use-after-free

In the opposite interleaving, txCommit() called from txQuiesce() clears
jfs_ip->atlhead = 0 before taking TXN_LOCK() to remove
jfs_ip->anon_inode_list.  Because jfs_evict_inode() checks if (ji->atlhead)
without commit_mutex or TXN_LOCK(), it can see ji->atlhead == 0, skip
txAbortAnon(), and hit BUG_ON(!list_empty(&ji->anon_inode_list)) or free
the inode while txCommit() is still running.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-work-fs-sb_rdonly-v3-0-0be92bb73709@kernel.org?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/