* Re: [PATCH] Fix sync. in inode_has_no_xattr() [not found] <13eaa1c6-def5-afad-89eb-6f149db90684@tu-dortmund.de> @ 2018-12-05 15:32 ` Jan Kara 2018-12-07 10:24 ` Alexander Lochmann 0 siblings, 1 reply; 3+ messages in thread From: Jan Kara @ 2018-12-05 15:32 UTC (permalink / raw) To: Alexander Lochmann Cc: viro, linux-fsdevel, linux-kernel, Horst Schirmeier, Jan Kara, linux-block, Jens Axboe 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix sync. in inode_has_no_xattr() 2018-12-05 15:32 ` [PATCH] Fix sync. in inode_has_no_xattr() Jan Kara @ 2018-12-07 10:24 ` Alexander Lochmann 2018-12-07 11:14 ` Jan Kara 0 siblings, 1 reply; 3+ messages in thread From: Alexander Lochmann @ 2018-12-07 10:24 UTC (permalink / raw) To: Jan Kara Cc: viro, linux-fsdevel, linux-kernel, Horst Schirmeier, linux-block, Jens Axboe [-- 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 --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Fix sync. in inode_has_no_xattr() 2018-12-07 10:24 ` Alexander Lochmann @ 2018-12-07 11:14 ` Jan Kara 0 siblings, 0 replies; 3+ messages in thread From: Jan Kara @ 2018-12-07 11:14 UTC (permalink / raw) To: Alexander Lochmann Cc: Jan Kara, viro, linux-fsdevel, linux-kernel, Horst Schirmeier, linux-block, Jens Axboe 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-07 11:14 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <13eaa1c6-def5-afad-89eb-6f149db90684@tu-dortmund.de> 2018-12-05 15:32 ` [PATCH] Fix sync. in inode_has_no_xattr() Jan Kara 2018-12-07 10:24 ` Alexander Lochmann 2018-12-07 11:14 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).