Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
Jeff Layton wrote:
On Tue, 21 Aug 2007 17:21:28 -0400
Josef Sipek <[EMAIL PROTECTED]> wrote:
On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin <[EMAIL PROTECTED]> wrote:
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton <[EMAIL PROTECTED]>
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry->d_inode;
- unsigned intia_valid = attr->ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode->i_mode, attr);
+ ia_valid = attr->ia_valid;
+
if (ia_valid & ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr->ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin <[EMAIL PROTECTED]>
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op->setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op->setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
I should probably clarify -- in the hypothetical situation above,
the setattr function would have to call generic_attrkill (as most
filesystems should do with this change).
Thanks for the confirmation. That's what it looked like to me
but I wanted to know explicitly what the thinking was.
It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).
That's not a bad idea at all. I suppose that would be easier than
modifying every fs like this, and it does seem like it might be
cleaner. I need to mull it over, but that might be the best
solution.
Yeah, sounds a much more direct way of handling things and as you
say wouldn't need most of the filesystems to all be modified calling
generic_attrkill.
Not sure what the ramifications of adding a new iop are though.
Cheers,
Tim.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
Jeff Layton wrote:
On Tue, 21 Aug 2007 17:21:28 -0400
Josef Sipek [EMAIL PROTECTED] wrote:
On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin [EMAIL PROTECTED] wrote:
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton [EMAIL PROTECTED]
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry-d_inode;
- unsigned intia_valid = attr-ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode-i_mode, attr);
+ ia_valid = attr-ia_valid;
+
if (ia_valid ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr-ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin [EMAIL PROTECTED]
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op-setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op-setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
I should probably clarify -- in the hypothetical situation above,
the setattr function would have to call generic_attrkill (as most
filesystems should do with this change).
Thanks for the confirmation. That's what it looked like to me
but I wanted to know explicitly what the thinking was.
It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).
That's not a bad idea at all. I suppose that would be easier than
modifying every fs like this, and it does seem like it might be
cleaner. I need to mull it over, but that might be the best
solution.
Yeah, sounds a much more direct way of handling things and as you
say wouldn't need most of the filesystems to all be modified calling
generic_attrkill.
Not sure what the ramifications of adding a new iop are though.
Cheers,
Tim.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Tue, 21 Aug 2007 17:21:28 -0400
Josef Sipek <[EMAIL PROTECTED]> wrote:
> On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
> > On Tue, 21 Aug 2007 15:35:08 +1000
> > Timothy Shimmin <[EMAIL PROTECTED]> wrote:
> >
> > > Jeff Layton wrote:
> > > > This should fix all of the filesystems in the mainline kernels to handle
> > > > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> > > > just a matter of making sure that they call generic_attrkill early in
> > > > the setattr inode op.
> > > >
> > > > Signed-off-by: Jeff Layton <[EMAIL PROTECTED]>
> > > > ---
> > > > fs/xfs/linux-2.6/xfs_iops.c |5 -
> > > > --- a/fs/xfs/linux-2.6/xfs_iops.c
> > > > +++ b/fs/xfs/linux-2.6/xfs_iops.c
> > > > @@ -651,12 +651,15 @@ xfs_vn_setattr(
> > > > struct iattr*attr)
> > > > {
> > > > struct inode*inode = dentry->d_inode;
> > > > - unsigned intia_valid = attr->ia_valid;
> > > > + unsigned intia_valid;
> > > > bhv_vnode_t *vp = vn_from_inode(inode);
> > > > bhv_vattr_t vattr = { 0 };
> > > > int flags = 0;
> > > > int error;
> > > >
> > > > + generic_attrkill(inode->i_mode, attr);
> > > > + ia_valid = attr->ia_valid;
> > > > +
> > > > if (ia_valid & ATTR_UID) {
> > > > vattr.va_mask |= XFS_AT_UID;
> > > > vattr.va_uid = attr->ia_uid;
> > >
> > > Looks reasonable to me for XFS.
> > > Acked-by: Tim Shimmin <[EMAIL PROTECTED]>
> > >
> > > So before, this clearing would happen directly in notify_change()
> > > and now this won't happen until notify_change() calls i_op->setattr
> > > which for a particular fs it can call generic_attrkill() to do it.
> > > So I guess for the cases where i_op->setattr is called outside of
> > > via notify_change, we don't normally have ATTR_KILL_SUID/SGID
> > > set so that nothing will happen there?
> >
> > Right. If neither ATTR_KILL bit is set then generic_attrkill is a
> > noop.
> >
> > > I guess just wondering the effect with having the code on all
> > > setattr's. (I'm not familiar with the code path)
> > >
> >
> > These bits are referenced in very few places in the current kernel
> > tree -- mostly in the VFS layer. The *only* place I see that they
> > actually get interpreted into a mode change is in notify_change. So
> > places that call setattr ops w/o going through notify_change are
> > not likely to have those bits set.
> >
> > But hypothetically, if a fs did set ATTR_KILL_* and call setattr
> > directly, then the setattr would now include a mode change that
> > clears setuid or setgid bits where it may not have before.
>
I should probably clarify -- in the hypothetical situation above,
the setattr function would have to call generic_attrkill (as most
filesystems should do with this change).
> It almost sounds like an argument for a new inode op (NULL would use
> generic_attr_kill).
>
That's not a bad idea at all. I suppose that would be easier than
modifying every fs like this, and it does seem like it might be
cleaner. I need to mull it over, but that might be the best
solution.
--
Jeff Layton <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
> On Tue, 21 Aug 2007 15:35:08 +1000
> Timothy Shimmin <[EMAIL PROTECTED]> wrote:
>
> > Jeff Layton wrote:
> > > This should fix all of the filesystems in the mainline kernels to handle
> > > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> > > just a matter of making sure that they call generic_attrkill early in
> > > the setattr inode op.
> > >
> > > Signed-off-by: Jeff Layton <[EMAIL PROTECTED]>
> > > ---
> > > fs/xfs/linux-2.6/xfs_iops.c |5 -
> > > --- a/fs/xfs/linux-2.6/xfs_iops.c
> > > +++ b/fs/xfs/linux-2.6/xfs_iops.c
> > > @@ -651,12 +651,15 @@ xfs_vn_setattr(
> > > struct iattr*attr)
> > > {
> > > struct inode*inode = dentry->d_inode;
> > > - unsigned intia_valid = attr->ia_valid;
> > > + unsigned intia_valid;
> > > bhv_vnode_t *vp = vn_from_inode(inode);
> > > bhv_vattr_t vattr = { 0 };
> > > int flags = 0;
> > > int error;
> > >
> > > + generic_attrkill(inode->i_mode, attr);
> > > + ia_valid = attr->ia_valid;
> > > +
> > > if (ia_valid & ATTR_UID) {
> > > vattr.va_mask |= XFS_AT_UID;
> > > vattr.va_uid = attr->ia_uid;
> >
> > Looks reasonable to me for XFS.
> > Acked-by: Tim Shimmin <[EMAIL PROTECTED]>
> >
> > So before, this clearing would happen directly in notify_change()
> > and now this won't happen until notify_change() calls i_op->setattr
> > which for a particular fs it can call generic_attrkill() to do it.
> > So I guess for the cases where i_op->setattr is called outside of
> > via notify_change, we don't normally have ATTR_KILL_SUID/SGID
> > set so that nothing will happen there?
>
> Right. If neither ATTR_KILL bit is set then generic_attrkill is a
> noop.
>
> > I guess just wondering the effect with having the code on all
> > setattr's. (I'm not familiar with the code path)
> >
>
> These bits are referenced in very few places in the current kernel
> tree -- mostly in the VFS layer. The *only* place I see that they
> actually get interpreted into a mode change is in notify_change. So
> places that call setattr ops w/o going through notify_change are
> not likely to have those bits set.
>
> But hypothetically, if a fs did set ATTR_KILL_* and call setattr
> directly, then the setattr would now include a mode change that
> clears setuid or setgid bits where it may not have before.
It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).
Josef 'Jeff' Sipek.
--
A CRAY is the only computer that runs an endless loop in just 4 hours...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Mon, Aug 20, 2007 at 04:53:22PM -0400, Jeff Layton wrote: > This should fix all of the filesystems in the mainline kernels to handle > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is > just a matter of making sure that they call generic_attrkill early in > the setattr inode op. > > Signed-off-by: Jeff Layton <[EMAIL PROTECTED]> Coda part looks fine to me. Couldn't test it beyond 'it compiles and doesn't oops', because our userspace client unconditionally strips setuid/setgid bits anyways. Acked-by: Jan Harkes <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin <[EMAIL PROTECTED]> wrote:
> Jeff Layton wrote:
> > This should fix all of the filesystems in the mainline kernels to handle
> > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
> > just a matter of making sure that they call generic_attrkill early in
> > the setattr inode op.
> >
> > Signed-off-by: Jeff Layton <[EMAIL PROTECTED]>
> > ---
> > fs/xfs/linux-2.6/xfs_iops.c |5 -
> > --- a/fs/xfs/linux-2.6/xfs_iops.c
> > +++ b/fs/xfs/linux-2.6/xfs_iops.c
> > @@ -651,12 +651,15 @@ xfs_vn_setattr(
> > struct iattr*attr)
> > {
> > struct inode*inode = dentry->d_inode;
> > - unsigned intia_valid = attr->ia_valid;
> > + unsigned intia_valid;
> > bhv_vnode_t *vp = vn_from_inode(inode);
> > bhv_vattr_t vattr = { 0 };
> > int flags = 0;
> > int error;
> >
> > + generic_attrkill(inode->i_mode, attr);
> > + ia_valid = attr->ia_valid;
> > +
> > if (ia_valid & ATTR_UID) {
> > vattr.va_mask |= XFS_AT_UID;
> > vattr.va_uid = attr->ia_uid;
>
> Looks reasonable to me for XFS.
> Acked-by: Tim Shimmin <[EMAIL PROTECTED]>
>
> So before, this clearing would happen directly in notify_change()
> and now this won't happen until notify_change() calls i_op->setattr
> which for a particular fs it can call generic_attrkill() to do it.
> So I guess for the cases where i_op->setattr is called outside of
> via notify_change, we don't normally have ATTR_KILL_SUID/SGID
> set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
> I guess just wondering the effect with having the code on all
> setattr's. (I'm not familiar with the code path)
>
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
--
Jeff Layton <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin [EMAIL PROTECTED] wrote:
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton [EMAIL PROTECTED]
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry-d_inode;
- unsigned intia_valid = attr-ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode-i_mode, attr);
+ ia_valid = attr-ia_valid;
+
if (ia_valid ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr-ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin [EMAIL PROTECTED]
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op-setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op-setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
--
Jeff Layton [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Mon, Aug 20, 2007 at 04:53:22PM -0400, Jeff Layton wrote: This should fix all of the filesystems in the mainline kernels to handle ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is just a matter of making sure that they call generic_attrkill early in the setattr inode op. Signed-off-by: Jeff Layton [EMAIL PROTECTED] Coda part looks fine to me. Couldn't test it beyond 'it compiles and doesn't oops', because our userspace client unconditionally strips setuid/setgid bits anyways. Acked-by: Jan Harkes [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin [EMAIL PROTECTED] wrote:
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton [EMAIL PROTECTED]
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry-d_inode;
- unsigned intia_valid = attr-ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode-i_mode, attr);
+ ia_valid = attr-ia_valid;
+
if (ia_valid ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr-ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin [EMAIL PROTECTED]
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op-setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op-setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).
Josef 'Jeff' Sipek.
--
A CRAY is the only computer that runs an endless loop in just 4 hours...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Tue, 21 Aug 2007 17:21:28 -0400
Josef Sipek [EMAIL PROTECTED] wrote:
On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin [EMAIL PROTECTED] wrote:
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton [EMAIL PROTECTED]
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry-d_inode;
- unsigned intia_valid = attr-ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode-i_mode, attr);
+ ia_valid = attr-ia_valid;
+
if (ia_valid ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr-ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin [EMAIL PROTECTED]
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op-setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op-setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
I should probably clarify -- in the hypothetical situation above,
the setattr function would have to call generic_attrkill (as most
filesystems should do with this change).
It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).
That's not a bad idea at all. I suppose that would be easier than
modifying every fs like this, and it does seem like it might be
cleaner. I need to mull it over, but that might be the best
solution.
--
Jeff Layton [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton <[EMAIL PROTECTED]>
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry->d_inode;
- unsigned intia_valid = attr->ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode->i_mode, attr);
+ ia_valid = attr->ia_valid;
+
if (ia_valid & ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr->ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin <[EMAIL PROTECTED]>
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op->setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op->setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
--Tim
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Jfs-discussion] [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Mon, 2007-08-20 at 16:53 -0400, Jeff Layton wrote: > This should fix all of the filesystems in the mainline kernels to handle > ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is > just a matter of making sure that they call generic_attrkill early in > the setattr inode op. Here's my ack for the jfs piece. > > Signed-off-by: Jeff Layton <[EMAIL PROTECTED]> Acked-by: Dave Kleikamp <[EMAIL PROTECTED]> -- David Kleikamp IBM Linux Technology Center - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton <[EMAIL PROTECTED]>
---
arch/powerpc/platforms/cell/spufs/inode.c |1 +
fs/9p/vfs_inode.c |1 +
fs/affs/inode.c |1 +
fs/afs/inode.c|3 +++
fs/coda/inode.c |1 +
fs/configfs/inode.c |4 +++-
fs/ext2/inode.c |1 +
fs/ext3/inode.c |5 -
fs/ext4/inode.c |5 -
fs/fuse/dir.c |2 ++
fs/gfs2/ops_inode.c |2 ++
fs/hostfs/hostfs_kern.c |2 ++
fs/hpfs/inode.c |1 +
fs/hugetlbfs/inode.c |5 -
fs/jffs2/fs.c |1 +
fs/jfs/acl.c |2 ++
fs/ocfs2/file.c |2 ++
fs/proc/base.c|3 +++
fs/proc/generic.c |3 +++
fs/proc/proc_sysctl.c |3 +++
fs/ramfs/file-nommu.c |5 -
fs/reiserfs/inode.c |6 +-
fs/smbfs/inode.c |2 ++
fs/sysfs/inode.c |5 -
fs/ufs/truncate.c |5 -
fs/xfs/linux-2.6/xfs_iops.c |5 -
mm/shmem.c|2 ++
27 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c
b/arch/powerpc/platforms/cell/spufs/inode.c
index b3d0dd1..66144a1 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -97,6 +97,7 @@ spufs_setattr(struct dentry *dentry, struct iattr *attr)
{
struct inode *inode = dentry->d_inode;
+ generic_attrkill(inode->i_mode, attr);
if ((attr->ia_valid & ATTR_SIZE) &&
(attr->ia_size != inode->i_size))
return -EINVAL;
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index e5c45ee..a90ebf1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -756,6 +756,7 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct
iattr *iattr)
return PTR_ERR(fid);
v9fs_blank_wstat();
+ generic_attrkill(dentry->d_inode->i_mode, iattr);
if (iattr->ia_valid & ATTR_MODE)
wstat.mode = unixmode2p9mode(v9ses, iattr->ia_mode);
diff --git a/fs/affs/inode.c b/fs/affs/inode.c
index 4609a6c..e8dbedf 100644
--- a/fs/affs/inode.c
+++ b/fs/affs/inode.c
@@ -218,6 +218,7 @@ affs_notify_change(struct dentry *dentry, struct iattr
*attr)
pr_debug("AFFS: notify_change(%lu,0x%x)\n",inode->i_ino,attr->ia_valid);
+ generic_attrkill(inode->i_mode, attr);
error = inode_change_ok(inode,attr);
if (error)
goto out;
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 84750c8..8c43c93 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -371,6 +371,9 @@ int afs_setattr(struct dentry *dentry, struct iattr *attr)
vnode->fid.vid, vnode->fid.vnode, dentry->d_name.name,
attr->ia_valid);
+ /* FIXME: is this necessary? */
+ generic_attrkill(dentry->d_inode->i_mode, attr);
+
if (!(attr->ia_valid & (ATTR_SIZE | ATTR_MODE | ATTR_UID | ATTR_GID |
ATTR_MTIME))) {
_leave(" = 0 [unsupported]");
diff --git a/fs/coda/inode.c b/fs/coda/inode.c
index 342f4e0..7dec870 100644
--- a/fs/coda/inode.c
+++ b/fs/coda/inode.c
@@ -239,6 +239,7 @@ int coda_setattr(struct dentry *de, struct iattr *iattr)
memset(, 0, sizeof(vattr));
inode->i_ctime = CURRENT_TIME_SEC;
+ generic_attrkill(inode->i_mode, iattr);
coda_iattr_to_vattr(iattr, );
vattr.va_type = C_VNON; /* cannot set type */
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index dbd257d..d3d6637 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -59,7 +59,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr *
iattr)
struct inode * inode = dentry->d_inode;
struct configfs_dirent * sd = dentry->d_fsdata;
struct iattr * sd_iattr;
- unsigned int ia_valid = iattr->ia_valid;
+ unsigned int ia_valid;
int error;
if (!sd)
@@ -67,6 +67,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr *
iattr)
sd_iattr = sd->s_iattr;
+ generic_attrkill(inode->i_mode, iattr);
error = inode_change_ok(inode, iattr);
if (error)
return error;
@@ -90,6 +91,7 @@ int configfs_setattr(struct dentry *
[PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton [EMAIL PROTECTED]
---
arch/powerpc/platforms/cell/spufs/inode.c |1 +
fs/9p/vfs_inode.c |1 +
fs/affs/inode.c |1 +
fs/afs/inode.c|3 +++
fs/coda/inode.c |1 +
fs/configfs/inode.c |4 +++-
fs/ext2/inode.c |1 +
fs/ext3/inode.c |5 -
fs/ext4/inode.c |5 -
fs/fuse/dir.c |2 ++
fs/gfs2/ops_inode.c |2 ++
fs/hostfs/hostfs_kern.c |2 ++
fs/hpfs/inode.c |1 +
fs/hugetlbfs/inode.c |5 -
fs/jffs2/fs.c |1 +
fs/jfs/acl.c |2 ++
fs/ocfs2/file.c |2 ++
fs/proc/base.c|3 +++
fs/proc/generic.c |3 +++
fs/proc/proc_sysctl.c |3 +++
fs/ramfs/file-nommu.c |5 -
fs/reiserfs/inode.c |6 +-
fs/smbfs/inode.c |2 ++
fs/sysfs/inode.c |5 -
fs/ufs/truncate.c |5 -
fs/xfs/linux-2.6/xfs_iops.c |5 -
mm/shmem.c|2 ++
27 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c
b/arch/powerpc/platforms/cell/spufs/inode.c
index b3d0dd1..66144a1 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -97,6 +97,7 @@ spufs_setattr(struct dentry *dentry, struct iattr *attr)
{
struct inode *inode = dentry-d_inode;
+ generic_attrkill(inode-i_mode, attr);
if ((attr-ia_valid ATTR_SIZE)
(attr-ia_size != inode-i_size))
return -EINVAL;
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index e5c45ee..a90ebf1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -756,6 +756,7 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct
iattr *iattr)
return PTR_ERR(fid);
v9fs_blank_wstat(wstat);
+ generic_attrkill(dentry-d_inode-i_mode, iattr);
if (iattr-ia_valid ATTR_MODE)
wstat.mode = unixmode2p9mode(v9ses, iattr-ia_mode);
diff --git a/fs/affs/inode.c b/fs/affs/inode.c
index 4609a6c..e8dbedf 100644
--- a/fs/affs/inode.c
+++ b/fs/affs/inode.c
@@ -218,6 +218,7 @@ affs_notify_change(struct dentry *dentry, struct iattr
*attr)
pr_debug(AFFS: notify_change(%lu,0x%x)\n,inode-i_ino,attr-ia_valid);
+ generic_attrkill(inode-i_mode, attr);
error = inode_change_ok(inode,attr);
if (error)
goto out;
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 84750c8..8c43c93 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -371,6 +371,9 @@ int afs_setattr(struct dentry *dentry, struct iattr *attr)
vnode-fid.vid, vnode-fid.vnode, dentry-d_name.name,
attr-ia_valid);
+ /* FIXME: is this necessary? */
+ generic_attrkill(dentry-d_inode-i_mode, attr);
+
if (!(attr-ia_valid (ATTR_SIZE | ATTR_MODE | ATTR_UID | ATTR_GID |
ATTR_MTIME))) {
_leave( = 0 [unsupported]);
diff --git a/fs/coda/inode.c b/fs/coda/inode.c
index 342f4e0..7dec870 100644
--- a/fs/coda/inode.c
+++ b/fs/coda/inode.c
@@ -239,6 +239,7 @@ int coda_setattr(struct dentry *de, struct iattr *iattr)
memset(vattr, 0, sizeof(vattr));
inode-i_ctime = CURRENT_TIME_SEC;
+ generic_attrkill(inode-i_mode, iattr);
coda_iattr_to_vattr(iattr, vattr);
vattr.va_type = C_VNON; /* cannot set type */
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index dbd257d..d3d6637 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -59,7 +59,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr *
iattr)
struct inode * inode = dentry-d_inode;
struct configfs_dirent * sd = dentry-d_fsdata;
struct iattr * sd_iattr;
- unsigned int ia_valid = iattr-ia_valid;
+ unsigned int ia_valid;
int error;
if (!sd)
@@ -67,6 +67,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr *
iattr)
sd_iattr = sd-s_iattr;
+ generic_attrkill(inode-i_mode, iattr);
error = inode_change_ok(inode, iattr);
if (error)
return error;
@@ -90,6 +91,7 @@ int configfs_setattr(struct dentry * dentry, struct iattr *
Re: [Jfs-discussion] [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
On Mon, 2007-08-20 at 16:53 -0400, Jeff Layton wrote: This should fix all of the filesystems in the mainline kernels to handle ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is just a matter of making sure that they call generic_attrkill early in the setattr inode op. Here's my ack for the jfs piece. Signed-off-by: Jeff Layton [EMAIL PROTECTED] Acked-by: Dave Kleikamp [EMAIL PROTECTED] -- David Kleikamp IBM Linux Technology Center - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Fix mainline filesystems to handle ATTR_KILL_ bits correctly
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton [EMAIL PROTECTED]
---
fs/xfs/linux-2.6/xfs_iops.c |5 -
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr*attr)
{
struct inode*inode = dentry-d_inode;
- unsigned intia_valid = attr-ia_valid;
+ unsigned intia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode-i_mode, attr);
+ ia_valid = attr-ia_valid;
+
if (ia_valid ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr-ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin [EMAIL PROTECTED]
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op-setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op-setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
--Tim
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
