On Thu, Jan 18, 2024 at 11:30:29AM -0500, Gabriel Ryan wrote: > We found races in the fs subsystem in kernel v6.6 using a race testing > tool we are developing based on modified KCSAN. We are reporting the > races because they appear to be a potential bug. The races occur on > inode->i_mode, which is updated in > > fs/posix_acl.c:722 posix_acl_update_mode > > and can race with reads in the following locations: > > security/selinux/hooks.c:3087 selinux_inode_permission > include/linux/fsnotify.h:65 fsnotify_parent > include/linux/device_cgroup.h:24 devcgroup_inode_permission > fs/open.c:923,931 do_dentry_open > fs/namei.c:342 acl_permission_check > fs/namei.c:3242 may_open > > > In cases where multiple threads are updating and accessing a single > inode simultaneously, it seems like this could potentially lead to > undefined behavior, if for example an access check is passed based on > one i_mode setting, and then the inode->imode is modified by another > thread. Uhm, you need to provide more details than that. This is too vague to be meaningful especially without any traces. IIRC, posix_acl_update_mode() is called: * when a new inode is created before it has been added to the d+i-cache. Unless of course, there is a filesystem that updates the mode of the inode _after_ the inode has been added to the d+icache. * The other codepath would be setting an ACL directly via ->set_acl() through one of the setxattr() system calls. But even in that case the race would be semantically benign. IOW, if we passed the permission check on inode->i_mode and it's changed afterwads we don't care. We accept such races. So the issue you're referencing is what exactly? That the update to inode->i_mode can be done through a pointer when passing &inode->i_mode directly to posix_acl_update_mode() in which case you worry about torn reads/writes? So that would be fixed with a WRITE_ONCE() in posix_acl_update_mode()?
