* [PATCH 0/2] ceph: fix error handling bugs in async dirops callbacks @ 2020-04-08 14:21 Jeff Layton 2020-04-08 14:21 ` [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values Jeff Layton 2020-04-08 14:21 ` [PATCH 2/2] ceph: initialize base and pathlen variables in async dirops cb's Jeff Layton 0 siblings, 2 replies; 11+ messages in thread From: Jeff Layton @ 2020-04-08 14:21 UTC (permalink / raw) To: ceph-devel; +Cc: idryomov, dan.carpenter, sage This fixes a couple of problems that Dan spotted with a static checker. Additionally the first patch also simplifies some of the error handling in other callers. Originally, I was going to squash these into the earlier patches that introduced those bugs, but this is new code that isn't enabled by default and Ilya was planning to a PR soon, so it's probably best to just handle them as follow-on fixes to what's currently in ceph-client/master. Jeff Layton (2): ceph: have ceph_mdsc_free_path ignore ERR_PTR values ceph: initialize base and pathlen variables in async dirops cb's fs/ceph/debugfs.c | 4 ---- fs/ceph/dir.c | 4 ++-- fs/ceph/file.c | 4 ++-- fs/ceph/mds_client.c | 6 ++---- fs/ceph/mds_client.h | 2 +- 5 files changed, 7 insertions(+), 13 deletions(-) -- 2.25.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-08 14:21 [PATCH 0/2] ceph: fix error handling bugs in async dirops callbacks Jeff Layton @ 2020-04-08 14:21 ` Jeff Layton 2020-04-13 8:09 ` Ilya Dryomov 2020-04-08 14:21 ` [PATCH 2/2] ceph: initialize base and pathlen variables in async dirops cb's Jeff Layton 1 sibling, 1 reply; 11+ messages in thread From: Jeff Layton @ 2020-04-08 14:21 UTC (permalink / raw) To: ceph-devel; +Cc: idryomov, dan.carpenter, sage 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 <dan.carpenter@oracle.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- 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)", 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)", req->r_old_dentry_dir ? diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index a8a5b98148ec..e25ee9fe0ee8 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2535,11 +2535,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc, msg->hdr.data_off = cpu_to_le16(0); out_free2: - if (freepath2) - ceph_mdsc_free_path((char *)path2, pathlen2); + ceph_mdsc_free_path((char *)path2, pathlen2); out_free1: - if (freepath1) - ceph_mdsc_free_path((char *)path1, pathlen1); + ceph_mdsc_free_path((char *)path1, pathlen1); out: return msg; } 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-08 14:21 ` [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values Jeff Layton @ 2020-04-13 8:09 ` Ilya Dryomov 2020-04-13 11:15 ` Jeff Layton 0 siblings, 1 reply; 11+ messages in thread From: Ilya Dryomov @ 2020-04-13 8:09 UTC (permalink / raw) To: Jeff Layton; +Cc: Ceph Development, Dan Carpenter, Sage Weil On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> 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 <dan.carpenter@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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. Thanks, Ilya ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 8:09 ` Ilya Dryomov @ 2020-04-13 11:15 ` Jeff Layton 2020-04-13 12:35 ` Ilya Dryomov 0 siblings, 1 reply; 11+ messages in thread From: Jeff Layton @ 2020-04-13 11:15 UTC (permalink / raw) 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 <jlayton@kernel.org> 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 <dan.carpenter@oracle.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > 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 <dan.carpenter@oracle.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 11:15 ` Jeff Layton @ 2020-04-13 12:35 ` Ilya Dryomov 2020-04-13 13:23 ` Jeff Layton 0 siblings, 1 reply; 11+ messages in thread From: Ilya Dryomov @ 2020-04-13 12:35 UTC (permalink / raw) To: Jeff Layton; +Cc: Ceph Development, Dan Carpenter, Sage Weil On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote: > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> 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 <dan.carpenter@oracle.com> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > 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? I think it encourages fragile code. Less so than functions that return pointer, NULL or IS_ERR pointer, but still. You yourself almost fell into one of these traps while editing debugfs.c ;) That said, I won't stand in the way here. If you trim off everything else, merge it together with the other patch so that it is introduced along with the two users. > > ------------------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 <dan.carpenter@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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)); > } Thanks, Ilya ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 12:35 ` Ilya Dryomov @ 2020-04-13 13:23 ` Jeff Layton 2020-04-13 15:48 ` Ilya Dryomov 2020-04-13 19:41 ` Dan Carpenter 0 siblings, 2 replies; 11+ messages in thread From: Jeff Layton @ 2020-04-13 13:23 UTC (permalink / raw) To: Ilya Dryomov; +Cc: Ceph Development, Dan Carpenter, Sage Weil On Mon, 2020-04-13 at 14:35 +0200, Ilya Dryomov wrote: > On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote: > > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> 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 <dan.carpenter@oracle.com> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > 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? > > I think it encourages fragile code. Less so than functions that > return pointer, NULL or IS_ERR pointer, but still. You yourself > almost fell into one of these traps while editing debugfs.c ;) > We'll have to agree to disagree here. Having a free routine ignore ERR_PTR values seems perfectly reasonable to me. > That said, I won't stand in the way here. If you trim off everything > else, merge it together with the other patch so that it is introduced > along with the two users. > Do you mean that I should merge it with this? ceph: initialize base and pathlen variables in async dirops cb's I'm not sure I see the point in that. In any case, I pushed the updated patch into ceph-client/testing so let me know if you see anything else amiss with it. Thanks, -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 13:23 ` Jeff Layton @ 2020-04-13 15:48 ` Ilya Dryomov 2020-04-13 16:03 ` Jeff Layton 2020-04-13 19:41 ` Dan Carpenter 1 sibling, 1 reply; 11+ messages in thread From: Ilya Dryomov @ 2020-04-13 15:48 UTC (permalink / raw) To: Jeff Layton; +Cc: Ceph Development, Dan Carpenter, Sage Weil On Mon, Apr 13, 2020 at 3:23 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2020-04-13 at 14:35 +0200, Ilya Dryomov wrote: > > On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote: > > > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote: > > > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> 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 <dan.carpenter@oracle.com> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > --- > > > > > 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? > > > > I think it encourages fragile code. Less so than functions that > > return pointer, NULL or IS_ERR pointer, but still. You yourself > > almost fell into one of these traps while editing debugfs.c ;) > > > > We'll have to agree to disagree here. Having a free routine ignore > ERR_PTR values seems perfectly reasonable to me. > > > That said, I won't stand in the way here. If you trim off everything > > else, merge it together with the other patch so that it is introduced > > along with the two users. > > > > Do you mean that I should merge it with this? > > ceph: initialize base and pathlen variables in async dirops cb's > > I'm not sure I see the point in that. Yes, because ultimately this is all about those two pr_warn messages. path is built just to be printed, base and pathlen are a part of that. ceph_mdsc_free_path() didn't need to handle IS_ERR pointers before so it should be a single patch. Thanks, Ilya ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 15:48 ` Ilya Dryomov @ 2020-04-13 16:03 ` Jeff Layton 0 siblings, 0 replies; 11+ messages in thread From: Jeff Layton @ 2020-04-13 16:03 UTC (permalink / raw) To: Ilya Dryomov; +Cc: Ceph Development, Dan Carpenter, Sage Weil On Mon, 2020-04-13 at 17:48 +0200, Ilya Dryomov wrote: > On Mon, Apr 13, 2020 at 3:23 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2020-04-13 at 14:35 +0200, Ilya Dryomov wrote: > > > On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote: > > > > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> 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 <dan.carpenter@oracle.com> > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > --- > > > > > > 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? > > > > > > I think it encourages fragile code. Less so than functions that > > > return pointer, NULL or IS_ERR pointer, but still. You yourself > > > almost fell into one of these traps while editing debugfs.c ;) > > > > > > > We'll have to agree to disagree here. Having a free routine ignore > > ERR_PTR values seems perfectly reasonable to me. > > > > > That said, I won't stand in the way here. If you trim off everything > > > else, merge it together with the other patch so that it is introduced > > > along with the two users. > > > > > > > Do you mean that I should merge it with this? > > > > ceph: initialize base and pathlen variables in async dirops cb's > > > > I'm not sure I see the point in that. > > Yes, because ultimately this is all about those two pr_warn messages. > path is built just to be printed, base and pathlen are a part of that. > ceph_mdsc_free_path() didn't need to handle IS_ERR pointers before so > it should be a single patch. > Ok, done, and cleaned up the changelog. I won't bother re-posting since it's a fairly trivial squashing of two patches. -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 13:23 ` Jeff Layton 2020-04-13 15:48 ` Ilya Dryomov @ 2020-04-13 19:41 ` Dan Carpenter 2020-04-14 10:43 ` Jeff Layton 1 sibling, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2020-04-13 19:41 UTC (permalink / raw) To: Jeff Layton; +Cc: Ilya Dryomov, Ceph Development, Sage Weil On Mon, Apr 13, 2020 at 09:23:22AM -0400, Jeff Layton wrote: > > > 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? > > > > I think it encourages fragile code. Less so than functions that > > return pointer, NULL or IS_ERR pointer, but still. You yourself > > almost fell into one of these traps while editing debugfs.c ;) > > > > We'll have to agree to disagree here. Having a free routine ignore > ERR_PTR values seems perfectly reasonable to me. Freeing things which haven't been allocated is a constant source bugs. err: kfree(foo->bar); kfree(foo); Oops... "foo" wasn't allocated so the first line will crash. Every other day someone commits code like that. regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values 2020-04-13 19:41 ` Dan Carpenter @ 2020-04-14 10:43 ` Jeff Layton 0 siblings, 0 replies; 11+ messages in thread From: Jeff Layton @ 2020-04-14 10:43 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ilya Dryomov, Ceph Development, Sage Weil On Mon, 2020-04-13 at 22:41 +0300, Dan Carpenter wrote: > On Mon, Apr 13, 2020 at 09:23:22AM -0400, Jeff Layton wrote: > > > > 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? > > > > > > I think it encourages fragile code. Less so than functions that > > > return pointer, NULL or IS_ERR pointer, but still. You yourself > > > almost fell into one of these traps while editing debugfs.c ;) > > > > > > > We'll have to agree to disagree here. Having a free routine ignore > > ERR_PTR values seems perfectly reasonable to me. > > Freeing things which haven't been allocated is a constant source bugs. > > err: > kfree(foo->bar); > kfree(foo); > > Oops... "foo" wasn't allocated so the first line will crash. Every > other day someone commits code like that. > It's not clear to me whether you're advocating for Ilya's approach or mine (neither? both?). Which approach do you think is best? FWIW, my rationale for doing it this way is that the "allocator" for ceph_mdsc_free_path is ceph_mdsc_build_path. That routine returns an ERR_PTR value on failure, not a NULL pointer, so it makes sense to me to have the free routine accept and ignore those values. I don't quite follow the rationale that that encourages fragile code. -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] ceph: initialize base and pathlen variables in async dirops cb's 2020-04-08 14:21 [PATCH 0/2] ceph: fix error handling bugs in async dirops callbacks Jeff Layton 2020-04-08 14:21 ` [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values Jeff Layton @ 2020-04-08 14:21 ` Jeff Layton 1 sibling, 0 replies; 11+ messages in thread From: Jeff Layton @ 2020-04-08 14:21 UTC (permalink / raw) To: ceph-devel; +Cc: idryomov, dan.carpenter, sage Ensure that the pr_warn messages look sane even if ceph_mdsc_build_path fails. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/dir.c | 4 ++-- fs/ceph/file.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 10d528a455d5..93476d447a4b 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1057,8 +1057,8 @@ static void ceph_async_unlink_cb(struct ceph_mds_client *mdsc, /* If op failed, mark everyone involved for errors */ if (result) { - int pathlen; - u64 base; + int pathlen = 0; + u64 base = 0; char *path = ceph_mdsc_build_path(req->r_dentry, &pathlen, &base, 0); diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 3a1bd13de84f..160644ddaeed 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -529,8 +529,8 @@ static void ceph_async_create_cb(struct ceph_mds_client *mdsc, if (result) { struct dentry *dentry = req->r_dentry; - int pathlen; - u64 base; + int pathlen = 0; + u64 base = 0; char *path = ceph_mdsc_build_path(req->r_dentry, &pathlen, &base, 0); -- 2.25.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-14 10:43 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-08 14:21 [PATCH 0/2] ceph: fix error handling bugs in async dirops callbacks Jeff Layton 2020-04-08 14:21 ` [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values Jeff Layton 2020-04-13 8:09 ` Ilya Dryomov 2020-04-13 11:15 ` Jeff Layton 2020-04-13 12:35 ` Ilya Dryomov 2020-04-13 13:23 ` Jeff Layton 2020-04-13 15:48 ` Ilya Dryomov 2020-04-13 16:03 ` Jeff Layton 2020-04-13 19:41 ` Dan Carpenter 2020-04-14 10:43 ` Jeff Layton 2020-04-08 14:21 ` [PATCH 2/2] ceph: initialize base and pathlen variables in async dirops cb's Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.