| Message ID | 1425379119-3773-6-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 03, 2015 at 11:38:39AM +0100, Jan Kara wrote: > Currently XFS calls file_remove_privs() without holding i_mutex. This is > wrong because that function can end up messing with file permissions and > security xattrs for which we need i_mutex held. > > Fix the problem by grabbing iolock exclusively when we will need to > change anything in permissions / xattrs. > > Signed-off-by: Jan Kara <jack@suse.cz> Looks good. Reviewed-by: Dave Chinner <dchinner@redhat.com>
On 3/3/15 4:38 AM, Jan Kara wrote: > Currently XFS calls file_remove_privs() without holding i_mutex. This is > wrong because that function can end up messing with file permissions and > security xattrs for which we need i_mutex held. > > Fix the problem by grabbing iolock exclusively when we will need to > change anything in permissions / xattrs. > > Signed-off-by: Jan Kara <jack@suse.cz> This seems like it stands alone, modulo the file_remove_privs function renaming... should it just be pulled into XFS by Dave? I guess that would require the renaming patch to be rebased... Thanks, -Eric > --- > fs/xfs/xfs_file.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index fa81a43702ee..3d601e92bd44 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -563,6 +563,13 @@ restart: > if (error) > return error; > > + /* For changing security info in file_remove_privs() we need i_mutex */ > + if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) { > + xfs_rw_iunlock(ip, *iolock); > + *iolock = XFS_IOLOCK_EXCL; > + xfs_rw_ilock(ip, *iolock); > + goto restart; > + } > /* > * If the offset is beyond the size of the file, we need to zero any > * blocks that fall between the existing EOF and the start of this > @@ -601,7 +608,9 @@ restart: > * setgid bits if the process is not being run by root. This keeps > * people from modifying setuid and setgid binaries. > */ > - return file_remove_privs(file); > + if (!IS_NOSEC(inode)) > + return file_remove_privs(file); > + return 0; > } > > /* > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2015 at 11:54:55AM -0500, Eric Sandeen wrote: > On 3/3/15 4:38 AM, Jan Kara wrote: > > Currently XFS calls file_remove_privs() without holding i_mutex. This is > > wrong because that function can end up messing with file permissions and > > security xattrs for which we need i_mutex held. > > > > Fix the problem by grabbing iolock exclusively when we will need to > > change anything in permissions / xattrs. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > This seems like it stands alone, modulo the file_remove_privs function > renaming... should it just be pulled into XFS by Dave? I guess that would > require the renaming patch to be rebased... The file_remove_privs() is introduced earlier in the series. With all the changes pending to xfs_file_aio_write_checks() from the upcoming VFS merge, the DIo write fixes I'll be pushing, this, and other problems Al Viro has noticed, I'm probably going to have to refactor this code for 4.1-rc2 to fix all the bugs we've introduced during integration of all the changes... Cheers, Dave.
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index fa81a43702ee..3d601e92bd44 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -563,6 +563,13 @@ restart: if (error) return error; + /* For changing security info in file_remove_privs() we need i_mutex */ + if (*iolock == XFS_IOLOCK_SHARED && !IS_NOSEC(inode)) { + xfs_rw_iunlock(ip, *iolock); + *iolock = XFS_IOLOCK_EXCL; + xfs_rw_ilock(ip, *iolock); + goto restart; + } /* * If the offset is beyond the size of the file, we need to zero any * blocks that fall between the existing EOF and the start of this @@ -601,7 +608,9 @@ restart: * setgid bits if the process is not being run by root. This keeps * people from modifying setuid and setgid binaries. */ - return file_remove_privs(file); + if (!IS_NOSEC(inode)) + return file_remove_privs(file); + return 0; } /*
Currently XFS calls file_remove_privs() without holding i_mutex. This is wrong because that function can end up messing with file permissions and security xattrs for which we need i_mutex held. Fix the problem by grabbing iolock exclusively when we will need to change anything in permissions / xattrs. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/xfs/xfs_file.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)