Re: [PATCH] xfsqa: add testcase for ->setattr permission checking

[Christoph Hellwig <hch@infradead.org>]

On Mon, Dec 08, 2008 at 04:48:08PM +1100, Timothy Shimmin wrote:
> 1.
> > +echo "user: chown root owned file to qa_user (should fail)"
> > +su ${qa_user} -c "chown root test.${qa_user}"
> > +
> I think the description and command above don't match.
> I think we have a swap with subtest 4 below.
> Need to either swap descriptions or commands.

Yes, I swapped the descriptions.

> > +#
> > +# Setup a file owned by the qa_user and with the suid bit set.
> > +# A chown by root should not clean the suid bit.
> > +#
> 
> Typos:
> s/clean/clear/
> 
> s/suceed/succeed/ in a couple of places.

Yeah.

> * It looks like you test the clearing of suid/sgid bits
> for setting the mode permission bits and not
> for setting ownership as the description suggests;
> i.e. you test with chmod instead of chown for clearing of suid/sgid bits

Yes, that's also what I intended too, as XFS had some code to clear
the suid bits for changing permissions, but those shouldn't happen
for the restricted_chown case (and don't even happen in the XFS code,
it's just not obvious when reading the old setattr implementation).
While for sgid we want to clear it on mode changes if the gid
is not in the group list.  So what needs fixing here is once again
the comment.

Btw, I just noticed you checked in another testcase as 192.  Do you want
a respin or do you want to fix it up yourself?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs