- 12 Mar, 2020 6 commits
-
-
Darrick J. Wong authored
If we decide that a directory free block is corrupt, we must take care not to leak a buffer pointer to the caller. After xfs_trans_brelse returns, the buffer can be freed or reused, which means that we have to set *bpp back to NULL. Callers are supposed to notice the nonzero return value and not use the buffer pointer, but we should code more defensively, even if all current callers handle this situation correctly. Fixes: de14c5f5 ("xfs: verify free block header fields") Signed-off-by:
Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by:
Dave Chinner <dchinner@redhat.com>
-
Darrick J. Wong authored
xfs_verifier_error is supposed to be called on a corrupt metadata buffer from within a buffer verifier function, whereas xfs_buf_mark_corrupt is the function to be called when a piece of code has read a buffer and catches something that a read verifier cannot. The first function sets b_error anticipating that the low level buffer handling code will see the nonzero b_error and clear XBF_DONE on the buffer, whereas the second function does not. Since xfs_dir3_free_header_check examines fields in the dir free block header that require more context than can be provided to read verifiers, we must call xfs_buf_mark_corrupt when it finds a problem. Switching the calls has a secondary effect that we no longer corrupt the buffer state by setting b_error and leaving XBF_DONE set. When /that/ happens, we'll trip over various state assertions (most commonly the b_error check in xfs_buf_reverify) on a subsequent attempt to read the buffer. Fixes: bc1a09b8 ("xfs: refactor verifier callers to print address of failing check") Signed-off-by:
-
Darrick J. Wong authored
Add a xfs_failaddr_t parameter to this function so that callers can potentially pass in (and therefore report) the exact point in the code where we decided that a metadata buffer was corrupt. This enables us to wire it up to checking functions that have to run outside of verifiers. Signed-off-by:
-
Darrick J. Wong authored
Add a helper function to get rid of buffers that we have decided are corrupt after the verifiers have run. This function is intended to handle metadata checks that can't happen in the verifiers, such as inter-block relationship checking. Note that we now mark the buffer stale so that it will not end up on any LRU and will be purged on release. Signed-off-by:
-
Darrick J. Wong authored
In e7ee96df, we converted all ITER_ABORT users to use ECANCELED instead, but we forgot to teach xfs_rmap_has_other_keys not to return that magic value to callers. Fix it now by using ECANCELED both to abort the iteration and to signal that we found another reverse mapping. This enables us to drop the separate boolean flag. Fixes: e7ee96df ("xfs: remove all *_ITER_ABORT values") Signed-off-by:
Christoph Hellwig <hch@lst.de>
-
Darrick J. Wong authored
Log the corrupt buffer before we release the buffer. Fixes: a5155b87 ("xfs: always log corruption errors") Signed-off-by:
-
- 11 Mar, 2020 6 commits
-
-
Christoph Hellwig authored
Just dereference bp->b_addr directly and make the code a little simpler and more clear. Signed-off-by:
Brian Foster <bfoster@redhat.com> Reviewed-by:
Eric Sandeen <sandeen@redhat.com> Reviewed-by:
-
Christoph Hellwig authored
Just dereference bp->b_addr directly and make the code a little simpler and more clear. Signed-off-by:
-
Christoph Hellwig authored
Just dereference bp->b_addr directly and make the code a little simpler and more clear. Signed-off-by:
-
Christoph Hellwig authored
There is just a single user left, so remove it. Signed-off-by:
-
Christoph Hellwig authored
struct xfs_agfl is a header in front of the AGFL entries that exists for CRC enabled file systems. For not CRC enabled file systems the AGFL is simply a list of agbno. Make the CRC case similar to that by just using the list behind the new header. This indirectly solves a problem with modern gcc versions that warn about taking addresses of packed structures (and we have to pack the AGFL given that gcc rounds up structure sizes). Also replace the helper macro to get from a buffer with an inline function in xfs_alloc.h to make the code easier to read. Signed-off-by:
-
Eric Biggers authored
Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set during do_exit(). That can confuse things. In particular, if BSD process accounting is enabled, then do_exit() writes data to an accounting file. If that file has FS_SYNC_FL set, then this write occurs synchronously and can misbehave if PF_MEMALLOC is set. For example, if the accounting file is located on an XFS filesystem, then a WARN_ON_ONCE() in iomap_do_writepage() is triggered and the data doesn't get written when it should. Or if the accounting file is located on an ext4 filesystem without a journal, then a WARN_ON_ONCE() in ext4_write_inode() is triggered and the inode doesn't get written. Fix this in xfsaild() by using the helper functions to save and restore PF_MEMALLOC. This can be reproduced as follows in the kvm-xfstests test appliance modified to add the 'acct' Debian package, and with kvm-xfstests's recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y: mkfs.xfs -f /dev/vdb mount /vdb touch /vdb/file chattr +S /vdb/file accton /vdb/file mkfs.xfs -f /dev/vdc mount /vdc umount /vdc It causes: WARNING: CPU: 1 PID: 336 at fs/iomap/buffered-io.c:1534 CPU: 1 PID: 336 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #3 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014 RIP: 0010:iomap_do_writepage+0x16b/0x1f0 fs/iomap/buffered-io.c:1534 [...] Call Trace: write_cache_pages+0x189/0x4d0 mm/page-writeback.c:2238 iomap_writepages+0x1c/0x33 fs/iomap/buffered-io.c:1642 xfs_vm_writepages+0x65/0x90 fs/xfs/xfs_aops.c:578 do_writepages+0x41/0xe0 mm/page-writeback.c:2344 __filemap_fdatawrite_range+0xd2/0x120 mm/filemap.c:421 file_write_and_wait_range+0x71/0xc0 mm/filemap.c:760 xfs_file_fsync+0x7a/0x2b0 fs/xfs/xfs_file.c:114 generic_write_sync include/linux/fs.h:2867 [inline] xfs_file_buffered_aio_write+0x379/0x3b0 fs/xfs/xfs_file.c:691 call_write_iter include/linux/fs.h:1901 [inline] new_sync_write+0x130/0x1d0 fs/read_write.c:483 __kernel_write+0x54/0xe0 fs/read_write.c:515 do_acct_process+0x122/0x170 kernel/acct.c:522 slow_acct_process kernel/acct.c:581 [inline] acct_process+0x1d4/0x27c kernel/acct.c:607 do_exit+0x83d/0xbc0 kernel/exit.c:791 kthread+0xf1/0x140 kernel/kthread.c:257 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352 This bug was originally reported by syzbot at https://lore.kernel.org/r/0000000000000e7156059f751d7b@google.com. Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com Signed-off-by:Eric Biggers <ebiggers@google.com> Reviewed-by:
-
- 03 Mar, 2020 28 commits
-
-
Christoph Hellwig authored
Let the low-level attr code only allocate the needed buffer size for xfs_attrmulti_attr_get instead of allocating the upper bound at the top of the call chain. Suggested-by:
-
Christoph Hellwig authored
No need to allocate the max size if we can just allocate the easily known actual ACL size. Suggested-by:
-
Christoph Hellwig authored
Use the round_down macro, and use the size of the uint32 type we use in the callback that fills the buffer to make the code a little more clear - the size of it is always the same as int for platforms that Linux runs on. Suggested-by:
-
Christoph Hellwig authored
The attrlist cursor only exists as part of an attr list context, so embedd the structure instead of pointing to it. Also give it a proper xfs_ prefix and remove the obsolete typedef. Signed-off-by:
Chandan Rajendra <chandanrlinux@gmail.com> Reviewed-by:
-
Christoph Hellwig authored
Now that we use the on-disk flags field also for the interface to the lower level attr routines we can use the XFS_ATTR_INCOMPLETE definition from the on-disk format directly instead. Signed-off-by:
-
Christoph Hellwig authored
The ATTR_* flags have a long IRIX history, where they a userspace interface, the on-disk format and an internal interface. We've split out the on-disk interface to the XFS_ATTR_* values, but despite (or because?) of that the flag have still been a mess. Switch the internal interface to pass the on-disk XFS_ATTR_* flags for the namespace and the Linux XATTR_* flags for the actual flags instead. The ATTR_* values that are actually used are move to xfs_fs.h with a new XFS_IOC_* prefix to not conflict with the userspace version that has the same name and must have the same value. Signed-off-by:
-
Christoph Hellwig authored
Remove superflous braces, elses after return statements and use a goto label to merge common error handling. Signed-off-by:
-
Christoph Hellwig authored
Move the function to xfs_acl.c and provide a proper stub for the !CONFIG_XFS_POSIX_ACL case. Lift the flags check to the caller as it nicely fits in there. Signed-off-by:
-
Christoph Hellwig authored
Lift the common code to copy the cursor from and to user space into xfs_ioc_attr_list. Note that this means we copy in twice now as the cursor is in the middle of the conaining structure, but we never touch the memory for the original copy. Doing so keeps the cursor handling isolated in the common helper. Signed-off-by:
-
Christoph Hellwig authored
Lift the buffer allocation from the two callers into xfs_ioc_attr_list. Signed-off-by:
-
Christoph Hellwig authored
Lift the flags and bufsize checks from both callers into the common code in xfs_ioc_attr_list. Signed-off-by:
-
Christoph Hellwig authored
The version taking the context structure is the main interface to list attributes, so drop the _int postfix. Signed-off-by:
-
Christoph Hellwig authored
The old xfs_attr_list code is only used by the attrlist by handle ioctl. Move it to xfs_ioctl.c with its user. Also move the attrlist and attrlist_ent structure to xfs_fs.h, as they are exposed user ABIs. They are used through libattr headers with the same name by at least xfsdump. Also document this relation so that it doesn't require a research project to figure out. Signed-off-by:
-
Christoph Hellwig authored
Replace a single use macro containing open-coded variants of standard helpers with direct calls to the standard helpers. Signed-off-by:
-
Christoph Hellwig authored
Signed-off-by:
-
Christoph Hellwig authored
Replace the alist char pointer with a void buffer given that different callers use it in different ways. Use the chance to remove the typedef and reduce the indentation of the struct definition so that it doesn't overflow 80 char lines all over. Signed-off-by:
-
Christoph Hellwig authored
Factor out a helper that compares an on-disk attr vs the name, length and flags specified in struct xfs_da_args. Signed-off-by:
-
Christoph Hellwig authored
op_flags with the XFS_DA_OP_* flags is the usual place for in-kernel only flags, so move the notime flag there. Signed-off-by:
-
Christoph Hellwig authored
Use a NULL args->value as the indicator to lazily allocate a buffer instead, and let the caller always free args->value instead of duplicating the cleanup. Signed-off-by:
-
Christoph Hellwig authored
We can just pass down the Linux convention of a zero valuelen to just query for the existance of an attribute to the low-level code instead. The use in the legacy xfs_attr_list code only used by the ioctl interface was already dead code, as the callers check that the flag is not present. Signed-off-by:
-
Christoph Hellwig authored
The inode can easily be derived from the args structure. Also don't bother with else statements after early returns. Signed-off-by:
-
Christoph Hellwig authored
Instead of converting from one style of arguments to another in xfs_attr_set, pass the structure from higher up in the call chain. Signed-off-by:
-
Christoph Hellwig authored
Instead of converting from one style of arguments to another in xfs_attr_set, pass the structure from higher up in the call chain. Signed-off-by:
-
Christoph Hellwig authored
The xattr values are blobs and should not be typed. Signed-off-by:
-
Christoph Hellwig authored
All the callers already check the length when allocating the in-kernel xattrs buffers. Signed-off-by:
-
Christoph Hellwig authored
All callers provide a valid name pointer, remove the redundant check. Signed-off-by:
-
Christoph Hellwig authored
Add a new helper to handle a single attr multi ioctl operation that can be shared between the native and compat ioctl implementation. There is a slight change in behaviour in that we don't break out of the loop when copying in the attribute name fails. The previous behaviour was rather inconsistent here as it continued for any other kind of error, and that we don't clear the flags in the structure returned to userspace, a behavior only introduced as a bug fix in the last merge window. Signed-off-by:
-
Christoph Hellwig authored
Simplify the user copy code by using strndup_user. This means that we now do one memory allocation per operation instead of one per ioctl, but memory allocations are cheap compared to the actual file system operations. Also the error for an invalid path is now EINVAL or EFAULT instead of the previous odd and undocumented ERANGE. Signed-off-by:
-
