On Mar 16, 2017, at 11:47 PM, Eric Biggers <[email protected]> wrote: > On Thu, Mar 16, 2017 at 11:35:33AM +0000, David Howells wrote: >> + >> + ext4_get_inode_flags(ei); >> + flags = ei->i_flags & EXT4_FL_USER_VISIBLE; >> + if (flags & EXT4_APPEND_FL) >> + stat->attributes |= STATX_ATTR_APPEND; >> + if (flags & EXT4_COMPR_FL) >> + stat->attributes |= STATX_ATTR_COMPRESSED; >> + if (flags & EXT4_ENCRYPT_FL) >> + stat->attributes |= STATX_ATTR_ENCRYPTED; >> + if (flags & EXT4_IMMUTABLE_FL) >> + stat->attributes |= STATX_ATTR_IMMUTABLE; >> + if (flags & EXT4_NODUMP_FL) >> + stat->attributes |= STATX_ATTR_NODUMP; >> >> - inode = d_inode(path->dentry); >> generic_fillattr(inode, stat); >> + return 0; >> +} > > I think it's the wrong approach to be calling ext4_get_inode_flags() here to > sync the generic inode flags (inode->i_flags) to the ext4-specific inode flags > (ei->i_flags). I know the GETFLAGS and FSGETXATTR ioctls do it too, but I > think it's a mistake --- at least, when done so without holding the inode > lock. > The issue is that flag syncs can occur in both directions concurrently and > cause an update to be lost. For example, with thread 1 doing a stat() and > thread 2 doing the SETFLAGS ioctl to set the APPEND flag: > > Thread 1, ext4_get_inode_flags() Thread 2, ext4_ioctl_setflags() > ----------------------------------- --------------------------- > Read inode->i_flags; S_APPEND clear > Set EXT4_APPEND_FL in ei->i_flags > Clear EXT4_APPEND_FL in ei->i_flags > Read ei->i_flags; EXT4_APPEND_FL clear > Clear S_APPEND in inode->i_flags > > So the flag set by SETFLAGS gets lost. This bug probably hasn't really been > noticable with GETFLAGS and FSGETXATTR since they're rarely used, but stat() > on > the other hand is very common, and I'm worried that with this change people > would start seeing this race more often. > > Ultimately this needs to be addressed in ext4 more fully, but how about for > ->getattr() just skipping the call to ext4_get_inode_flags() and instead > populating the generic attributes like STATX_ATTR_APPEND and > STATX_ATTR_IMMUTABLE from the generic inode flags, rather than from the > ext4-specific flags? Actually, it could even be done in generic_fillattr(), > so > that all filesystems benefit.
Wouldn't it make more sense to just extract the ext4 flags directly from
ei->i_flags? I think ext4_get_inode_flags() is only really useful when
the VFS inode flags are changed and they need to be propagated to the ext4
inode.
I guess the other more significant question is when/where are the VFS inode
flags changed that they need to be propagated into the ext4 disk inode?
The main avenue for changing these attribute flags that I know about is via
EXT4_IOC_SETFLAGS (FS_IOC_SETFLAGS), and there is one place that I could
find in fs/nsfs.c that sets S_IMMUTABLE but I don't think that propagates
down to an ext4 disk inode.
It seems like the use of ext4_get_inode_flags() is largely superfluous.
The original commit ff9ddf7e847 indicates this was for quota inodes only.
I think it can be removed from EXT4_IOC_FSGETXATTR, and EXT4_IOC_GETFLAGS
and made conditional in ext4_do_update_inode():
#ifdef CONFIG_QUOTA
for (i = 0; i < EXT4_MAXQUOTAS; i++) {
if (unlikely(sb_dqopt(sb)->files[i] == inode))
ext4_get_inode_flags(EXT4_I(inode));
}
#endif
Jan, what do you think?
Cheers, Andreas
>
>> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
>> index 73b184d161fc..4c6b23a0603e 100644
>> --- a/fs/ext4/symlink.c
>> +++ b/fs/ext4/symlink.c
>> @@ -91,11 +91,13 @@ const struct inode_operations
>> ext4_encrypted_symlink_inode_operations = {
>> const struct inode_operations ext4_symlink_inode_operations = {
>> .get_link = page_get_link,
>> .setattr = ext4_setattr,
>> + .getattr = ext4_getattr,
>> .listxattr = ext4_listxattr,
>> };
>>
>> const struct inode_operations ext4_fast_symlink_inode_operations = {
>> .get_link = simple_get_link,
>> .setattr = ext4_setattr,
>> + .getattr = ext4_getattr,
>> .listxattr = ext4_listxattr,
>> };
>>
>
> getattr needs to be added to ext4_encrypted_symlink_inode_operations too.
>
> - Eric
Cheers, Andreas
signature.asc
Description: Message signed with OpenPGP
