[PATCH 01/25] VFS: move attr_kill logic from notify_change into helper function

[Jeff Layton]

Separate the handling of the local ia_valid bitmask from the one in
attr->ia_valid. This allows us to hand off the actual handling of the
ATTR_KILL_* flags to the .setattr i_op when one is defined.

notify_change still needs to process those flags for the local ia_valid
variable, since it uses that to decide whether to return early, and to pass
a (hopefully) appropriate bitmask to fsnotify_change.

Signed-off-by: Jeff Layton <***@redhat.com>
---
fs/attr.c | 54 +++++++++++++++++++++++++++++++++------------------
include/linux/fs.h | 1 +
2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index f8dfc22..47015e0 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -100,15 +100,39 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
}
EXPORT_SYMBOL(inode_setattr);

+void attr_kill_to_mode(struct inode *inode, struct iattr *attr)
+{
+ if (attr->ia_valid & ATTR_KILL_SUID) {
+ attr->ia_valid &= ~ATTR_KILL_SUID;
+ if (inode->i_mode & S_ISUID) {
+ if (!(attr->ia_valid & ATTR_MODE)) {
+ attr->ia_valid |= ATTR_MODE;
+ attr->ia_mode = inode->i_mode;
+ }
+ attr->ia_mode &= ~S_ISUID;
+ }
+ }
+ if (attr->ia_valid & ATTR_KILL_SGID) {
+ attr->ia_valid &= ~ATTR_KILL_SGID;
+ if ((inode->i_mode & (S_ISGID | S_IXGRP)) ==
+ (S_ISGID | S_IXGRP)) {
+ if (!(attr->ia_valid & ATTR_MODE)) {
+ attr->ia_valid |= ATTR_MODE;
+ attr->ia_mode = inode->i_mode;
+ }
+ attr->ia_mode &= ~S_ISGID;
+ }
+ }
+}
+EXPORT_SYMBOL(attr_kill_to_mode);
+
int notify_change(struct dentry * dentry, struct iattr * attr)
{
struct inode *inode = dentry->d_inode;
- mode_t mode;
int error;
struct timespec now;
unsigned int ia_valid = attr->ia_valid;

- mode = inode->i_mode;
now = current_fs_time(inode->i_sb);

attr->ia_ctime = now;
@@ -117,26 +141,17 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
if (!(ia_valid & ATTR_MTIME_SET))
attr->ia_mtime = now;
if (ia_valid & ATTR_KILL_SUID) {
- attr->ia_valid &= ~ATTR_KILL_SUID;
- if (mode & S_ISUID) {
- if (!(ia_valid & ATTR_MODE)) {
- ia_valid = attr->ia_valid |= ATTR_MODE;
- attr->ia_mode = inode->i_mode;
- }
- attr->ia_mode &= ~S_ISUID;
- }
+ ia_valid &= ~ATTR_KILL_SUID;
+ if (inode->i_mode & S_ISUID)
+ ia_valid |= ATTR_MODE;
}
if (ia_valid & ATTR_KILL_SGID) {
- attr->ia_valid &= ~ ATTR_KILL_SGID;
- if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
- if (!(ia_valid & ATTR_MODE)) {
- ia_valid = attr->ia_valid |= ATTR_MODE;
- attr->ia_mode = inode->i_mode;
- }
- attr->ia_mode &= ~S_ISGID;
- }
+ ia_valid &= ~ATTR_KILL_SGID;
+ if ((inode->i_mode & (S_ISGID | S_IXGRP)) ==
+ (S_ISGID | S_IXGRP))
+ ia_valid |= ATTR_MODE;
}
- if (!attr->ia_valid)
+ if (!ia_valid)
return 0;

