Received-SPF: pass (google.com: domain of linux-nfs+bounces-3647-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <8b882658-da3f-47ba-abd0-08656153fddd@talpey.com>
Date: Tue, 11 Jun 2024 09:32:25 -0400
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH] NFSD: Support write delegations for pNFS LAYOUT
 operations
To: Trond Myklebust <trondmy@hammerspace.com>,
 "chuck.lever@oracle.com" <chuck.lever@oracle.com>
Cc: "hch@lst.de" <hch@lst.de>, "cel@kernel.org" <cel@kernel.org>,
 "kolga@netapp.com" <kolga@netapp.com>,
 "jlayton@kernel.org" <jlayton@kernel.org>,
 "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
 "neilb@suse.de" <neilb@suse.de>, "dai.ngo@oracle.com" <dai.ngo@oracle.com>
References: <20240610150448.2377-2-cel@kernel.org>
 <30924327aaee17182a83e18bc86e6a27a19d88ab.camel@hammerspace.com>
 <Zmc7UOSMmeGcqkBa@tissot.1015granger.net>
 <0f25c54fde0089aa642de9e34fed0814dac8da17.camel@hammerspace.com>
Content-Language: en-US
From: Tom Talpey <tom@talpey.com>
In-Reply-To: <0f25c54fde0089aa642de9e34fed0814dac8da17.camel@hammerspace.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bXZZbGt1ODgrV0ZWREpFQ0d1YXhsbHJoRlVmdDUwTFlySWR3SWxjZG05UVlH?=
 =?utf-8?B?RmRiNzlDMUlSU2lLSklTKzhzWTZtUDV1MVlIc0VJYTVXeDlMSGg1ZVQySmRl?=
 =?utf-8?B?Q1BhZjA1aUdLTnpjSCtkRld4VjZNdmxUUjIyenlCRUFKSjFjOUp6cElwa3Zm?=
 =?utf-8?B?R2d5SGl0a1JGejNucXduRDRzL1lZdmtvZTVvZDBqbG5mWTNpUGNoZjRSWjJV?=
 =?utf-8?B?RFRDNUZYc01BSGRYc3NYTklrMEwrQldKQ2prcTZuRWUyUzczMEdQQkJhekdy?=
 =?utf-8?B?SUo4YW9hV0VuaEEvd2N2TWs2UjNsOWF6Z0VLT2Z1ZHFRN2RSY1VTckNPWjJa?=
 =?utf-8?B?dC9qdGxNbEpQNlhMYjBvekdsWXVjNU02TmgxclJ2RkFnNkd5aEtHV2NUb3pZ?=
 =?utf-8?B?SC9xSlpmbXlFeWFzMURXYXlhUHg1bTJSamVDK21ZR2NLOEpwUFJlT3ZsQTZT?=
 =?utf-8?B?c2dabnRQekFGVHA3SjlSUUJ0NGdTc0lnY1JxZHpieUszT2hHZmovcFJESlIr?=
 =?utf-8?B?K2RKTjk0dUJ6VTlqeVNURGY2Q1BRdnZXeE1MTTVocWFjMURpb01GYUR4L1dU?=
 =?utf-8?B?U3NPNVc2NmpjR3JXNXhCNDgwOU5oVjR1SmMwSkFwWm50SEpONEZzN0JEaUFS?=
 =?utf-8?B?dWcwaE9Sa3hvNXdESi96bUVsa0dhSTVXZ2xUY3RZWlMvZDNmQkd6NStBU2dq?=
 =?utf-8?B?SktQaTYzYUVqSVB0NFEvVmZNMENNaXRvVGw0NTRzeHg4NFFNNEowKytiWktw?=
 =?utf-8?B?R01NK3NMb21WQW1GbThKRnBpOEF1MjdPQ2YzQU44NXFBWlRvQ2k1ZEdlNTY2?=
 =?utf-8?B?TjV0ZUVlc0Uyd2Q0SURoWERFMjBPS0pCL243RFlOYjJOaE0yYmlNWWdwT3hB?=
 =?utf-8?B?SFdtcnltellXYmsxUndMTGpVZ1htZlV3YTFkd0VJWGNlOHdkdnlyZTVScTJY?=
 =?utf-8?B?RVRzbVZ5VkZzd3ZvZFh0Q1J1R1NtZ2VwcXNYSUlTKzRJbENSM2hQRldUcjZq?=
 =?utf-8?B?ME9Vb2RjdGRkTEp1Nmp4ek9iZ0Zvc3JYL1N3cFBWM25uR1RaZTJaa0NnWWtv?=
 =?utf-8?B?MksyUEhnQjBvbnhvTGJUNEdtdlE5Q2daRWhHZFBJOW02MlV2QThyU2NWSHVS?=
 =?utf-8?B?d1oyYnZRZFc2bktQOWRQb1BrL1FGelc3YlBCbmJJWUlCbGYvUGxkbTFKWEZX?=
 =?utf-8?B?TVJLdXdzcTZTVko3NmdZSUluSXNhRS9rT0wwbDJjZjZCT0JwcEUxdkJVN2ox?=
 =?utf-8?B?UGZEWG5SM21qVE5yWFd5aUdhWWM1c3d4Nnk3M2gyRjZpeWpEbHJuWkkwaHpk?=
 =?utf-8?B?WE5ydGgrcE1IaDNoS2JScWtVZ3preE5mS2xzRXB1ZE1FaG5tSitzcURxS25S?=
 =?utf-8?B?ZFNYWmViRnFLdUZvNnhiVlRxU3VSQ2xUV1hLQ205RXliT0dqd1dpeWYxOE9h?=
 =?utf-8?B?LzJWclRyeUlzdWF2aTR0QUI3ZTd0MEk4WmFCK0Q2VXR3TzRVUzlNdWtORFlk?=
 =?utf-8?B?dVpYWVp1K01WWHJmNjFtMG1HOWtpWCtISDJPVXJ6MndLRFRoZ2pwY1hIbDN2?=
 =?utf-8?B?Vi84bnVYVy9KVGdmZWVMaVkrenpJMGdFRnQ1QjBuODhTdlI1Z3JvUi9YUTlk?=
 =?utf-8?B?YXBiWHJTckQyMVRUMzVxSElxNGZKcXN4UENSTVBKSm1pV1l1b1FLT20yQVB5?=
 =?utf-8?B?b0h3REhXYmljT0hQU0hvbkpUbVlGbEZBYzhSN1p2L2RkOEhCMzVTUXR5WkZr?=
 =?utf-8?B?UkNGbFk5WE1SdVZ3T0RrakpIZ2h5RGMrb3NQV01HMFRrbjVvQklSOFVFeENX?=
 =?utf-8?B?cGk2bmI3ZkYwanBUbHVGY1ZJajJtUWJ4ODFvS2poQXh1MGpQa1VhVnV3MExS?=
 =?utf-8?B?K201Qjd6KytBeTh0bTY1WUs5OE44aElOa1hkQXlUK0pwUkMxUzBnamFjL3R6?=
 =?utf-8?B?b3MzZFZzYmk5eHM2STFveTdwdTQrSlRodjRsNFVUMytlRThpUUFSRlFuVWNM?=
 =?utf-8?B?VU5LUUYwMTg0dXhhZXoyQUIyMGVWR0xOWUhFbURWSXZWRWUyVE9tMW56OGZa?=
 =?utf-8?B?dEdBWi80aHI4WFdwZzhUSExZWTV1dWhQZDZ6emNObUNFTDh6d3JZeW9kc0xV?=
 =?utf-8?Q?Xim9L/TDktheF9V3NEwLIdMu+?=
