[PATCH] sysfs: Fixup broken chown tracking

[PATCH] sysfs: Fixup broken chown tracking

[Jason Gunthorpe]

This patch:

commit ddd29ec6597125c830f7badb608a86c98b936b64
Author: David P. Quigley <dpquigl@tycho.nsa.gov>
Date:   Wed Sep 9 14:25:37 2009 -0400

    sysfs: Add labeling support for sysfs

Introduced a bug into the way sysfs handles attribute changes from user
space. Booting a kernel straight to busy box:

Linux version 2.6.33-rc8-00033-g5e96a56-dirty (jgg@bertha1) (gcc version 3.4.4) #37 Tue Mar 9 11:
[..]
Freeing unused kernel memory: 1184k init
BusyBox v1.1.0 (2006.12.14-23:35+0000) Built-in shell (ash)
/empty # chown 0.301 /sys/devices/system/fpga/fpga0/led_0b
/empty # chmod 0664 /sys/devices/system/fpga/fpga0/led_0b
/empty # ls -l /sys/devices/system/fpga/fpga0/led_0b
-rw-rw-r--    1 0        0            4096 Mar  9 18:44 /sys/devices/system/fpga/fpga0/led_0b

The chown was lost.

The problem seems to stem from moving the check of iattr to be
conditional on the first allocation of the sd_iattr. The chown
call path looks like:

[c308be50] [c00b1db4] sysfs_sd_setattr+0x34/0xf0 (unreliable)
[c308be70] [c00b1ee8] sysfs_setattr+0x78/0xa0
[c308be90] [c0086338] notify_change+0x19c/0x2d4
[c308bec0] [c00707d4] chown_common+0x84/0xa8
[c308bf20] [c007084c] sys_chown+0x54/0x80
[c308bf40] [c000dcf0] ret_from_syscall+0x0/0x3c

And sysfs_sd_setattr goes through the sysfs_init_inode_attrs allocation
path and completely ignores the passed in iattr that contains the
alteration from the chown.

This patch makes the run through the iattr mandatory in all cases - this
seems to solve the problem for me.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 fs/sysfs/inode.c |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

I've no idea if this is correct for any cases other than this single
one, but there is definitely a nasty bug here.

Hopefully someone more knowledgeable can sort this out :)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 220b758..b36f81c 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -81,24 +81,24 @@ int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
 		if (!sd_attrs)
 			return -ENOMEM;
 		sd->s_iattr = sd_attrs;
-	} else {
-		/* attributes were changed at least once in past */
-		iattrs = &sd_attrs->ia_iattr;
-
-		if (ia_valid & ATTR_UID)
-			iattrs->ia_uid = iattr->ia_uid;
-		if (ia_valid & ATTR_GID)
-			iattrs->ia_gid = iattr->ia_gid;
-		if (ia_valid & ATTR_ATIME)
-			iattrs->ia_atime = iattr->ia_atime;
-		if (ia_valid & ATTR_MTIME)
-			iattrs->ia_mtime = iattr->ia_mtime;
-		if (ia_valid & ATTR_CTIME)
-			iattrs->ia_ctime = iattr->ia_ctime;
-		if (ia_valid & ATTR_MODE) {
-			umode_t mode = iattr->ia_mode;
-			iattrs->ia_mode = sd->s_mode = mode;
-		}
+	}
+
+	/* attributes were changed at least once in past */
+	iattrs = &sd_attrs->ia_iattr;
+
+	if (ia_valid & ATTR_UID)
+		iattrs->ia_uid = iattr->ia_uid;
+	if (ia_valid & ATTR_GID)
+		iattrs->ia_gid = iattr->ia_gid;
+	if (ia_valid & ATTR_ATIME)
+		iattrs->ia_atime = iattr->ia_atime;
+	if (ia_valid & ATTR_MTIME)
+		iattrs->ia_mtime = iattr->ia_mtime;
+	if (ia_valid & ATTR_CTIME)
+		iattrs->ia_ctime = iattr->ia_ctime;
+	if (ia_valid & ATTR_MODE) {
+		umode_t mode = iattr->ia_mode;
+		iattrs->ia_mode = sd->s_mode = mode;
 	}
 	return 0;
 }
-- 
1.5.4.2

Re: [PATCH] sysfs: Fixup broken chown tracking

[David P. Quigley]

It looks like your patch is already in the tree as commit
7c0ff870d1ed287504a61ed865f3d728c757436b. We saw this problem before and
Eric fixed it up. I thought it was very odd that you posted this 5
minutes ago and it was already upstream.

Dave

Re: [PATCH] sysfs: Fixup broken chown tracking

[Greg KH]

On Tue, Mar 09, 2010 at 02:36:12PM -0500, David P. Quigley wrote:
> It looks like your patch is already in the tree as commit
> 7c0ff870d1ed287504a61ed865f3d728c757436b. We saw this problem before and
> Eric fixed it up. I thought it was very odd that you posted this 5
> minutes ago and it was already upstream.

