* [PATCH] Forbid user to change file flags on quota files
@ 2007-11-01 15:52 Jan Kara
2007-11-02 17:32 ` Jan Engelhardt
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2007-11-01 15:52 UTC (permalink / raw)
To: linux-kernel; +Cc: LIOU Payphone, Andrew Morton
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 <jack@suse.cz>
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 <jack@suse.cz>
CC: LIOU Payphone <lioupayphone@gmail.com>
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))
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Forbid user to change file flags on quota files
2007-11-01 15:52 [PATCH] Forbid user to change file flags on quota files Jan Kara
@ 2007-11-02 17:32 ` Jan Engelhardt
2007-11-05 10:32 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Jan Engelhardt @ 2007-11-02 17:32 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-kernel, LIOU Payphone, Andrew Morton
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Forbid user to change file flags on quota files
2007-11-02 17:32 ` Jan Engelhardt
@ 2007-11-05 10:32 ` Jan Kara
2007-11-05 10:47 ` Andreas Schwab
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2007-11-05 10:32 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: linux-kernel, LIOU Payphone, Andrew Morton
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 <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Forbid user to change file flags on quota files
2007-11-05 10:32 ` Jan Kara
@ 2007-11-05 10:47 ` Andreas Schwab
2007-11-05 16:58 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Andreas Schwab @ 2007-11-05 10:47 UTC (permalink / raw)
To: Jan Kara; +Cc: Jan Engelhardt, linux-kernel, LIOU Payphone, Andrew Morton
Jan Kara <jack@suse.cz> 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, schwab@suse.de
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."
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Forbid user to change file flags on quota files
2007-11-05 10:47 ` Andreas Schwab
@ 2007-11-05 16:58 ` Jan Kara
0 siblings, 0 replies; 5+ messages in thread
From: Jan Kara @ 2007-11-05 16:58 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jan Engelhardt, linux-kernel, LIOU Payphone, Andreas Schwab
On Mon 05-11-07 11:47:01, Andreas Schwab wrote:
> Jan Kara <jack@suse.cz> 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 <jack@suse.cz>
SUSE Labs, CR
---
A grammar correction in a comment.
Signed-off-by: Jan Kara <jack@suse.cz>
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)->
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-11-05 16:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-01 15:52 [PATCH] Forbid user to change file flags on quota files Jan Kara
2007-11-02 17:32 ` Jan Engelhardt
2007-11-05 10:32 ` Jan Kara
2007-11-05 10:47 ` Andreas Schwab
2007-11-05 16:58 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®