On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@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?
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@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));
> }
Thanks,
Ilya
