- 11 Mar, 2020 1 commit
-
-
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:
Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by:
Christoph Hellwig <hch@lst.de>
-
- 03 Mar, 2020 39 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:
Dave Chinner <dchinner@redhat.com> Signed-off-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:
-
Christoph Hellwig authored
Merge the ioctl handlers just like the low-level xfs_attr_set function. Signed-off-by:
-
Christoph Hellwig authored
The Linux xattr and acl APIs use a single call for set and remove. Modify the high-level XFS API to match that and let xfs_attr_set handle removing attributes as well. With a little bit of reordering this removes a lot of code. Signed-off-by:
-
Christoph Hellwig authored
Replace the ATTR_INCOMPLETE flag with a new boolean field in struct xfs_attr_list_context. Signed-off-by:
-
Christoph Hellwig authored
While the flags field in the ABI and the on-disk format allows for multiple namespace flags, an attribute can only exist in a single namespace at a time. Hence asking to list attributes that exist in multiple namespaces simultaneously is a logically invalid request and will return no results. Reject this case early with -EINVAL. Signed-off-by:
-
Brian Foster authored
The collapse range operation uses a unique transaction and ilock cycle for the hole punch and each extent shift iteration of the overall operation. While the hole punch is safe as a separate operation due to the iolock, cycling the ilock after each extent shift is risky w.r.t. concurrent operations, similar to insert range. To avoid this problem, make collapse range atomic with respect to ilock. Hold the ilock across the entire operation, replace the individual transactions with a single rolling transaction sequence and finish dfops on each iteration to perform pending frees and roll the transaction. Remove the unnecessary quota reservation as collapse range can only ever merge extents (and thus remove extent records and potentially free bmap blocks). The dfops call automatically relogs the inode to keep it moving in the log. This guarantees that nothing else can change the extent mapping of an inode while a collapse range operation is in progress. Signed-off-by:
Brian Foster <bfoster@redhat.com> Reviewed-by:
-
Brian Foster authored
The insert range operation uses a unique transaction and ilock cycle for the extent split and each extent shift iteration of the overall operation. While this works, it is risks racing with other operations in subtle ways such as COW writeback modifying an extent tree in the middle of a shift operation. To avoid this problem, make insert range atomic with respect to ilock. Hold the ilock across the entire operation, replace the individual transactions with a single rolling transaction sequence and relog the inode to keep it moving in the log. This guarantees that nothing else can change the extent mapping of an inode while an insert range operation is in progress. Signed-off-by:
Allison Collins <allison.henderson@oracle.com> Reviewed-by:
-
Brian Foster authored
The insert range operation currently splits the extent at the target offset in a separate transaction and lock cycle from the one that shifts extents. In preparation for reworking insert range into an atomic operation, lift the code into the caller so it can be easily condensed to a single rolling transaction and lock cycle and eliminate the helper. No functional changes. Signed-off-by:
-
Jules Irenge authored
Sparse reports a warning at xfs_ail_check() warning: context imbalance in xfs_ail_check() - unexpected unlock The root cause is the missing annotation at xfs_ail_check() Add the missing __must_hold(&ailp->ail_lock) annotation Signed-off-by:
Jules Irenge <jbi.octave@gmail.com> Reviewed-by:
-
Qian Cai authored
In xfs_da3_path_shift() "blk" can be assigned to state->path.blk[-1] if state->path.active is 1 (which is a valid state) when it tries to add an entry to a single dir leaf block and then to shift forward to see if there's a sibling block that would be a better place to put the new entry. This causes a UBSAN warning given negative array indices are undefined behavior in C. In practice the warning is entirely harmless given that "blk" is never dereferenced in this case, but it is still better to fix up the warning and slightly improve the code. UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_da_btree.c:1989:14 index -1 is out of range for type 'xfs_da_state_blk_t [5]' Call trace: dump_backtrace+0x0/0x2c8 show_stack+0x20/0x2c dump_stack+0xe8/0x150 __ubsan_handle_out_of_bounds+0xe4/0xfc xfs_da3_path_shift+0x860/0x86c [xfs] xfs_da3_node_lookup_int+0x7c8/0x934 [xfs] xfs_dir2_node_addname+0x2c8/0xcd0 [xfs] xfs_dir_createname+0x348/0x38c [xfs] xfs_create+0x6b0/0x8b4 [xfs] xfs_generic_create+0x12c/0x1f8 [xfs] xfs_vn_mknod+0x3c/0x4c [xfs] xfs_vn_create+0x34/0x44 [xfs] do_last+0xd4c/0x10c8 path_openat+0xbc/0x2f4 do_filp_open+0x74/0xf4 do_sys_openat2+0x98/0x180 __arm64_sys_openat+0xf8/0x170 do_el0_svc+0x170/0x240 el0_sync_handler+0x150/0x250 el0_sync+0x164/0x180 Suggested-by:
Christoph Hellwig <hch@infradead.org> Signed-off-by:
Qian Cai <cai@lca.pw> Reviewed-by:
-
Christoph Hellwig authored
Use printk_ratelimit() to limit the amount of messages printed from xfs_discard_page. Without that a failing device causes a large number of errors that doesn't really help debugging the underling issue. Signed-off-by:
-
Christoph Hellwig authored
Use printk_ratelimit() to limit the amount of messages printed from xfs_buf_ioerror_alert. Without that a failing device causes a large number of errors that doesn't really help debugging the underling issue. Signed-off-by:
-