X-OriginatorOrg: talpey.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 453c64cc-a18d-4775-0c6e-08dc8a1af0fc
X-MS-Exchange-CrossTenant-AuthSource: CH0PR01MB7170.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 13:32:29.9167
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 2b2dcae7-2555-4add-bc80-48756da031d5
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3EI2vtlDYHD5nnMmPMleTz1Gsyr54XrCxRzeuEmRjhDT7UxYDVSBBAJo+d5AeJWo
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR01MB7999

On 6/10/2024 7:54 PM, Trond Myklebust wrote:
> On Mon, 2024-06-10 at 13:43 -0400, Chuck Lever wrote:
>> On Mon, Jun 10, 2024 at 04:21:33PM +0000, Trond Myklebust wrote:
>>> On Mon, 2024-06-10 at 11:04 -0400, cel@kernel.org wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>
>>>> I noticed LAYOUTGET(LAYOUTIOMODE4_RW) returning NFS4ERR_ACCESS
>>>> unexpectedly. The NFS client had created a file with mode 0444,
>>>> and
>>>> the server had returned a write delegation on the OPEN(CREATE).
>>>> The
>>>> client was requesting a RW layout using the write delegation
>>>> stateid
>>>> so that it could flush file modifications.
>>>>
>>>> This client behavior was permitted for NFSv4.1 without pNFS, so I
>>>> began looking at NFSD's implementation of LAYOUTGET.
>>>>
>>>> The failure was because fh_verify() was doing a permission check
>>>> as
>>>> part of verifying the FH. It uses the loga_iomode value to
>>>> specify
>>>> the @accmode argument. fh_verify(MAY_WRITE) on a file whose mode
>>>> is
>>>> 0444 fails with -EACCES.
>>>>
>>>> RFC 8881 Section 18.43.3 states:
>>>>> The use of the loga_iomode field depends upon the layout type,
>>>>> but
>>>>> should reflect the client's data access intent.
>>>>
>>>> Further discussion of iomode values focuses on how the server is
>>>> permitted to change returned the iomode when coalescing layouts.
>>>> It says nothing about mandating the denial of LAYOUTGET requests
>>>> due to file permission settings.
>>>>
>>>> Appropriate permission checking is done when the client acquires
>>>> the
>>>> stateid used in the LAYOUTGET operation, so remove the permission
>>>> check from LAYOUTGET and LAYOUTCOMMIT, and rely on layout stateid
>>>> checking instead.
>>>
>>> Hmm... I'm not seeing any checking or enforcement of the
>>> EXCHGID4_FLAG_BIND_PRINC_STATEID flag in knfsd.
>>
>> I appreciate your review!
>>
>> I see that BIND_PRINC_STATEID is not set by NFSD. RFC 8881 Section
>> 18.16.4 says:
>>> Note that if the client ID was not created with the
>>> EXCHGID4_FLAG_BIND_PRINC_STATEID capability set in the reply to
>>> EXCHANGE_ID, then the server MUST NOT impose any requirement that
>>> READs and WRITEs sent for an open file have the same credentials
>>> as the OPEN itself, and the server is REQUIRED to perform access
>>> checking on the READs and WRITEs themselves.
>>
>>
>> Trond:
>>> Doesn't that mean that
>>> READ and WRITE are required to check access permissions, as per
>>> RFC8881, section 13.9.2.3?
>>
>> Every NFSv4 READ and WRITE calls nfs4_preprocess_stateid_op(), and
>> nfs4_preprocess_stateid_op() calls nfsd_permission() (in
>> nfs4_check_file()). Seems like we're covered.
>>
>>
>> Trond:
>>> I'm not saying that the return of an ACCESS error is correct here,
>>> since the file was created using this credential, and so the caller
>>> should benefit from having owner privileges.
>>
>> The alternative is to preserve the accmode logic but instead add the
>> NFSD_MAY_OWNER_OVERRIDE flag, me thinks, which is not objectionable.
>>
>>
>> Trond:
>>> However the issue is that knfsd should be either checking that the
>>> credential is correct w.r.t. the stateid (if
>>> EXCHGID4_FLAG_BIND_PRINC_STATEID is set and the stateid is not a
>>> special stateid) or that it is correct w.r.t. the file permissions
>>> (if
>>> EXCHGID4_FLAG_BIND_PRINC_STATEID is not set or the stateid is a
>>> special
>>> stateid).
>>
>> But LAYOUTGET is not a READ or WRITE. I don't see language that
>> requires stateid / credential checking for it, but it's always
>> possible I might have missed something.
>>
>> Also, RFC 5663 has nothing to say about BIND_PRINC_STATEID. Further,
>> I'm not sure how a SCSI READ or WRITE can check credentials as NFS
>> stateids are not presented to SCSI/iSCSI targets.
>>
>> Likewise, how would this impact a flexfile layout that targets
>> an NFSv3 DS?
>>
>> I think NFSD is checking stateids used for NFSv4 READ and WRITE as
>> needed, but help me understand how BIND_PRINC_STATEID is applicable
>> to pNFS block layouts...?
>>
>>
> 
> If you look at Section 15.2, then NFS4ERR_ACCESS is definitely listed
> as a valid error for LAYOUTGET (in fact, the very first in the list).
> 
> Furthermore, please see the following blanket statement in RFC8881,
> section 12.5.1.:
> 
>     "Layouts are provided to NFSv4.1 clients, and user access still
>     follows the rules of the protocol as if they did not exist."
> 
> While you can argue that the above language is not normative, because
> it doesn't use the official IETF "MUST" / "MUST NOT" /..., it clearly
> does declare an intention to ensure that pNFS not be allowed to weaken
> the protocol.
> 
> So my point would be that if LAYOUTGET is the only time where a proper
> credential check can occur, then it needs to happen there. Otherwise,
> your implementation is responsible for ensuring that it happens at some
> other time, in order to ensure that user access follows the rules of
> the protocol.

Absolutely agreed. The MDS needs to verify access before returning
the layout, for no other reason that the DS can't perform the same
verification.

Tom.