On Wed, Mar 04, 2020 at 10:45:33AM +1100, Dave Chinner wrote: > On Tue, Mar 03, 2020 at 08:38:53AM -0800, Darrick J. Wong wrote: > > On Mon, Mar 02, 2020 at 05:54:07PM -0600, Eric Sandeen wrote: > > > On 2/28/20 5:48 PM, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > > > > > Fix two problems in the dir3 free block read routine when we want to > > > > reject a corrupt free block. First, buffers should never have DONE set > > > > at the same time that b_error is EFSCORRUPTED. Second, don't leak a > > > > pointer back to the caller. > > > > > > For both of these things I'm left wondering; why does this particular > > > location need to have XBF_DONE cleared after the verifier error? Most > > > other locations that mark errors don't do this. > > > > Read verifier functions don't need to clear XBF_DONE because > > xfs_buf_reverify will notice b_error being set, and clear XBF_DONE for > > us. > > > > __xfs_dir3_free_read calls _read_buf. If the buffer read succeeds, > > _free_read then has xfs_dir3_free_header_check do some more checking on > > the buffer that we can't do in read verifiers. This is *outside* the > > regular read verifier (because we can't pass the owner into _read_buf) > > so if we're going to use xfs_verifier_error() to set b_error then we > > also have to clear XBF_DONE so that when we release the buffer a few > > lines later the buffer will be in a state that the buffer code expects. > > Actually, if the data in the buffer is bad after it has been > successfully read and we want to make sure it never gets used, the > buffer should be marked stale. > > That will prevent the buffer from being placed on the LRU when it is > released, and if a lookup finds it in cache it will clear /all/ the > flags on it > > xfs_da_read_buf() has read the buffer successfully, and set up it's > state so that it is cached via insertion into the LRU on release. We > want to make sure that nothing uses this buffer again without a > complete re-initialisation, and that's effectively what > xfs_buf_stale() does. > > > This isn't theoretical, if the _header_check fails then we start > > tripping the b_error assert the next time someone calls > > xfs_buf_reverify. > > We shouldn't be trying to re-use a corrupt buffer - it should cycle > out of memory immediately. Clearing the XBF_DONE flag doesn't > accomplish that; it works for buffer read verifier failures because > that results in the buffer being released before they are configured > to be cached on the LRU by the caller... > > Indeed, xfs_buf_read_map() already stales the buffer on read and > reverify failure.... I coded up making xfs_buf_corruption_error stale the buffer and it didn't let out the magic smoke, so I'll add that to this series. --D > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
