| Message ID | YBjne8A1gn0mvQtT@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: Fix an Oops in error handling | expand |
On Tue, Feb 2, 2021 at 6:47 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The "req" pointer is an error pointer and not NULL so this check needs > to be fixed. > > Fixes: 1cf7fdf52d5a ("ceph: convert readpage to fscache read helper") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > fs/ceph/addr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c > index 5eec6f66fe52..fb0238a4d34f 100644 > --- a/fs/ceph/addr.c > +++ b/fs/ceph/addr.c > @@ -273,7 +273,7 @@ static void ceph_netfs_issue_op(struct netfs_read_subrequest *subreq) > if (err) > iput(inode); > out: > - if (req) > + if (!IS_ERR_OR_NULL(req)) > ceph_osdc_put_request(req); > if (err) > netfs_subreq_terminated(subreq, err); Hi Dan, I think a better fix would be to set req to NULL in the offending IS_ERR branch since ceph_osdc_new_request() never returns NULL or use two separate goto labels. While at it, the initialization of req and the check on req before calling ceph_osdc_put_request() are redundant. Thanks, Ilya
On Tue, 2021-02-02 at 08:47 +0300, Dan Carpenter wrote: > The "req" pointer is an error pointer and not NULL so this check needs > to be fixed. > > Fixes: 1cf7fdf52d5a ("ceph: convert readpage to fscache read helper") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > fs/ceph/addr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c > index 5eec6f66fe52..fb0238a4d34f 100644 > --- a/fs/ceph/addr.c > +++ b/fs/ceph/addr.c > @@ -273,7 +273,7 @@ static void ceph_netfs_issue_op(struct netfs_read_subrequest *subreq) > if (err) > iput(inode); > out: > - if (req) > + if (!IS_ERR_OR_NULL(req)) > ceph_osdc_put_request(req); > if (err) > netfs_subreq_terminated(subreq, err); Good catch. David, could you take this into your fscache-next branch? Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Tue, Feb 02, 2021 at 07:37:57AM -0500, Jeff Layton wrote: > On Tue, 2021-02-02 at 08:47 +0300, Dan Carpenter wrote: > > The "req" pointer is an error pointer and not NULL so this check needs > > to be fixed. > > > > Fixes: 1cf7fdf52d5a ("ceph: convert readpage to fscache read helper") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > fs/ceph/addr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c > > index 5eec6f66fe52..fb0238a4d34f 100644 > > --- a/fs/ceph/addr.c > > +++ b/fs/ceph/addr.c > > @@ -273,7 +273,7 @@ static void ceph_netfs_issue_op(struct netfs_read_subrequest *subreq) > > if (err) > > iput(inode); > > out: > > - if (req) > > + if (!IS_ERR_OR_NULL(req)) > > ceph_osdc_put_request(req); > > if (err) > > netfs_subreq_terminated(subreq, err); > > Good catch. > > David, could you take this into your fscache-next branch? > > Reviewed-by: Jeff Layton <jlayton@kernel.org> Jeff sent a different fix for this. Let's just apply his instead. regards, dan carpenter
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c index 5eec6f66fe52..fb0238a4d34f 100644 --- a/fs/ceph/addr.c +++ b/fs/ceph/addr.c @@ -273,7 +273,7 @@ static void ceph_netfs_issue_op(struct netfs_read_subrequest *subreq) if (err) iput(inode); out: - if (req) + if (!IS_ERR_OR_NULL(req)) ceph_osdc_put_request(req); if (err) netfs_subreq_terminated(subreq, err);
The "req" pointer is an error pointer and not NULL so this check needs to be fixed. Fixes: 1cf7fdf52d5a ("ceph: convert readpage to fscache read helper") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- fs/ceph/addr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)