From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values Date: Mon, 13 Apr 2020 07:15:52 -0400 Message-ID: References: <20200408142125.52908-1-jlayton@kernel.org> <20200408142125.52908-2-jlayton@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.kernel.org ([198.145.29.99]:48654 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728096AbgDMLPy (ORCPT ); Mon, 13 Apr 2020 07:15:54 -0400 In-Reply-To: Sender: ceph-devel-owner@vger.kernel.org List-ID: To: Ilya Dryomov Cc: Ceph Development , Dan Carpenter , Sage Weil On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote: > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton wrote: > > This makes the error handling simpler in some callers, and fixes a > > couple of bugs in the new async dirops callback code. > > > > Reported-by: Dan Carpenter > > Signed-off-by: Jeff Layton > > --- > > fs/ceph/debugfs.c | 4 ---- > > fs/ceph/mds_client.c | 6 ++---- > > fs/ceph/mds_client.h | 2 +- > > 3 files changed, 3 insertions(+), 9 deletions(-) > > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c > > index eebbce7c3b0c..3a198e40f100 100644 > > --- a/fs/ceph/debugfs.c > > +++ b/fs/ceph/debugfs.c > > @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p) > > } else if (req->r_dentry) { > > path = ceph_mdsc_build_path(req->r_dentry, &pathlen, > > &pathbase, 0); > > - if (IS_ERR(path)) > > - path = NULL; > > spin_lock(&req->r_dentry->d_lock); > > seq_printf(s, " #%llx/%pd (%s)", > > Hi Jeff, > > This ends up attempting to print an IS_ERR pointer as %s. > > > ceph_ino(d_inode(req->r_dentry->d_parent)), > > @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p) > > if (req->r_old_dentry) { > > path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen, > > &pathbase, 0); > > - if (IS_ERR(path)) > > - path = NULL; > > spin_lock(&req->r_old_dentry->d_lock); > > seq_printf(s, " #%llx/%pd (%s)", > > Ditto. > > It looks like in newer kernels printf copes with this and outputs > "(efault)". But anything older than 5.2 will crash. > > Further, the code looks weird because ceph_mdsc_build_path() doesn't > return NULL, but path is tested for NULL in the call to seq_printf(). > > Why not just follow the same approach as existing mdsc_show()? It > makes it clear that the error is handled and where the NULL pointer > comes from. This kind of "don't handle errors and rely on everything > else being able to bail" approach is very fragile and hard to audit. > I don't see a problem with having a "free" routine ignore IS_ERR values just like it does NULL values. How about I just trim off the other deltas in this patch? Something like this? ------------------8<----------------- [PATCH] ceph: have ceph_mdsc_free_path ignore ERR_PTR values This fixes a couple of bugs in the new async dirops callback code. Reported-by: Dan Carpenter Signed-off-by: Jeff Layton --- fs/ceph/mds_client.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h index 1b40f30e0a8e..43111e408fa2 100644 --- a/fs/ceph/mds_client.h +++ b/fs/ceph/mds_client.h @@ -531,7 +531,7 @@ extern void ceph_mdsc_pre_umount(struct ceph_mds_client *mdsc); static inline void ceph_mdsc_free_path(char *path, int len) { - if (path) + if (!IS_ERR_OR_NULL(path)) __putname(path - (PATH_MAX - 1 - len)); } -- 2.25.2