On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote:
> On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
---
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
