Re: Does NFSv4 need to call inode_permission on every write???

[Boaz Harrosh <boaz@plexistor.com>]

On 05/21/2015 04:07 PM, J. Bruce Fields wrote:
> On Thu, May 21, 2015 at 02:40:29PM +1000, NeilBrown wrote:
>>
>>
>> Apologies if this has been answered before, however...
>>
>> In nfsd_write() we have:
>>
>> 	if (file) {
>> 		err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
>> 				NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
>> 		if (err)
>> 			goto out;
>> 		err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
>> 				stablep);
>> 	} else {
>>
>> So if a 'file' is already available - because the request came via NFSv4 and
>> there was a valid state id, and a 'struct file' was associated with that
>> state - we still call nfsd_permission().
>>
>> Is that really needed?  The permission check will have been performed at open
>> - it shouldn't be needed again now.
>>
>> With NFSv3 we have to check permission at each IO, and this is slightly
>> different from POSIX semantics.  We shouldn't have to with NFSv4... should we?
>>
>> The particular issue that brought this to my attention is that "chattr +i" -
>> to make a file immutable - is not supposed to affect current opens, only
>> future opens.  But a current open over NFSv4 is affected.
>>
>> Is there some reason that we cannot just remove that nfsd_permission() check?
> 
> The only proof that this write is part of the open is a stateid provided
> as part of the write arguments.  Anyone could sniff or guess that
> stateid.
> 

Perhaps like in OSD take a few bits in the state ID for a crypto
sig on the stateid+internal-server-counter this way it will be
impossible to fake. (The crypto key is only known to the server
and is volatile) (Yes you'll need to fake something with the DSs
unless you want to communicate public keys for verification)

(No it is not implemented in open-osd, is just provisioned for
 in the STD)

Just a thought
Boaz

> We could try to make that work by checking the stateid against the
> principal from the rpc header.  Unfortunately that turns out to be more
> complicated than "is this the same principal as did the open"; among
> other things I think it's possible the stateid resulted from opens done
> by different principals, so we'd need to keep a list.  If we added that
> kind of check, could we drop the per-operation check?  It's not obvious
> to me.
> 
> --b.