Yeah, this has already been resolved, did that patch not solve the issue
for you?

thanks,

greg k-h

Re: [PATCH] sysfs: Fixup broken chown tracking

[David P. Quigley]

On Tue, 2010-03-09 at 11:42 -0800, Greg KH wrote:
> On Tue, Mar 09, 2010 at 02:36:12PM -0500, David P. Quigley wrote:
> > It looks like your patch is already in the tree as commit
> > 7c0ff870d1ed287504a61ed865f3d728c757436b. We saw this problem before and
> > Eric fixed it up. I thought it was very odd that you posted this 5
> > minutes ago and it was already upstream.
> 
> Yeah, this has already been resolved, did that patch not solve the issue
> for you?
> 
> thanks,
> 
> greg k-h

I don't see how it couldn't have. The patch he submitted which he says
fixes the problem is the exact same patch that Eric submitted back in
Feb. Its possible that he is using a version of 2.6.33-rc8 that is based
off of a commit before the patch was merged since 2.6.33-rc8 was tagged
on Feb 12th and your commit date in the repo is Feb 16th.

Re: [PATCH] sysfs: Fixup broken chown tracking

[Jason Gunthorpe]

On Tue, Mar 09, 2010 at 02:46:28PM -0500, David P. Quigley wrote:
> On Tue, 2010-03-09 at 11:42 -0800, Greg KH wrote:
> > On Tue, Mar 09, 2010 at 02:36:12PM -0500, David P. Quigley wrote:
> > > It looks like your patch is already in the tree as commit
> > > 7c0ff870d1ed287504a61ed865f3d728c757436b. We saw this problem before and
> > > Eric fixed it up. I thought it was very odd that you posted this 5
> > > minutes ago and it was already upstream.
> > 
> > Yeah, this has already been resolved, did that patch not solve the issue
> > for you?
> 
> I don't see how it couldn't have. The patch he submitted which he says
> fixes the problem is the exact same patch that Eric submitted back in
> Feb. Its possible that he is using a version of 2.6.33-rc8 that is based
> off of a commit before the patch was merged since 2.6.33-rc8 was tagged
> on Feb 12th and your commit date in the repo is Feb 16th.

Yes, that is correct, I was only looking up to 2.6.33-rc8, I didn't
think to check Linus's head branch, my mistake. It looks to me like
the changeset you pointed to was put in Linus's tree after that tag,
so it is fixed, just not in any tagged kernel.

Thanks for looking at this,
Jason

Re: [PATCH] sysfs: Fixup broken chown tracking

[Greg KH]

On Tue, Mar 09, 2010 at 12:53:03PM -0700, Jason Gunthorpe wrote:
> On Tue, Mar 09, 2010 at 02:46:28PM -0500, David P. Quigley wrote:
> > On Tue, 2010-03-09 at 11:42 -0800, Greg KH wrote:
> > > On Tue, Mar 09, 2010 at 02:36:12PM -0500, David P. Quigley wrote:
> > > > It looks like your patch is already in the tree as commit
> > > > 7c0ff870d1ed287504a61ed865f3d728c757436b. We saw this problem before and
> > > > Eric fixed it up. I thought it was very odd that you posted this 5
> > > > minutes ago and it was already upstream.
> > > 
> > > Yeah, this has already been resolved, did that patch not solve the issue
> > > for you?
> > 
> > I don't see how it couldn't have. The patch he submitted which he says
> > fixes the problem is the exact same patch that Eric submitted back in
> > Feb. Its possible that he is using a version of 2.6.33-rc8 that is based
> > off of a commit before the patch was merged since 2.6.33-rc8 was tagged
> > on Feb 12th and your commit date in the repo is Feb 16th.
> 
> Yes, that is correct, I was only looking up to 2.6.33-rc8, I didn't
> think to check Linus's head branch, my mistake. It looks to me like
> the changeset you pointed to was put in Linus's tree after that tag,
> so it is fixed, just not in any tagged kernel.

2.6.33 is tagged, as is 2.6.34-rc1, so it should be showing up on your
end by now :)

thanks,

greg k-h

Re: [PATCH] sysfs: Fixup broken chown tracking

[Jason Gunthorpe]

On Tue, Mar 09, 2010 at 12:03:19PM -0800, Greg KH wrote:
> > Yes, that is correct, I was only looking up to 2.6.33-rc8, I didn't
> > think to check Linus's head branch, my mistake. It looks to me like
> > the changeset you pointed to was put in Linus's tree after that tag,
> > so it is fixed, just not in any tagged kernel.
> 
> 2.6.33 is tagged, as is 2.6.34-rc1, so it should be showing up on your
> end by now :)

s/tagged kernel/tagged kernel in my git pull/

Network troubles today :|

git.kernel.org[0: 149.20.20.136]: errno=Connection timed out

Thanks,
Jason