On Mon, Feb 17, 2020 at 4:02 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
>
> On 2020/2/17 22:52, Ilya Dryomov wrote:
> > On Mon, Feb 17, 2020 at 12:28 PM <xiubli@xxxxxxxxxx> wrote:
> >> From: Xiubo Li <xiubli@xxxxxxxxxx>
> >>
> >> For example, if dentry and inode is NULL, the log will be:
> >> ceph: lookup result=000000007a1ca695
> >> ceph: submit_request on 0000000041d5070e for inode 000000007a1ca695
> >>
> >> The will be confusing without checking the corresponding code carefully.
> >>
> >> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> >> ---
> >> fs/ceph/dir.c | 2 +-
> >> fs/ceph/mds_client.c | 6 +++++-
> >> 2 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> >> index ffeaff5bf211..245a262ec198 100644
> >> --- a/fs/ceph/dir.c
> >> +++ b/fs/ceph/dir.c
> >> @@ -798,7 +798,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
> >> err = ceph_handle_snapdir(req, dentry, err);
> >> dentry = ceph_finish_lookup(req, dentry, err);
> >> ceph_mdsc_put_request(req); /* will dput(dentry) */
> >> - dout("lookup result=%p\n", dentry);
> >> + dout("lookup result=%d\n", err);
> >> return dentry;
> >> }
> >>
> >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >> index b6aa357f7c61..e34f159d262b 100644
> >> --- a/fs/ceph/mds_client.c
> >> +++ b/fs/ceph/mds_client.c
> >> @@ -2772,7 +2772,11 @@ int ceph_mdsc_submit_request(struct ceph_mds_client *mdsc, struct inode *dir,
> >> ceph_get_cap_refs(ceph_inode(req->r_old_dentry_dir),
> >> CEPH_CAP_PIN);
> >>
> >> - dout("submit_request on %p for inode %p\n", req, dir);
> >> + if (dir)
> >> + dout("submit_request on %p for inode %p\n", req, dir);
> >> + else
> >> + dout("submit_request on %p\n", req);
> > Hi Xiubo,
> >
> > It's been this way for a couple of years now. There are a lot more
> > douts in libceph, ceph and rbd that are sometimes fed NULL pointers.
> > I don't think replacing them with conditionals is the way forward.
> >
> > I honestly don't know what security concern is addressed by hashing
> > NULL pointers, but that is what we have... Ultimately, douts are just
> > for developers, and when you find yourself having to chase individual
> > pointers, you usually have a large enough piece of log to figure out
> > what the NULL hash is.
>
> Hi Ilya
>
> For the ceph_lookup(). The dentry will be NULL(when the directory exists
> or -ENOENT) or ERR_PTR(-errno) in most cases here, it seems for the
> rename case it will be the old dentry returned.
>
> So today I was trying to debug and get some logs from it, the
> 000000007a1ca695 really confused me for a long time before I dig into
> the source code.
I was reacting to ceph_mdsc_submit_request() hunk. Feel free to tweak
ceph_lookup() or refactor it so that err is not threaded through three
different functions as Jeff suggested.
Thanks,
Ilya
