Received-SPF: pass (google.com: domain of linux-nfs+bounces-838-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 29 Dec 2023 18:29:00 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: trondmy@kernel.org, Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH] knfsd: fix the fallback implementation of the get_name
 export operation
Message-ID: <ZY9WPKwO2M6FXKpT@tissot.1015granger.net>
References: <20231228201510.985235-1-trondmy@kernel.org>
 <CAOQ4uxiCf=FWtZWw2uLRmfPvgSxsnmqZC6A+FQgQs=MBQwA30w@mail.gmail.com>
 <ZY7ZC9q8dGtoC2U/@tissot.1015granger.net>
 <CAOQ4uxh1VDPVq7a82HECtKuVwhMRLGe3pvL6TY6Xoobp=vaTTw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAOQ4uxh1VDPVq7a82HECtKuVwhMRLGe3pvL6TY6Xoobp=vaTTw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?VXN4WndxblBMYWFBYkEyTXNwWWRSUUx2TG96UlZzVkZ3Rzl1T1JjaFdXanJW?=
 =?utf-8?B?Mm9jWHVsVHJ2MFR6blVNOVozSmhDRFNFOHhtK2ZyK0g3a25VektNc1diQU1D?=
 =?utf-8?B?VDZLdkwzdnNPb1VWQXRqYjhlYlRMUm42TnJkUTNwR010QVdCVjRxVjc0RXJy?=
 =?utf-8?B?ZlZvdGVoOFRVSkY3SmdaOE5WKzRqMXYvOE1ZZWlaclp1UnVQTkRTNGJaSnBk?=
 =?utf-8?B?TnhxZDBmUXdKQjNvdE1aVEtDdDhZNDdiNG1MSG51WXhsN2k1a3VDT085c29r?=
 =?utf-8?B?WVZYL2lNd09qLzJVcnlURktzdldKQzB1dXdOK3R3VHpjUEdCY2JlbzREaW45?=
 =?utf-8?B?dEZkaTU3b1Z3bi9pZG83dEg4T1lCdk5tYVB0ZVdiQlhESGZLMTVDK0JXWDYw?=
 =?utf-8?B?SlBtakFIVmdvQlRQQjIxNWcyMDdPaGs5ZWdDMGRWUGVRWjdSN25yVC9zNXRL?=
 =?utf-8?B?bGJXaVFMZyttdUQ4YjM2a040dUtQRDNLNW90endlc1RyR2w3bmxYUDFyVXFV?=
 =?utf-8?B?czVBMENSeTdkY09hOHdkV3VZTFYwSDFLSWNkME9mMElOWlJ4MzBRdldlUlhW?=
 =?utf-8?B?ZGZSU0FMSUhGbDZMYytkODRGRHRMeENTaW14RVdyZzRqYUdHSW00U0NOMlZT?=
 =?utf-8?B?V0JodVZOOEpNUWkwWlRXQ3ZUem52NXB2ekJoZjNzY0pPMDF5eTBFT25lSDdH?=
 =?utf-8?B?M2JLRDg2dEtNZXVPQzliV0dMNm5EcGJnRVlVczIyQTBUR3dtTkM1c25Kemwz?=
 =?utf-8?B?TEJRTEZDOFFQdmZDdjlPd0lvZytzaERsVVI3WTA1N0VTUmNYbkhLOS9lMktp?=
 =?utf-8?B?KzMyZFZyYnlSbVM2bUlIS2JsR2x0UElFSUFsMFBOWU1FNGs0eWZFZ3VPcXlD?=
 =?utf-8?B?ZklRWDRxLy84MGtOUVVKMzE0QjcrYWRBZUMrNGZsU3VzSXlGd3poakNXRVl4?=
 =?utf-8?B?UEVKU1BHaUc0SDJOS0x5NDlYSWttQUNNVmU1Vm5mdHJPTWt6YSs5bXRzcTBx?=
 =?utf-8?B?bFVQN2hwTlkwaEpxTnBndjhaMjNOM1Y5NTRMTXNIU2kwalRLUTRsSkJDUEZV?=
 =?utf-8?B?M1hmVmJNV2E3N0ZSVXJhWmlqLzVEdHVuS2FIZDJLTTgzTVBhK1BjV0JzbHBZ?=
 =?utf-8?B?YjlheW9LQnBnSWY5bnRJanZ1MGxNTE1aRHFTZHlQM0FrK2ZBT0JpYUo1cFIx?=
 =?utf-8?B?bkljaUoxUW1rUEFrMHBhWENTT3pPVXh1eWU5L2Q0Ylo0Qnh4YUFCb2hZVWUx?=
 =?utf-8?B?OWt6Tks2eVdUOHlvQkRucFVlUkNDWVZjTHlUa0Z4MmhQMDRubnRkaE5VdDFF?=
 =?utf-8?B?QlhEL0dNandEMnlzSEw3QnNIVDVqSnNUODhYWHlIUnU5OW5QVExoN3kyUXk1?=
 =?utf-8?B?RDRqWG1CQnhMVGZhOURrNFgyaHBBVDRPQ0lxSkxFalVCYlpFUEVSV1h1QU4x?=
 =?utf-8?B?cHo4N1BvSVovalBiQk0zemtkZjBUUWRTOTNEU08vS2MwOTBUWEQyY3Nyd3dC?=
 =?utf-8?B?NmZwRTc3cHJicUxvbGVyYm43TGZTalE2TUMzU2dmUFRnZ04rWDRyT1dhd01T?=
 =?utf-8?B?ei9XU2xWNHRkVFNFL1hGZkVxTUFvNVRyeDJtUGpUdkluSTJqcGpodzNLaW4r?=
 =?utf-8?B?bmY0RWd0b1ZDbi92WVRMK0hNS3poM2hVWFdyb2Rva1ZIZ0ppelI4UWJhNGNj?=
 =?utf-8?B?T1lEME5aT2F2eGZnWkF5OWpCNmtpMzhSb2RlWU5jY2hsVTJqVG9WeWJ6ZTF1?=
 =?utf-8?B?eU5zZVN0cnBOdnJWVHc2SUtQZTV2c0NCakNnNnZ6RlVKTGJTeFRlcGFkWlFE?=
 =?utf-8?B?UEhrUUs2NW9YblpOY2VDbEw4RkdxQ1d4NHFEU3YxME9remtLZUxMYmtSS0Qz?=
 =?utf-8?B?RXhDMnJ0SWV0UDdXdm5VdHlDNXRHM0JZU2ZlZHlRN3h6RnZKajN6WEpyd3d0?=
 =?utf-8?B?NEdnWEM4N2xTc25UNXdtT0h0WFMxMGJjRGpRNUdkZStHRnBreFV4ZEhlUjRM?=
 =?utf-8?B?cjBSNEFONnUrdWxEZmMvZDlWelc4WC9FeEx3dkY2cTNBRHVXMExkc0QyNGNI?=
 =?utf-8?B?TGVnZk42SkxMd3FjUEZjOTBEbjJlRldPUVdFSmcwMWg5SURRcDhraVFRTm9x?=
 =?utf-8?B?Z2F5TDhxQVpxa25yUEdYenVOOTFlWW55R2ExUDNIaTRCMjA0VlpFcnNzMkZ4?=
 =?utf-8?B?enc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ZIljVHdIJEV57p69d0KThCZedc0XHEOsI47A6U7tqF1mjZ2Oa2PYyhyi516LcXSHyEornJrXGkzfDjM+y9hN6h+09WS37ptGM20fQw2btYugEZ4QKx36yO8YKC4pGwOGwUsavFvPfgpEqrD8+B0ZeKYyluCSbDgzit0xOXvSvJ5UBIjzIw7xNwvFLwjazTZP+OQb43B3yFbpVkSqYdNIuJoTHuqLeS+k/fz14OI5xdaRB85Dfs+JC+HDGu842jTIAoNTU3jlEjQynny1KQ7w0qZLKDQxZ281Oc9Wy6ung5Hw1/C7tJPvPgzA99mzpYTCVddnLZRZxdCgZkiN9rY1vCzezG//u8xSAIH2QA/7CizZnbB1ompq3Y5ZlIMxBCbw61tdveu52gttZMd2N7I5EG/NyHd3FaL3qmDRzmqC9O89aJbN8vBrgS6tTye0S/eSpa+h9iMHJmof5aan0chDcdUwFV1UmPpYmz3YlCSwSBBpj2KgZ+P1FRFtaYLYusBhxhgD6+vnaYjLwxNzoOr5+2lgdKuQcSm62JtmSO4hvWhipbvSuGzLDsd6EEpEYtyJ9TRWAa+0LHnXEYev+fkTfgpsco4/qVd/K29C+JJ5whg=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b8abb717-08f9-4510-63f0-08dc08c5f16b
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Dec 2023 23:29:03.3767
 (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: 2DNb4CP2tk+gcpb6lqVXGM4PTvDzJmQxRIGX4v5DZSlz8X815Kxt+WG/Cjga5MmhVOz+o1k6Re0S+6QFFrZ1rw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4767
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-29_12,2023-12-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 adultscore=0
 mlxlogscore=999 bulkscore=0 malwarescore=0 mlxscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312290189
X-Proofpoint-GUID: mAxZ8Tu2wkoj8Bp1a0Zu-RJYv_OxMqeL
X-Proofpoint-ORIG-GUID: mAxZ8Tu2wkoj8Bp1a0Zu-RJYv_OxMqeL

On Fri, Dec 29, 2023 at 07:44:20PM +0200, Amir Goldstein wrote:
> On Fri, Dec 29, 2023 at 4:35 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> > On Fri, Dec 29, 2023 at 07:46:54AM +0200, Amir Goldstein wrote:
> > > [CC: fsdevel, viro]
> >
> > Thanks for picking this up, Amir, and for copying viro/fsdevel. I
> > was planning to repost this next week when more folks are back, but
> > this works too.
> >
> > Trond, if you'd like, I can handle review changes if you don't have
> > time to follow up.
> >
> >
> > > On Thu, Dec 28, 2023 at 10:22 PM <trondmy@kernel.org> wrote:
> > > >
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > >
> > > > The fallback implementation for the get_name export operation uses
> > > > readdir() to try to match the inode number to a filename. That filename
> > > > is then used together with lookup_one() to produce a dentry.
> > > > A problem arises when we match the '.' or '..' entries, since that
> > > > causes lookup_one() to fail. This has sometimes been seen to occur for
> > > > filesystems that violate POSIX requirements around uniqueness of inode
> > > > numbers, something that is common for snapshot directories.
> > >
> > > Ouch. Nasty.
> > >
> > > Looks to me like the root cause is "filesystems that violate POSIX
> > > requirements around uniqueness of inode numbers".
> > > This violation can cause any of the parent's children to wrongly match
> > > get_name() not only '.' and '..' and fail the d_inode sanity check after
> > > lookup_one().
> > >
> > > I understand why this would be common with parent of snapshot dir,
> > > but the only fs that support snapshots that I know of (btrfs, bcachefs)
> > > do implement ->get_name(), so which filesystem did you encounter
> > > this behavior with? can it be fixed by implementing a snapshot
> > > aware ->get_name()?
> > >
> > > > This patch just ensures that we skip '.' and '..' rather than allowing a
> > > > match.
> > >
> > > I agree that skipping '.' and '..' makes sense, but...
> >
> > Does skipping '.' and '..' make sense for file systems that do
> 
> It makes sense because if the child's name in its parent would
> have been "." or ".." it would have been its own parent or its own
> grandparent (ELOOP situation).
> IOW, we can safely skip "." and "..", regardless of anything else.

This new comment:

+	/* Ignore the '.' and '..' entries */

then seems inadequate to explain why dot and dot-dot are now never
matched. Perhaps the function's documenting comment could expand on
this a little. I'll give it some thought.


> > indeed guarantee inode number uniqueness? Given your explanation
> > here, I'm wondering whether the generic get_name() function is the
> > right place to address the issue.

-- 
Chuck Lever