ceph: fix cap ref leak via netfs init_request
Subsystems (2)
PATCH 1/1 ceph: fix cap ref leak via netfs init_request
From: Patrick Donnelly <[email protected]>
Log recovered from a user's cluster:
<7>[ 5413.970692] ceph: get_cap_refs 00000000958c114b ret 1 got Fr
<7>[ 5413.970695] ceph: start_read 00000000958c114b, no cache cap
...
<7>[ 5473.934609] ceph: my wanted = Fr, used = Fr, dirty -
<7>[ 5473.934616] ceph: revocation: pAsLsXsFr -> pAsLsXs (revoking Fr)
<7>[ 5473.934632] ceph: __ceph_caps_issued 00000000958c114b cap 00000000f7784259 issued pAsLsXs
<7>[ 5473.934638] ceph: check_caps 10000000e68.fffffffffffffffe file_want - used Fr dirty - flushing - issued pAsLsXs revoking Fr retain pAsLsXsFsr AUTHONLY NOINVAL FLUSH_FORCEThe MDS subsequently complains that the kernel client is late releasing caps.
Approximately, a series of changes to this code by the three commits cited below resulted in subtle resource cleanup to be missed. The main culprit is the change in error handling in 2d31604 which meant that a failure in init_request would no longer cause cleanup to be called. That would prevent the ceph_put_cap_refs which would cleanup the leaked cap ref.
--- fs/ceph/addr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
fs/ceph/addr.c +4 -1
@@ -489,8 +489,11 @@ static int ceph_init_request(struct netfs_io_request *rreq, struct file *file) rreq->io_streams[0 ].sreq_max_len = fsc->mount_options->rsize; out: -if (ret <0 ) +if (ret <0 ) { +if (got) +ceph_put_cap_refs (ceph_inode (inode), got);kfree (priv); + }return ret; }
base-commit: e32cde8d2bd7d251a8f9b434143977ddf13dcec6
Signature · 5 lines
Patrick Donnelly, Ph.D. He / Him / His Red Hat Partner Engineer IBM, Inc. GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Quoted 49 lines · Patrick Donnelly
From: Patrick Donnelly <[email protected]>
Log recovered from a user's cluster:
<7>[ 5413.970692] ceph: get_cap_refs 00000000958c114b ret 1 got Fr <7>[ 5413.970695] ceph: start_read 00000000958c114b, no cache cap ... <7>[ 5473.934609] ceph: my wanted = Fr, used = Fr, dirty - <7>[ 5473.934616] ceph: revocation: pAsLsXsFr -> pAsLsXs (revoking Fr) <7>[ 5473.934632] ceph: __ceph_caps_issued 00000000958c114b cap 00000000f7784259 issued pAsLsXs <7>[ 5473.934638] ceph: check_caps 10000000e68.fffffffffffffffe file_want - used Fr dirty - flushing - issued pAsLsXs revoking Fr retain pAsLsXsFsr AUTHONLY NOINVAL FLUSH_FORCEThe MDS subsequently complains that the kernel client is late releasing caps.
Approximately, a series of changes to this code by the three commits cited below resulted in subtle resource cleanup to be missed. The main culprit is the change in error handling in 2d31604 which meant that a failure in
init_requestwould no longer cause cleanup to be called. That would prevent theceph_put_cap_refswhich would cleanup the leaked cap ref.Fixes 49870056005ca9387e5ee31451991491f99cc45f ("ceph: convert ceph_readpages to ceph_readahead")Fixes 2de160417315b8d64455fe03e9bb7d3308ac3281 ("netfs: Change ->init_request() to return an error code")Fixes a5c9dc4451394b2854493944dcc0ff71af9705a3 ("ceph: Make ceph_init_request() check caps on readahead")Signed-off-by Patrick Donnelly <[email protected]>--- fs/ceph/addr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/ceph/addr.c +4
-1@@ -489,8 +489,11 @@ static int ceph_init_request(struct netfs_io_request *rreq, struct file *file) rreq->io_streams[0 ].sreq_max_len = fsc->mount_options->rsize; out: -if (ret <0 ) +if (ret <0 ) { +if (got) +ceph_put_cap_refs (ceph_inode (inode), got);kfree (priv); + }return ret; }
Quoted 8 lines
base-commit:
e32cde8d2bd7d251a8f9b434143977ddf13dcec6--
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Applied.
Thanks,
Ilya
Quoted 18 lines · Patrick Donnelly
Log recovered from a user's cluster:
<7>[ 5413.970692] ceph: get_cap_refs 00000000958c114b ret 1 got Fr <7>[ 5413.970695] ceph: start_read 00000000958c114b, no cache cap ... <7>[ 5473.934609] ceph: my wanted = Fr, used = Fr, dirty - <7>[ 5473.934616] ceph: revocation: pAsLsXsFr -> pAsLsXs (revoking Fr) <7>[ 5473.934632] ceph: __ceph_caps_issued 00000000958c114b cap 00000000f7784259 issued pAsLsXs <7>[ 5473.934638] ceph: check_caps 10000000e68.fffffffffffffffe file_want - used Fr dirty - flushing - issued pAsLsXs revoking Fr retain pAsLsXsFsr AUTHONLY NOINVAL FLUSH_FORCEThe MDS subsequently complains that the kernel client is late releasing caps.
Approximately, a series of changes to this code by the three commits cited below resulted in subtle resource cleanup to be missed. The main culprit is the change in error handling in 2d31604 which meant that a failure in
init_requestwould no longer cause cleanup to be called. That would prevent theceph_put_cap_refswhich would cleanup the leaked cap ref.
Quoted 4 lines
Fixes 49870056005ca9387e5ee31451991491f99cc45f ("ceph: convert ceph_readpages to ceph_readahead")Fixes 2de160417315b8d64455fe03e9bb7d3308ac3281 ("netfs: Change ->init_request() to return an error code")Fixes a5c9dc4451394b2854493944dcc0ff71af9705a3 ("ceph: Make ceph_init_request() check caps on readahead")
Note that you only need the first 12 digits of the SHA1 sum.
Quoted 2 lines
Signed-off-by Patrick Donnelly <[email protected]>
Hi Ilya,
Are you going to pick this up, or should I ask Christian to take it through the vfs tree?
David
Ilya Dryomov
maintainerQuoted 5 lines · David Howells
Hi Ilya,
Are you going to pick this up, or should I ask Christian to take it through the vfs tree?
Hi David,
I picked it up yesterday.
Thanks,
Ilya
Quoted 48 lines · Patrick Donnelly
From: Patrick Donnelly <[email protected]>
Log recovered from a user's cluster:
<7>[ 5413.970692] ceph: get_cap_refs 00000000958c114b ret 1 got Fr <7>[ 5413.970695] ceph: start_read 00000000958c114b, no cache cap ... <7>[ 5473.934609] ceph: my wanted = Fr, used = Fr, dirty - <7>[ 5473.934616] ceph: revocation: pAsLsXsFr -> pAsLsXs (revoking Fr) <7>[ 5473.934632] ceph: __ceph_caps_issued 00000000958c114b cap 00000000f7784259 issued pAsLsXs <7>[ 5473.934638] ceph: check_caps 10000000e68.fffffffffffffffe file_want - used Fr dirty - flushing - issued pAsLsXs revoking Fr retain pAsLsXsFsr AUTHONLY NOINVAL FLUSH_FORCEThe MDS subsequently complains that the kernel client is late releasing caps.
Approximately, a series of changes to this code by the three commits cited below resulted in subtle resource cleanup to be missed. The main culprit is the change in error handling in 2d31604 which meant that a failure in
init_requestwould no longer cause cleanup to be called. That would prevent theceph_put_cap_refswhich would cleanup the leaked cap ref.Fixes 49870056005ca9387e5ee31451991491f99cc45f ("ceph: convert ceph_readpages to ceph_readahead")Fixes 2de160417315b8d64455fe03e9bb7d3308ac3281 ("netfs: Change ->init_request() to return an error code")Fixes a5c9dc4451394b2854493944dcc0ff71af9705a3 ("ceph: Make ceph_init_request() check caps on readahead")Signed-off-by Patrick Donnelly <[email protected]>--- fs/ceph/addr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/ceph/addr.c +4
-1@@ -489,8 +489,11 @@ static int ceph_init_request(struct netfs_io_request *rreq, struct file *file) rreq->io_streams[0 ].sreq_max_len = fsc->mount_options->rsize; out: -if (ret <0 ) +if (ret <0 ) { +if (got) +ceph_put_cap_refs (ceph_inode (inode), got);kfree (priv); + }return ret; }
Quoted 1 line
base-commit:
e32cde8d2bd7d251a8f9b434143977ddf13dcec6
Good catch!