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.
Thanks,
Ilya