if (ia_valid & ATTR_SIZE)
@@ -147,6 +162,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
if (!error)
error = inode->i_op->setattr(dentry, attr);
} else {
+ attr_kill_to_mode(inode, attr);
error = inode_change_ok(inode, attr);
if (!error)
error = security_inode_setattr(dentry, attr);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d33bead..f617b23 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1561,6 +1561,7 @@ extern int do_remount_sb(struct super_block *sb, int flags,
#ifdef CONFIG_BLOCK
extern sector_t bmap(struct inode *, sector_t);
#endif
+extern void attr_kill_to_mode(struct inode *inode, struct iattr *attr);
extern int notify_change(struct dentry *, struct iattr *);
extern int permission(struct inode *, int, struct nameidata *);
extern int generic_permission(struct inode *, int,

[Miklos Szeredi]

Post by Jeff Layton
Separate the handling of the local ia_valid bitmask from the one in
attr->ia_valid. This allows us to hand off the actual handling of the
ATTR_KILL_* flags to the .setattr i_op when one is defined.
notify_change still needs to process those flags for the local ia_valid
variable, since it uses that to decide whether to return early, and to pass
a (hopefully) appropriate bitmask to fsnotify_change.

I agree with this change and fuse will make use of it as well.

Maybe instead of unconditionally moving attr_kill_to_mode() inside
->setattr() it could be made conditional based on an inode flag
similarly to S_NOCMTIME. Advantages:

- no need to modify a lot of in-tree filesystems
- no silent breakage of out-of-tree fs

Actually I think the new flag would be used by exacly the same
filesystems as S_NOCMTIME, so maybe it would make sense to rename
S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and
use that.

But that could still break out-of-tree fs, so a separate flag is
probably better.

Miklos

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[Jeff Layton]

On Mon, 06 Aug 2007 19:43:46 +0200

Post by Miklos Szeredi

Post by Jeff Layton
Separate the handling of the local ia_valid bitmask from the one in
attr->ia_valid. This allows us to hand off the actual handling of the
ATTR_KILL_* flags to the .setattr i_op when one is defined.
notify_change still needs to process those flags for the local ia_valid
variable, since it uses that to decide whether to return early, and to pass
a (hopefully) appropriate bitmask to fsnotify_change.

I agree with this change and fuse will make use of it as well.
Maybe instead of unconditionally moving attr_kill_to_mode() inside
->setattr() it could be made conditional based on an inode flag
- no need to modify a lot of in-tree filesystems
- no silent breakage of out-of-tree fs
Actually I think the new flag would be used by exacly the same
filesystems as S_NOCMTIME, so maybe it would make sense to rename
S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and
use that.
But that could still break out-of-tree fs, so a separate flag is
probably better.

In the past I've been told that adding new flags is something of a
"last resort". Since it's not strictly necessary to fix this then
it may be best to avoid that.

That said, if the concensus is that we need a transition mechanism,
then I'd be open to such a suggestion.

[Miklos Szeredi]

Post by Jeff Layton

Post by Miklos Szeredi
I agree with this change and fuse will make use of it as well.
Maybe instead of unconditionally moving attr_kill_to_mode() inside
->setattr() it could be made conditional based on an inode flag
- no need to modify a lot of in-tree filesystems
- no silent breakage of out-of-tree fs
Actually I think the new flag would be used by exacly the same
filesystems as S_NOCMTIME, so maybe it would make sense to rename
S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and
use that.
But that could still break out-of-tree fs, so a separate flag is
probably better.

In the past I've been told that adding new flags is something of a
"last resort". Since it's not strictly necessary to fix this then
it may be best to avoid that.
That said, if the concensus is that we need a transition mechanism,
then I'd be open to such a suggestion.

I think there's really no other choice here.

Your patch is changing the API in a very unsafe way, since there will
be no error or warning on an unconverted fs. And that could lead to
security holes.

If we would rename the setattr method to setattr_new as well as
changing it's behavior, that would be fine. But I guess we do not
want to do that.

Miklos

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[Trond Myklebust]

Post by Miklos Szeredi
Your patch is changing the API in a very unsafe way, since there will
be no error or warning on an unconverted fs. And that could lead to
security holes.
If we would rename the setattr method to setattr_new as well as
changing it's behavior, that would be fine. But I guess we do not
want to do that.

Which "unconverted fses"? If we're talking out of tree stuff, then too
bad: it is _their_ responsibility to keep up with kernel changes.

Trond

-
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Miklos Szeredi]

Post by Trond Myklebust

Post by Miklos Szeredi
Your patch is changing the API in a very unsafe way, since there will
be no error or warning on an unconverted fs. And that could lead to
security holes.
If we would rename the setattr method to setattr_new as well as
changing it's behavior, that would be fine. But I guess we do not
want to do that.

Which "unconverted fses"? If we're talking out of tree stuff, then too
bad: it is _their_ responsibility to keep up with kernel changes.

It is usually a good idea to not change the semantics of an API in a
backward incompatible way without changing the syntax as well.

This is true regardless of whether we care about out-of-tree code or
not (and we should care to some degree). And especially true if the
change in question is security sensitive.

Miklos

[Trond Myklebust]

Post by Miklos Szeredi

Post by Trond Myklebust

Post by Miklos Szeredi
Your patch is changing the API in a very unsafe way, since there will
be no error or warning on an unconverted fs. And that could lead to
security holes.
If we would rename the setattr method to setattr_new as well as
changing it's behavior, that would be fine. But I guess we do not
want to do that.

Which "unconverted fses"? If we're talking out of tree stuff, then too
bad: it is _their_ responsibility to keep up with kernel changes.

It is usually a good idea to not change the semantics of an API in a
backward incompatible way without changing the syntax as well.

We're taking two setattr flags ATTR_KILL_SGID, and ATTR_KILL_SUID which
have existed for several years in the VFS, and making them visible to
the filesystems. Out-of-tree filesystems that care can check for them in
a completely backward compatible way: you don't even need to add a
#define.

Post by Miklos Szeredi
This is true regardless of whether we care about out-of-tree code or
not (and we should care to some degree). And especially true if the
change in question is security sensitive.

It is not true "regardless": the in-tree code is being converted.
Out-of-tree code is the only "problem" here, and their only problem is
that they may have to add support for the new flags if they also support
suid/sgid mode bits.

Are you advocating reserving a new filesystem bit every time we need to
add an attribute flag?

Trond

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Christoph Hellwig]

Post by Jeff Layton
+void attr_kill_to_mode(struct inode *inode, struct iattr *attr)

This function badly needs a kerneldoc description. Also I can't say
I like the name a lot, but without a clearly better idea I should
probably not complain :)

We should at least add a generic_ prefix to indicate it's a generic
helper valid for most filesystem (and the kerneldoc comment can explain
the details)

[Jeff Layton]

On Tue, 7 Aug 2007 21:51:49 +0100

Post by Christoph Hellwig

Post by Jeff Layton
+void attr_kill_to_mode(struct inode *inode, struct iattr *attr)

This function badly needs a kerneldoc description. Also I can't say
I like the name a lot, but without a clearly better idea I should
probably not complain :)

Thanks for the comments.

I'm not thrilled with the name either, but kill_suid and *remove_suid
were already taken, and I really didn't want to name this something too
similar since there are already so many similarly named functions that
don't do the same thing. I'm definitely open to suggestions for
something different.

Post by Christoph Hellwig
We should at least add a generic_ prefix to indicate it's a generic
helper valid for most filesystem (and the kerneldoc comment can explain
the details)

Both good suggestions. I'll plan to incorporate them in the next
respin of the set.

Thanks,
--
Jeff Layton <***@redhat.com>