RE: [PATCH] ceph: is_root_ceph_dentry() cleanup

[Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>]

On Tue, 2025-01-28 at 03:07 +0000, Al Viro wrote:
> On Mon, Jan 27, 2025 at 05:10:23PM -0800, Viacheslav Dubeyko wrote:
> > From: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
> > 
> > This patch introduces CEPH_HIDDEN_DIR_NAME. It
> > declares name of the hidden directory .ceph in
> > the include/linux/ceph/ceph_fs.h instead of hiding
> > it in dir.c file. Also hardcoded length of the name
> > is changed on strlen(CEPH_HIDDEN_DIR_NAME).
> 
> Hmm...
> 
> Speaking of that area
> 	* how the hell could ceph_lookup() ever be called with dentry
> that is *NOT* negative?  VFS certainly won't do that; I'm not sure about
> ceph_handle_notrace_create(), but it doesn't look like that's possible
> without server being malicious (if it's possible at all).
> 
> 	* speaking of malicious servers, what happens if
> it gets CEPH_MDS_OP_LOOKUP and it returns a normal reply to positive
> lookup, but with cpu_to_le32(-ENOENT) shoved into head->result?
> 	AFAICS, ceph_handle_snapdir() will be called with dentry
> that is already made positive; results will not be pretty...

I assume that you imply this code:

	/* can we conclude ENOENT locally? */
	if (d_really_is_negative(dentry)) {
		struct ceph_inode_info *ci = ceph_inode(dir);
		struct ceph_dentry_info *di = ceph_dentry(dentry);

		spin_lock(&ci->i_ceph_lock);
		doutc(cl, " dir %llx.%llx flags are 0x%lx\n",
		      ceph_vinop(dir), ci->i_ceph_flags);
		if (strncmp(dentry->d_name.name,
			    fsc->mount_options->snapdir_name,
			    dentry->d_name.len) &&
		    !is_root_ceph_dentry(dir, dentry) &&
		    ceph_test_mount_opt(fsc, DCACHE) &&
		    __ceph_dir_is_complete(ci) &&
		    __ceph_caps_issued_mask_metric(ci, CEPH_CAP_FILE_SHARED,
1)) {
			__ceph_touch_fmode(ci, mdsc, CEPH_FILE_MODE_RD);
			spin_unlock(&ci->i_ceph_lock);
			doutc(cl, " dir %llx.%llx complete, -ENOENT\n",
			      ceph_vinop(dir));
			d_add(dentry, NULL);
			di->lease_shared_gen = atomic_read(&ci->i_shared_gen);
			return NULL;
		}
		spin_unlock(&ci->i_ceph_lock);
	}

Am I correct? So, how can we rework this code if it's wrong? What is your
vision? Do you mean that it's dead code? How can we check it?

Thanks,
Slava.