Re: [xfs-masters] [Bug 768] New: Move restrict_chown to mount-time option

Re: [xfs-masters] [Bug 768] New: Move restrict_chown to mount-time option

[Josef Sipek]

On Thu, Sep 06, 2007 at 05:35:29PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 05, 2007 at 11:31:54AM -0700, bugzilla-daemon@oss.sgi.com wrote:
> > We've run into a situation where it would be extremely advantageous to be able
> > to restrict_chown on certain volumes on a server but not on others.  I'm not
> > real familiar with kernel internals, programming, etc., but I'd like to suggest
> > that restrict_chown be made a mount option for an XFS filesystem instead of a
> > system-wide, sysctl/proc option.  This would allow finer control over which
> > filesystems restrict giving away files and which don't.
> 
> It's quite easily doable.  I don't have time for that right now, but if
> anyone wants to do it's just adding the option to the mount option
> parser and adding a flag to the mount structure.
 
Wouldn't making this a generic mount-option make sense? Or is it far too
low-level of a concept?

Josef 'Jeff' Sipek.

-- 
I already backed up the [server] once, I can do it again.

Re: [xfs-masters] [Bug 768] New: Move restrict_chown to mount-time option

[Christoph Hellwig]

On Thu, Sep 06, 2007 at 12:10:06PM -0400, Josef Sipek wrote:
> > It's quite easily doable.  I don't have time for that right now, but if
> > anyone wants to do it's just adding the option to the mount option
> > parser and adding a flag to the mount structure.
>  
> Wouldn't making this a generic mount-option make sense? Or is it far too
> low-level of a concept?

Basically it's a simple boolean flag that's checked in the inode
allocator when we decide about the permission of the newly created
inode.  Because of that the implementation will be inherently
filesystem-specific.  We could still add a binary mount flag for it in
common code, but my stance is to only add these when we actually need
to check the flag in general code.

Re: [xfs-masters] [Bug 768] New: Move restrict_chown to mount-time option

[Josef Sipek]

On Fri, Sep 07, 2007 at 09:04:27PM +0200, Christoph Hellwig wrote:
> On Thu, Sep 06, 2007 at 12:10:06PM -0400, Josef Sipek wrote:
> > > It's quite easily doable.  I don't have time for that right now, but if
> > > anyone wants to do it's just adding the option to the mount option
> > > parser and adding a flag to the mount structure.
> >  
> > Wouldn't making this a generic mount-option make sense? Or is it far too
> > low-level of a concept?
> 
> Basically it's a simple boolean flag that's checked in the inode
> allocator when we decide about the permission of the newly created
> inode.  Because of that the implementation will be inherently
> filesystem-specific.

Couldn't that be done just before the call to ->create as a mask on the
mode?

> We could still add a binary mount flag for it in common code, but my
> stance is to only add these when we actually need to check the flag in
> general code.

Or if the feature is so useful that all fs should support it. Is it useful?
If not, then I agree, not cluttering the VFS is a Good Thing.

Josef 'Jeff' Sipek.

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are, by
definition, not smart enough to debug it.
		- Brian W. Kernighan 

Re: [xfs-masters] [Bug 768] New: Move restrict_chown to mount-time option

[Christoph Hellwig]

On Fri, Sep 07, 2007 at 03:15:31PM -0400, Josef Sipek wrote:
> On Fri, Sep 07, 2007 at 09:04:27PM +0200, Christoph Hellwig wrote:
> > On Thu, Sep 06, 2007 at 12:10:06PM -0400, Josef Sipek wrote:
> > > > It's quite easily doable.  I don't have time for that right now, but if
> > > > anyone wants to do it's just adding the option to the mount option
> > > > parser and adding a flag to the mount structure.
> > >  
> > > Wouldn't making this a generic mount-option make sense? Or is it far too
> > > low-level of a concept?
> > 
> > Basically it's a simple boolean flag that's checked in the inode
> > allocator when we decide about the permission of the newly created
> > inode.  Because of that the implementation will be inherently
> > filesystem-specific.
> 
> Couldn't that be done just before the call to ->create as a mask on the
> mode?

Sorry, my post above was talking about the bsd group semantics for which
we had a similar discussion before.  For restricted_chown the method
handling it is ->setattr and given how it's defined to be filesystem
specific I can't see how to do it generically.

> > We could still add a binary mount flag for it in common code, but my
> > stance is to only add these when we actually need to check the flag in
> > general code.
> 
> Or if the feature is so useful that all fs should support it. Is it useful?
> If not, then I agree, not cluttering the VFS is a Good Thing.

It's not really a feature, more a workaround for legacy behaviour in
other Unix variants.  It basically disallows chown in some cases where
it's normally allowed.