Hello,
attached patch fixes possible deadlock spotted by LIOU Payphone. If we
set inode flags for quota files, we first lock i_mutex and then start a
transaction (which is a standard ordering) but quota files are special and
their i_mutex should be acquired only when a transaction is started.
Because users have no reason to muck with inode flags when quota is on
anyway, just forbid users from changing inode flags when quota is on.
I've done the change for all the filesystems supporting quota so that
they are consistent. Andrew, would you please queue it? Thanks.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
---
Forbid user from changing file flags on quota files. User has no bussiness
in playing with these flags when quota is on. Furthermore there is a remote
possibility of deadlock due to a lock inversion between quota file's i_mutex and
transaction's start (i_mutex for quota file is locked only when trasaction is
started in quota operations) in ext3 and ext4.
Signed-off-by: Jan Kara <[email protected]>
CC: LIOU Payphone <[email protected]>
diff -rupX /home/jack/.kerndiffexclude linux-2.6.24-rc1/fs/ext2/ioctl.c linux-2.6.24-rc1-1-forbid_setflags/fs/ext2/ioctl.c
--- linux-2.6.24-rc1/fs/ext2/ioctl.c 2007-10-24 20:43:56.000000000 +0200
+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext2/ioctl.c 2007-11-01 16:01:26.000000000 +0100
@@ -47,6 +47,11 @@ int ext2_ioctl (struct inode * inode, st
flags &= ~EXT2_DIRSYNC_FL;
mutex_lock(&inode->i_mutex);
+ /* Is it quota file? Don't allow user mess with it */
+ if (IS_NOQUOTA(inode)) {
+ mutex_unlock(&inode->i_mutex);
+ return -EPERM;
+ }
oldflags = ei->i_flags;
/*
diff -rupX /home/jack/.kerndiffexclude linux-2.6.24-rc1/fs/ext3/ioctl.c linux-2.6.24-rc1-1-forbid_setflags/fs/ext3/ioctl.c
--- linux-2.6.24-rc1/fs/ext3/ioctl.c 2007-10-11 12:01:23.000000000 +0200
+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext3/ioctl.c 2007-11-01 16:01:58.000000000 +0100
@@ -51,6 +51,11 @@ int ext3_ioctl (struct inode * inode, st
flags &= ~EXT3_DIRSYNC_FL;
mutex_lock(&inode->i_mutex);
+ /* Is it quota file? Don't allow user mess with it */
+ if (IS_NOQUOTA(inode)) {
+ mutex_unlock(&inode->i_mutex);
+ return -EPERM;
+ }
oldflags = ei->i_flags;
/* The JOURNAL_DATA flag is modifiable only by root */
diff -rupX /home/jack/.kerndiffexclude linux-2.6.24-rc1/fs/ext4/ioctl.c linux-2.6.24-rc1-1-forbid_setflags/fs/ext4/ioctl.c
--- linux-2.6.24-rc1/fs/ext4/ioctl.c 2007-10-11 12:01:23.000000000 +0200
+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext4/ioctl.c 2007-11-01 16:03:02.000000000 +0100
@@ -51,6 +51,11 @@ int ext4_ioctl (struct inode * inode, st
flags &= ~EXT4_DIRSYNC_FL;
mutex_lock(&inode->i_mutex);
+ /* Is it quota file? Don't allow user mess with it */
+ if (IS_NOQUOTA(inode)) {
+ mutex_unlock(&inode->i_mutex);
+ return -EPERM;
+ }
oldflags = ei->i_flags;
/* The JOURNAL_DATA flag is modifiable only by root */
diff -rupX /home/jack/.kerndiffexclude linux-2.6.24-rc1/fs/jfs/ioctl.c linux-2.6.24-rc1-1-forbid_setflags/fs/jfs/ioctl.c
--- linux-2.6.24-rc1/fs/jfs/ioctl.c 2007-10-11 12:01:23.000000000 +0200
+++ linux-2.6.24-rc1-1-forbid_setflags/fs/jfs/ioctl.c 2007-11-01 16:13:07.000000000 +0100
@@ -79,6 +79,9 @@ int jfs_ioctl(struct inode * inode, stru
if (!S_ISDIR(inode->i_mode))
flags &= ~JFS_DIRSYNC_FL;
+ /* Is it quota file? Don't allow user mess with it */
+ if (IS_NOQUOTA(inode))
+ return -EPERM;
jfs_get_inode_flags(jfs_inode);
oldflags = jfs_inode->mode2;
diff -rupX /home/jack/.kerndiffexclude linux-2.6.24-rc1/fs/reiserfs/ioctl.c linux-2.6.24-rc1-1-forbid_setflags/fs/reiserfs/ioctl.c
--- linux-2.6.24-rc1/fs/reiserfs/ioctl.c 2007-10-24 20:43:58.000000000 +0200
+++ linux-2.6.24-rc1-1-forbid_setflags/fs/reiserfs/ioctl.c 2007-11-01 16:13:27.000000000 +0100
@@ -57,6 +57,9 @@ int reiserfs_ioctl(struct inode *inode,
if (get_user(flags, (int __user *)arg))
return -EFAULT;
+ /* Is it quota file? Don't allow user mess with it. */
+ if (IS_NOQUOTA(inode))
+ return -EPERM;
if (((flags ^ REISERFS_I(inode)->
i_attrs) & (REISERFS_IMMUTABLE_FL |
REISERFS_APPEND_FL))
On Nov 1 2007 16:52, Jan Kara wrote:
>--- linux-2.6.24-rc1/fs/ext2/ioctl.c 2007-10-24 20:43:56.000000000 +0200
>+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext2/ioctl.c 2007-11-01 16:01:26.000000000 +0100
>@@ -47,6 +47,11 @@ int ext2_ioctl (struct inode * inode, st
> flags &= ~EXT2_DIRSYNC_FL;
>
> mutex_lock(&inode->i_mutex);
>+ /* Is it quota file? Don't allow user mess with it */
"Do not allow user to mess with it".
>--- linux-2.6.24-rc1/fs/ext3/ioctl.c 2007-10-11 12:01:23.000000000 +0200
>--- linux-2.6.24-rc1/fs/ext4/ioctl.c 2007-10-11 12:01:23.000000000 +0200
>--- linux-2.6.24-rc1/fs/jfs/ioctl.c 2007-10-11 12:01:23.000000000 +0200
>--- linux-2.6.24-rc1/fs/reiserfs/ioctl.c 2007-10-24 20:43:58.000000000 +0200
And the other filesystems are all fine?
On Fri 02-11-07 18:32:08, Jan Engelhardt wrote:
>
> On Nov 1 2007 16:52, Jan Kara wrote:
>
> >--- linux-2.6.24-rc1/fs/ext2/ioctl.c 2007-10-24 20:43:56.000000000 +0200
> >+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext2/ioctl.c 2007-11-01 16:01:26.000000000 +0100
> >@@ -47,6 +47,11 @@ int ext2_ioctl (struct inode * inode, st
> > flags &= ~EXT2_DIRSYNC_FL;
> >
> > mutex_lock(&inode->i_mutex);
> >+ /* Is it quota file? Don't allow user mess with it */
>
> "Do not allow user to mess with it".
I think both "allow user mess" and "allow user to mess" are possible, maybe
with a slightly different meanings. If you are confident that the first one
is not correct in this place, I can change it :)
> >--- linux-2.6.24-rc1/fs/ext3/ioctl.c 2007-10-11 12:01:23.000000000 +0200
> >--- linux-2.6.24-rc1/fs/ext4/ioctl.c 2007-10-11 12:01:23.000000000 +0200
> >--- linux-2.6.24-rc1/fs/jfs/ioctl.c 2007-10-11 12:01:23.000000000 +0200
> >--- linux-2.6.24-rc1/fs/reiserfs/ioctl.c 2007-10-24 20:43:58.000000000 +0200
>
> And the other filesystems are all fine?
I'm not sure what you mean... Forbidding IOC_SETFLAGS for them on quota
files is fine.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Jan Kara <[email protected]> writes:
> On Fri 02-11-07 18:32:08, Jan Engelhardt wrote:
>>
>> On Nov 1 2007 16:52, Jan Kara wrote:
>>
>> >--- linux-2.6.24-rc1/fs/ext2/ioctl.c 2007-10-24 20:43:56.000000000 +0200
>> >+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext2/ioctl.c 2007-11-01 16:01:26.000000000 +0100
>> >@@ -47,6 +47,11 @@ int ext2_ioctl (struct inode * inode, st
>> > flags &= ~EXT2_DIRSYNC_FL;
>> >
>> > mutex_lock(&inode->i_mutex);
>> >+ /* Is it quota file? Don't allow user mess with it */
>>
>> "Do not allow user to mess with it".
> I think both "allow user mess" and "allow user to mess" are possible,
I think it's either "to allow doing sth" or "to allow to do sth", but
"to allow do sth" looks definitely wrong to me.
Andreas.
--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
On Mon 05-11-07 11:47:01, Andreas Schwab wrote:
> Jan Kara <[email protected]> writes:
>
> > On Fri 02-11-07 18:32:08, Jan Engelhardt wrote:
> >>
> >> On Nov 1 2007 16:52, Jan Kara wrote:
> >>
> >> >--- linux-2.6.24-rc1/fs/ext2/ioctl.c 2007-10-24 20:43:56.000000000 +0200
> >> >+++ linux-2.6.24-rc1-1-forbid_setflags/fs/ext2/ioctl.c 2007-11-01 16:01:26.000000000 +0100
> >> >@@ -47,6 +47,11 @@ int ext2_ioctl (struct inode * inode, st
> >> > flags &= ~EXT2_DIRSYNC_FL;
> >> >
> >> > mutex_lock(&inode->i_mutex);
> >> >+ /* Is it quota file? Don't allow user mess with it */
> >>
> >> "Do not allow user to mess with it".
> > I think both "allow user mess" and "allow user to mess" are possible,
>
> I think it's either "to allow doing sth" or "to allow to do sth", but
> "to allow do sth" looks definitely wrong to me.
OK, you've convinced me :). Andrew, please fold this patch in the
previous one. Thanks.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
---
A grammar correction in a comment.
Signed-off-by: Jan Kara <[email protected]>
diff -rupX /home/jack/.kerndiffexclude linux-2.6.23-1-forbid_setflags/fs/ext2/ioctl.c linux-2.6.23-2-comment_fix/fs/ext2/ioctl.c
--- linux-2.6.23-1-forbid_setflags/fs/ext2/ioctl.c 2007-11-01 16:19:00.000000000 +0100
+++ linux-2.6.23-2-comment_fix/fs/ext2/ioctl.c 2007-11-05 17:53:53.000000000 +0100
@@ -46,7 +46,7 @@ int ext2_ioctl (struct inode * inode, st
flags &= ~EXT2_DIRSYNC_FL;
mutex_lock(&inode->i_mutex);
- /* Is it quota file? Don't allow user mess with it */
+ /* Is it quota file? Do not allow user to mess with it */
if (IS_NOQUOTA(inode)) {
mutex_unlock(&inode->i_mutex);
return -EPERM;
diff -rupX /home/jack/.kerndiffexclude linux-2.6.23-1-forbid_setflags/fs/ext3/ioctl.c linux-2.6.23-2-comment_fix/fs/ext3/ioctl.c
--- linux-2.6.23-1-forbid_setflags/fs/ext3/ioctl.c 2007-11-01 16:19:00.000000000 +0100
+++ linux-2.6.23-2-comment_fix/fs/ext3/ioctl.c 2007-11-05 17:54:00.000000000 +0100
@@ -51,7 +51,7 @@ int ext3_ioctl (struct inode * inode, st
flags &= ~EXT3_DIRSYNC_FL;
mutex_lock(&inode->i_mutex);
- /* Is it quota file? Don't allow user mess with it */
+ /* Is it quota file? Do not allow user to mess with it */
if (IS_NOQUOTA(inode)) {
mutex_unlock(&inode->i_mutex);
return -EPERM;
diff -rupX /home/jack/.kerndiffexclude linux-2.6.23-1-forbid_setflags/fs/ext4/ioctl.c linux-2.6.23-2-comment_fix/fs/ext4/ioctl.c
--- linux-2.6.23-1-forbid_setflags/fs/ext4/ioctl.c 2007-11-01 16:19:00.000000000 +0100
+++ linux-2.6.23-2-comment_fix/fs/ext4/ioctl.c 2007-11-05 17:54:07.000000000 +0100
@@ -51,7 +51,7 @@ int ext4_ioctl (struct inode * inode, st
flags &= ~EXT4_DIRSYNC_FL;
mutex_lock(&inode->i_mutex);
- /* Is it quota file? Don't allow user mess with it */
+ /* Is it quota file? Do not allow user to mess with it */
if (IS_NOQUOTA(inode)) {
mutex_unlock(&inode->i_mutex);
return -EPERM;
diff -rupX /home/jack/.kerndiffexclude linux-2.6.23-1-forbid_setflags/fs/jfs/ioctl.c linux-2.6.23-2-comment_fix/fs/jfs/ioctl.c
--- linux-2.6.23-1-forbid_setflags/fs/jfs/ioctl.c 2007-11-01 16:19:00.000000000 +0100
+++ linux-2.6.23-2-comment_fix/fs/jfs/ioctl.c 2007-11-05 17:53:39.000000000 +0100
@@ -79,7 +79,7 @@ int jfs_ioctl(struct inode * inode, stru
if (!S_ISDIR(inode->i_mode))
flags &= ~JFS_DIRSYNC_FL;
- /* Is it quota file? Don't allow user mess with it */
+ /* Is it quota file? Do not allow user to mess with it */
if (IS_NOQUOTA(inode))
return -EPERM;
jfs_get_inode_flags(jfs_inode);
diff -rupX /home/jack/.kerndiffexclude linux-2.6.23-1-forbid_setflags/fs/reiserfs/ioctl.c linux-2.6.23-2-comment_fix/fs/reiserfs/ioctl.c
--- linux-2.6.23-1-forbid_setflags/fs/reiserfs/ioctl.c 2007-11-01 16:19:00.000000000 +0100
+++ linux-2.6.23-2-comment_fix/fs/reiserfs/ioctl.c 2007-11-05 17:53:47.000000000 +0100
@@ -57,7 +57,7 @@ int reiserfs_ioctl(struct inode *inode,
if (get_user(flags, (int __user *)arg))
return -EFAULT;
- /* Is it quota file? Don't allow user mess with it. */
+ /* Is it quota file? Do not allow user to mess with it. */
if (IS_NOQUOTA(inode))
return -EPERM;
if (((flags ^ REISERFS_I(inode)->