CEPH filesystem development
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Xiubo Li <xiubli@redhat.com>
Cc: Ilya Dryomov <idryomov@gmail.com>,
	Milind Changire <mchangir@redhat.com>,
	ceph-devel@vger.kernel.org
Subject: [RFC] odd games with d_find_alias() in ceph_unlink()
Date: Mon, 6 Jan 2025 04:57:24 +0000	[thread overview]
Message-ID: <20250106045724.GM1977892@ZenIV> (raw)

	In 2827badaf8162 "ceph: check the cephx mds auth access for async dirop"
you've added the following chunk:

+       dn = d_find_alias(dir);
+       if (!dn) {
+               try_async = false;
+       } else {
+               path = ceph_mdsc_build_path(mdsc, dn, &pathlen, &pathbase, 0);
+               if (IS_ERR(path)) {
+                       try_async = false;
+                       err = 0;
+               } else {
+                       err = ceph_mds_check_access(mdsc, path, MAY_WRITE);
+               }
+               ceph_mdsc_free_path(path, pathlen);
+               dput(dn);
+
+               /* For none EACCES cases will let the MDS do the mds auth check */
+               if (err == -EACCES) {
+                       return err;
+               } else if (err < 0) {
+                       try_async = false;
+                       err = 0;
+               }
+       }

What was that d_find_alias() for?  I mean, we are in ->unlink(); we'd
bloody well better have dentry->d_parent->d_inode == dir and dir locked
exclusive.  If that is _not_ true, we have a really big pile of problems
all over the place...  So your d_find_alias() is an obfuscated
	dn = dget(dentry->d_parent);
and even dget() is pointless here - dentry->d_parent is pinned, will stay
so and won't change.

What's wrong with simply

+       path = ceph_mdsc_build_path(mdsc, dentry->d_parent, &pathlen, &pathbase, 0);
+       if (IS_ERR(path)) {
+               try_async = false;
+               err = 0;
+       } else {
+               err = ceph_mds_check_access(mdsc, path, MAY_WRITE);
+       }
+       ceph_mdsc_free_path(path, pathlen);
+
+       /* For none EACCES cases will let the MDS do the mds auth check */
+       if (err == -EACCES) {
+               return err;
+       } else if (err < 0) {
+               try_async = false;
+               err = 0;
+       }

instead?  What am I missing here?  The same goes for the part in ceph_atomic_open()...

                 reply	other threads:[~2025-01-06  4:57 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250106045724.GM1977892@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=ceph-devel@vger.kernel.org \
    --cc=idryomov@gmail.com \
    --cc=mchangir@redhat.com \
    --cc=xiubli@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox