[PATCH] Fix sync. in inode_has_no_xattr()

[PATCH] Fix sync. in inode_has_no_xattr()

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 1501 bytes --]


inode.i_flags might be altered without proper
synchronisation when the inode belongs to devtmpfs.
The following stacktrace shows how to get there:
13: entry_SYSENTER_32:460
12: do_fast_syscall_32:410
11: _static_cpu_has:146
10: do_syscall_32_irqs_on:322
09: SyS_pwrite64:636
08: SYSC_pwrite64:650
07: fdput:38
06: vfs_write:560
05: __vfs_write:512
04: new_sync_write:500
03: blkdev_write_iter:1977
02: __generic_file_write_iter:2897
01: file_remove_privs:1818
00: inode_has_no_xattr:3163

Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
Spinczyk)

Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
---
 include/linux/fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index c95c0807471f..40722678d741 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3443,10 +3443,14 @@ static inline int check_sticky(struct inode
*dir, struct inode *inode)
 	return __check_sticky(dir, inode);
 }

+/*
+ * blkdev_write_iter() can call this without i_rwsem, need to be
+ * careful with i_flags update.
+ */
 static inline void inode_has_no_xattr(struct inode *inode)
 {
 	if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & SB_NOSEC))
-		inode->i_flags |= S_NOSEC;
+		inode_set_flags(inode, S_NOSEC, S_NOSEC);
 }

 static inline bool is_root_inode(struct inode *inode)
-- 
2.19.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix sync. in inode_has_no_xattr()

[Jan Kara]

On Wed 05-12-18 12:45:06, Alexander Lochmann wrote:
> 
> inode.i_flags might be altered without proper
> synchronisation when the inode belongs to devtmpfs.
> The following stacktrace shows how to get there:
> 13: entry_SYSENTER_32:460
> 12: do_fast_syscall_32:410
> 11: _static_cpu_has:146
> 10: do_syscall_32_irqs_on:322
> 09: SyS_pwrite64:636
> 08: SYSC_pwrite64:650
> 07: fdput:38
> 06: vfs_write:560
> 05: __vfs_write:512
> 04: new_sync_write:500
> 03: blkdev_write_iter:1977
> 02: __generic_file_write_iter:2897
> 01: file_remove_privs:1818
> 00: inode_has_no_xattr:3163
> 
> Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
> Spinczyk)
> 
> Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
...
> +/*
> + * blkdev_write_iter() can call this without i_rwsem, need to be
> + * careful with i_flags update.
> + */
>  static inline void inode_has_no_xattr(struct inode *inode)
>  {
>  	if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & SB_NOSEC))
> -		inode->i_flags |= S_NOSEC;
> +		inode_set_flags(inode, S_NOSEC, S_NOSEC);
>  }

Thinking more about this I'm not sure if this is actually the right
solution. Because for example the write(2) can set S_NOSEC flag wrongly
when it would race with chmod adding SUID bit. So probably we rather need
to acquire i_rwsem in blkdev_write_iter() if file does not have S_NOSEC set
(we don't want to acquire it unconditionally as that would heavily impact
scalability of block device writes).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] Fix sync. in inode_has_no_xattr()

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 1067 bytes --]

Am 05.12.18 um 16:32 schrieb Jan Kara:
> 
> Thinking more about this I'm not sure if this is actually the right
> solution. Because for example the write(2) can set S_NOSEC flag wrongly
> when it would race with chmod adding SUID bit. So probably we rather need
> to acquire i_rwsem in blkdev_write_iter() if file does not have S_NOSEC set
> (we don't want to acquire it unconditionally as that would heavily impact
> scalability of block device writes).
> 
> 								Honza
> 
Trying to implement your suggestion, I'm not sure which inode to use:
In blkdev_write_iter() there is the "bd_inode = bdev_file_inode(file)".
file_remove_privs() uses "inode = file_inode(file)" as a parameter for
inode_has_no_xattr().
So, do file->f_mapping->host and f->f_inode refer to the identical inode?

- Alex

-- 
Technische Universität Dortmund
Alexander Lochmann                PGP key: 0xBC3EF6FD
Otto-Hahn-Str. 16                 phone:  +49.231.7556141
D-44227 Dortmund                  fax:    +49.231.7556116
http://ess.cs.tu-dortmund.de/Staff/al


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix sync. in inode_has_no_xattr()

[Jan Kara]

On Fri 07-12-18 11:24:47, Alexander Lochmann wrote:
> Am 05.12.18 um 16:32 schrieb Jan Kara:
> > 
> > Thinking more about this I'm not sure if this is actually the right
> > solution. Because for example the write(2) can set S_NOSEC flag wrongly
> > when it would race with chmod adding SUID bit. So probably we rather need
> > to acquire i_rwsem in blkdev_write_iter() if file does not have S_NOSEC set
> > (we don't want to acquire it unconditionally as that would heavily impact
> > scalability of block device writes).
> > 
> > 								Honza
> > 
> Trying to implement your suggestion, I'm not sure which inode to use:
> In blkdev_write_iter() there is the "bd_inode = bdev_file_inode(file)".
> file_remove_privs() uses "inode = file_inode(file)" as a parameter for
> inode_has_no_xattr().
> So, do file->f_mapping->host and f->f_inode refer to the identical inode?

Ah, that's a good question and I forgot to warn you. Sorry for that and my
respect for noticing this yourself :). For block devices f->f_inode refers
to the device inode in the filesystem (i.e., where xattrs are attached to,
permissions are stored, etc.).  file->f_mapping->host refers to the inode
carrying data - this split is needed so that if there are more instances of
device inode in the system for the same block device, they see data
coherently. So you need to use file_inode(file) for the locking. Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR