On Tue, Jul 19, 2022 at 10:03:24AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > I observed the following evidence of a memory leak while running xfs/399 > from the xfs fsck test suite (edited for brevity): > > XFS (sde): Metadata corruption detected at xfs_attr_shortform_verify_struct.part.0+0x7b/0xb0 [xfs], inode 0x1172 attr fork > XFS: Assertion failed: ip->i_af.if_u1.if_data == NULL, file: fs/xfs/libxfs/xfs_inode_fork.c, line: 315 > ------------[ cut here ]------------ > WARNING: CPU: 2 PID: 91635 at fs/xfs/xfs_message.c:104 assfail+0x46/0x4a [xfs] > CPU: 2 PID: 91635 Comm: xfs_scrub Tainted: G W 5.19.0-rc7-xfsx #rc7 6e6475eb29fd9dda3181f81b7ca7ff961d277a40 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014 > RIP: 0010:assfail+0x46/0x4a [xfs] > Call Trace: > <TASK> > xfs_ifork_zap_attr+0x7c/0xb0 > xfs_iformat_attr_fork+0x86/0x110 > xfs_inode_from_disk+0x41d/0x480 > xfs_iget+0x389/0xd70 > xfs_bulkstat_one_int+0x5b/0x540 > xfs_bulkstat_iwalk+0x1e/0x30 > xfs_iwalk_ag_recs+0xd1/0x160 > xfs_iwalk_run_callbacks+0xb9/0x180 > xfs_iwalk_ag+0x1d8/0x2e0 > xfs_iwalk+0x141/0x220 > xfs_bulkstat+0x105/0x180 > xfs_ioc_bulkstat.constprop.0.isra.0+0xc5/0x130 > xfs_file_ioctl+0xa5f/0xef0 > __x64_sys_ioctl+0x82/0xa0 > do_syscall_64+0x2b/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > This newly-added assertion checks that there aren't any incore data > structures hanging off the incore fork when we're trying to reset its > contents. From the call trace, it is evident that iget was trying to > construct an incore inode from the ondisk inode, but the attr fork > verifier failed and we were trying to undo all the memory allocations > that we had done earlier. > > The three assertions in xfs_ifork_zap_attr check that the caller has > already called xfs_idestroy_fork, which clearly has not been done here. > As the zap function then zeroes the pointers, we've effectively leaked > the memory. > > The shortest change would have been to insert an extra call to > xfs_idestroy_fork, but it makes more sense to bundle the _idestroy_fork > call into _zap_attr, since all other callsites call _idestroy_fork > immediately prior to calling _zap_attr. IOWs, it eliminates one way to > fail. > > Note: This change only applies cleanly to 2ed5b09b3e8f, since we just > reworked the attr fork lifetime. However, I think this memory leak has > existed since 0f45a1b20cd8, since the chain xfs_iformat_attr_fork -> > xfs_iformat_local -> xfs_init_local_fork will allocate > ifp->if_u1.if_data, but if xfs_ifork_verify_local_attr fails, > xfs_iformat_attr_fork will free i_afp without freeing any of the stuff > hanging off i_afp. The solution for older kernels I think is to add the > missing call to xfs_idestroy_fork just prior to calling kmem_cache_free. > > Found by fuzzing a.sfattr.hdr.totsize = lastbit in xfs/399. > > Fixes: 2ed5b09b3e8f ("xfs: make inode attribute forks a permanent part of struct xfs_inode") > Probably-Fixes: 0f45a1b20cd8 ("xfs: improve local fork verification") > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> Looks good. Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> -- Dave Chinner david@xxxxxxxxxxxxx