Received-SPF: pass (google.com: domain of linux-nfs+bounces-3638-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 10 Jun 2024 13:43:44 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "cel@kernel.org" <cel@kernel.org>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "tom@talpey.com" <tom@talpey.com>, "hch@lst.de" <hch@lst.de>,
        "dai.ngo@oracle.com" <dai.ngo@oracle.com>,
        "jlayton@kernel.org" <jlayton@kernel.org>,
        "neilb@suse.de" <neilb@suse.de>, "kolga@netapp.com" <kolga@netapp.com>
Subject: Re: [RFC PATCH] NFSD: Support write delegations for pNFS LAYOUT
 operations
Message-ID: <Zmc7UOSMmeGcqkBa@tissot.1015granger.net>
References: <20240610150448.2377-2-cel@kernel.org>
 <30924327aaee17182a83e18bc86e6a27a19d88ab.camel@hammerspace.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <30924327aaee17182a83e18bc86e6a27a19d88ab.camel@hammerspace.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?pp0jkx3F58jLnFfaYAE2I+745rgh9p69zTdUgm4T5LopUaKJl19mVaWIGk7O?=
 =?us-ascii?Q?Ugpvn9Yh2Qjm5+gHrYbU8qeeZbdVWgXWDiw4xjUOCgyw3H3mt1udEGeqOe9U?=
 =?us-ascii?Q?Ke0XU+/b9uXdwdcHZX51JHgMTehyLDK42eQvMox09MhwT3lCZpRUiTW/gBxj?=
 =?us-ascii?Q?EFOcUjQNIVUhAMKofWmNPtXugf3ix2jHdidx8wYevWtgIARh4epvqPe+qG+f?=
 =?us-ascii?Q?EAQfazkVePDSHRuGr9/rjCXJglcM+q/fpmIq7+27VzHjk3udxSK5z1xRzoPl?=
 =?us-ascii?Q?DbRI3awMisI84xdOiN06NijXv65I9eTUjo7U+J7nwcTtctO+rmVriqg5Oun2?=
 =?us-ascii?Q?VlVek7YMG8wKXUCyFRXwMFy7qsHJomXVF9rdUX45AlLH1hMLLdmyBc0rjcd8?=
 =?us-ascii?Q?eCA1cUkHyo5pauILuTx58/83wuuDbU2vPH9yFBI5Bf1Xd8bMX5RZHL8hZwtw?=
 =?us-ascii?Q?5qF6LSghIwzckv8Zg5VksbnzM9hAssKx0uMWB+ZclpVGZfPODWFIuFUO9Pus?=
 =?us-ascii?Q?GSPkatck7OVv1lORRWrTVRiMVJsiLvXkmsy/Iq+5XZ50JjO1m4ugJETcd+Gu?=
 =?us-ascii?Q?Oy0nMLCA1TODHnz3ZjU4KFeWlJ8xjEJ8i4VZAdAChYL5E+ewlcbrJm2dvcU1?=
 =?us-ascii?Q?SKhCkXL0r5VzOD0VJr2gbEkHDvKzFSkW7V/oO6viCTk5yVMCUpu7ygdu29Yj?=
 =?us-ascii?Q?UU7B82LXZU8Gjn92Li7KmsBGi6vXpaT/3v4sb+wS40fMc0fJOIQ6HtA/WMLO?=
 =?us-ascii?Q?QCs9q1DYuUmVSmM+UlGAlf/mwPfsZ7Et8SHKLlemU4K4YuVozCjJh/pK4z6A?=
 =?us-ascii?Q?MuOF2286uNB5Jb/JJ4bmu9RT/NtVWmcTJvs2bkZzm4qjEdSrMCfGCnvS+VhD?=
 =?us-ascii?Q?MCOLYW4aQPUwLQwDdJZtsFwmMCjuOL0LG76XpY9Pt3FvhNzD+tJ7Iy9i+ySd?=
 =?us-ascii?Q?p/9xtL6To5kvZMjDbMVIgasstC65J8Ixgun8UPqzFuNzVoUogU7UiHu+qtZE?=
 =?us-ascii?Q?u+g7SaUDcAgdHd5yutij21FPri5MeFV9yzjpy6zRtjR2BuPALS90fGFLUrn2?=
 =?us-ascii?Q?tkWZACBH6/WrV9Bk4/fKeADSgGKD57aAkHjxj6ZdPdl5lqzMvuI/d65WRxcK?=
 =?us-ascii?Q?krn+9mUXVzX8xDXRlpgYEGFA6BRO6dHFDjdbiKxGj4dcj/GZ/7X0Yx22UVKi?=
 =?us-ascii?Q?bTNPfily1SpOjT9BLwUe4MvHGUHxVK0hpBXOoU0NUVoukIWZJMoNFEGKPWgW?=
 =?us-ascii?Q?t2gh45Y6ySk1lLLI7BFbigQrofLvxcRSRkY5ufa9dIqI1H+PebUrdWJsqBmY?=
 =?us-ascii?Q?KaUVnluaRZScxG9yGo2lE6U4Z3AoqgpUR3iThdbZ/eGhguMSsyL3P/ZNOwyQ?=
 =?us-ascii?Q?wpeGZAAU8nQjy30CXqbAmfAF68CC0kBCKgXwjUNEXVuieDgKE+7f1TXULduq?=
 =?us-ascii?Q?tVTH+gVgb72oT2/Ju3y38K+xHRGpVxDuE7nr5qiCpRxCYXbSQW7jFXcJ8S9B?=
 =?us-ascii?Q?faoBPpbbRKjutMOZXXVMhXBNZYTWTRRXBaqK4Kkinu6GJnYPZYZxmsyBfcwZ?=
 =?us-ascii?Q?2tZeeOXyb68skJk2ZOVK6WJaavwUBRZ+MQqealQIHevmdbE88S7OH0xxPyr1?=
 =?us-ascii?Q?GQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	wwr881zHfOwxHdGVcqLSynOD/YPbMapP7AX7F4M8MKliHbp09KICkvCzSn5VE1SYMfi0J/CW8Cv4p9ziBVZkbvtCR8P6z4Z9wIkzG/RyNN9oq7aN2VhyS9gIJ65y9lu+8b0X9u0dNjg6mnn5wkqT42iAc3EaeZR6aN2bakW676Przj5D4GqsU5Kx/gDG4NENPY48zyV7qa8MerKGp+CbuLtutiTxEeLhV1BRSnZ4MC5e1TBq83yWW9bJ7QBvVe5CjXmdCWnWzqvUUfgnTd5QwlBsYQH8FiPGdxSjNQrtZ48vQkg63v57NSP39QZQG+k6JVVPGXdyd8AKOv7AVuUT8tKwy4ffEfKvaRHNiF4GcQVNHOkTKrCr32J/BXwrTtL56mWf5lJ7hQTtt0RSdVvzgZc1jS/VfuSuj9vfXDwSmIyk2uNqDB2q221kzfg5eRUbwltKh4ejHo5jYfSFZDfd6V2+BbAzB+DoFPpN2pyHnQCQVkb+PlAC+sL3Vbu9l9hsJF4ExTI0N/2c7IYih6/wPUaEp/uTr3GnEskbDtMwEidqStn9RSBzI3HXywWLvLgix3Ju/ABE4AneVXnT7jAYceUIDAdZpsZUZx/ht5OXSeg=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 99cef2e1-0868-4ee3-f283-08dc8974e22c
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2024 17:43:48.5349
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2byL+nOhWzWc/7VrYYgSV88UzYAlxlss7a39Zp30pS4WKX8tO95jdv/jT3+/HizP8xFuRRhajIRHZ+mK0lDsdQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4816
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-10_04,2024-06-10_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 bulkscore=0 malwarescore=0 spamscore=0 phishscore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406100133
X-Proofpoint-GUID: 8ZWRrGZuOOxULgqk6N0cObpHnLB1k6DQ
X-Proofpoint-ORIG-GUID: 8ZWRrGZuOOxULgqk6N0cObpHnLB1k6DQ

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...?


-- 
Chuck Lever