nfsd: EACCES vs EPERM on utime()/utimes() calls

On 6/5/2015 4:27 AM, J. Bruce Fields wrote:
> On Thu, Jun 04, 2015 at 08:43:14PM +0800, Kinglong Mee wrote:
>> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
>>> On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
>>>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>>>>> Hello.
>>>>>
>>>>> As the man page for utime/utimes states [1], EPERM is returned if
>>>>> the second argument of utime/utimes is not NULL and:
>>>>>   * the caller's effective user id does not match the owner of the file
>>>>>   * the caller does not have write access to the file
>>>>>   * the caller is not privileged
>>>>>
>>>>> However, I don't see this behavior with NFS, I see EACCES is
>>>>> generated instead.
>>>>
>>>> Agreed that it's probably a server bug.  (Have you run across a case
>>>> where this makes a difference?)
>>>
>>> Thank you.
>>>
>>> No, I've not seen such a real-word scenario.
>>>
>>> I have these LTP test cases failing:
>>>
>>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
>>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
>>>
>>> and it makes me a bit nervous :)
>>>
>>>>
>>>> Looking at nfsd_setattr()....  The main work is done by notify_change(),
>>>> which is probably doing the right thing.  But before that there's an
>>>> fh_verify()--looks like that is expected to fail in your case.  I bet
>>>> that's the cause.
>>
>> Yes, it is.
>>
>> nfsd do the permission checking before notify_change() as,
>>
>> /* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
>> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
>>
>> return -EACCES for non-owner user.
>>
>>>
>>> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
>>>
>>> Anyway, if nobody is interested, I'll give it a try, but later.
>>
>> Here is a diff patch for this problem, please try testing.
>> If okay, I will send an official patch.
>>
>> Note: must apply the following patch first in the url,
>> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a
> 
> We could do that if we have to.
> 
> I wonder if we could instead skip the fh_verify's inode_permission call
> entirely?  Most callers of fh_verify don't need the inode_permission
> call at all as far as I can tell, because the following vfs operation
> does permission checking already.
> 
> We still need some nfs-specific checking, I guess (e.g. to make sure we
> aren't allowing setattr on a read-only export of a writeable mount), but
> the inode_permission itself I think we can usually skip.

I have made a draft for fh_verify without inode_permisson as following.
1. rename the check_nfsd_access() to rqst_check_access() that only
   checking the request's flavor.  (Maybe a split patch is better)
2. split a new helper nfsd_check_access() from nfsd_permission()
   for checking nfsd access.
3. nfsd_permission() is not changed by call nfsd_check_access().
4. let fh_verify() call nfsd_check_access(), not nfsd_permission().
5. add nfsd_permission() for do_open_permission().

I just test with pynfs, and the result is same as the old.

> 
> Andreas Gruenbacher has also been complaining that the redundant
> inode_permission calls make the richacl work more difficult, I can't
> remember why exactly.

I will check those patch and discuss of richacl, but maybe later. 

thanks,
Kinglong Mee

-----------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

nfsd: EACCES vs EPERM on utime()/utimes() calls

Commit Message

Patch