Re: [RFC] vfs: cleanup of permission()

[Sam Vilain <sam@vilain.net>]

Trond Myklebust wrote:
>>after thinking some time about the oracle words
>>(sent in reply to previous BME submissions) we 
>>(Sam and I) came to the conclusion that it would 
>>be a good idea to remove the nameidata introduced
>>in September 2003 from the inode permission()
>>checks, so that vfs_permission() can take care
>>of them ...
> Why? There may be perfectly legitimate reasons for the filesystem to
> request information about the path.

Part of what we're trying to achieve is clearly seperating VFS from FS 
operations.  A lot of the problems with supporting the per-vfsmnt 
options stemmed from this confusion.

In fact this was largely developed from your feedback on the prior 
thread¹ :) we audited the uses, and you were right - some places really 
couldn't set that field properly.  After some playing and discussion, we 
came to guess that this kind of cleanup was a good way to satisfy your 
concerns as well as Christoph's².

¹ http://xrl.us/j9et
² http://xrl.us/j9eo

The only place that the nameidata structure is currently used is in 
those two places, and given that it *does* appear to represent FS vs VFS 
brain damage to some extent, it's quite important we get this right 
before umpteen security modules spring up using this bad hook ... the 
API is broken (doesn't cover (struct file*)-based operations, only 
lookup (struct nameidata*), and parameter is optional).

> I can think of server failover
> situations in NFSv4 where the client may need to look up the filehandle
> for the file on the new server before it can service the ACCESS call.

The current code doesn't seem to use that right now, at least not via 
permission().  Either way, the information should be available - it just 
needs to hook in somewhere else.

> Firstly, the fact that the lookup intent flags happen not to collide
> with MAY_* is a complete fluke, not a design. The numerical values of
> either set of flags could change tomorrow for all you know.
 > Secondly, an intent is _not_ a permissions mask by any stretch of the
 > imagination.

Yeah, I had this in there at one point as a safety check;

#if (_LOOKUP_MASK ^ _ACCESS_MASK) != (_LOOKUP_MASK | _ACCESS_MASK)
#error splode
#endif

You are right, it should either be converted to two parameters or the 
combination made more formal.

> IOW: at the very least make that intent flag a separate parameter.

Yes, or rename it mask_and_intent perhaps if they were to be combined in 
the same word (which makes sense, with a grand total of 7 bits being 
used, and gcc not being in a position to spot that and optimise for us).

> 
> Cheers,
>   Trond

Thanks for your feedback!

Sam.