[PATCH] ceph: fix cap ref leak via netfs init_request

[PATCH] ceph: fix cap ref leak via netfs init_request

[Patrick Donnelly]

From: Patrick Donnelly <pdonnell@redhat.com>

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_FORCE

The MDS subsequently complains that the kernel client is late releasing caps.

Closes: https://tracker.ceph.com/issues/67008
Fixes: a5c9dc4451394b2854493944dcc0ff71af9705a3 ("ceph: Make ceph_init_request() check caps on readahead")
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/ceph/addr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 53fef258c2bc..702c6a730b70 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -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
-- 
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[Ilya Dryomov]

On Wed, Oct 2, 2024 at 10:08 PM Patrick Donnelly <batrick@batbytes.com> wrote:
>
> From: Patrick Donnelly <pdonnell@redhat.com>
>
> 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

Hi Patrick,

Noting that start_read() was removed in kernel 5.13 in commit
49870056005c ("ceph: convert ceph_readpages to ceph_readahead").

>     ...
>     <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_FORCE
>
> The MDS subsequently complains that the kernel client is late releasing caps.
>
> Closes: https://tracker.ceph.com/issues/67008
> Fixes: a5c9dc4451394b2854493944dcc0ff71af9705a3 ("ceph: Make ceph_init_request() check caps on readahead")

I think it's worth going into a bit more detail here because
superficially this commit just replaced

    ret = ceph_try_get_caps(inode, CEPH_CAP_FILE_RD, want, true, &got);
    if (ret < 0)
            dout("start_read %p, error getting cap\n", inode);
    else if (!(got & want))
            dout("start_read %p, no cache cap\n", inode);

    if (ret <= 0)
            return;

in ceph_readahead() with

    ret = ceph_try_get_caps(inode, CEPH_CAP_FILE_RD, want, true, &got);
    if (ret < 0) {
            dout("start_read %p, error getting cap\n", inode);
            return ret;
    }

    if (!(got & want)) {
            dout("start_read %p, no cache cap\n", inode);
            return -EACCES;
    }
    if (ret == 0)
            return -EACCES;

in ceph_init_request().  Neither called ceph_put_cap_refs() before
bailing.  It was commit 49870056005c ("ceph: convert ceph_readpages to
ceph_readahead") that turned a direct call to ceph_put_cap_refs() in
start_read() to one in ceph_readahead_cleanup() (later renamed to
ceph_netfs_free_request()).

The actual problem is that netfs_alloc_request() just frees rreq if
init_request() callout fails and ceph_netfs_free_request() is never
called, right?  If so, I'd mention that explicitly and possibly also
reference commit 2de160417315 ("netfs: Change ->init_request() to
return an error code") which introduced that.

> Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/ceph/addr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 53fef258c2bc..702c6a730b70 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -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;
>  }

The change itself looks fine.  Great catch!

Thanks,

                Ilya

[PATCH] ceph: fix cap ref leak via netfs init_request

[Patrick Donnelly]

From: Patrick Donnelly <pdonnell@redhat.com>

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_FORCE

The 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.

Closes: https://tracker.ceph.com/issues/67008
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 <pdonnell@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/ceph/addr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 53fef258c2bc..702c6a730b70 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -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
-- 
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[Patrick Donnelly]

On Wed, Oct 2, 2024 at 5:52 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Wed, Oct 2, 2024 at 10:08 PM Patrick Donnelly <batrick@batbytes.com> wrote:
> >
> > From: Patrick Donnelly <pdonnell@redhat.com>
> >
> > 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
>
> Hi Patrick,
>
> Noting that start_read() was removed in kernel 5.13 in commit
> 49870056005c ("ceph: convert ceph_readpages to ceph_readahead").
>
> >     ...
> >     <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_FORCE
> >
> > The MDS subsequently complains that the kernel client is late releasing caps.
> >
> > Closes: https://tracker.ceph.com/issues/67008
> > Fixes: a5c9dc4451394b2854493944dcc0ff71af9705a3 ("ceph: Make ceph_init_request() check caps on readahead")
>
> I think it's worth going into a bit more detail here because
> superficially this commit just replaced
>
>     ret = ceph_try_get_caps(inode, CEPH_CAP_FILE_RD, want, true, &got);
>     if (ret < 0)
>             dout("start_read %p, error getting cap\n", inode);
>     else if (!(got & want))
>             dout("start_read %p, no cache cap\n", inode);
>
>     if (ret <= 0)
>             return;
>
> in ceph_readahead() with
>
>     ret = ceph_try_get_caps(inode, CEPH_CAP_FILE_RD, want, true, &got);
>     if (ret < 0) {
>             dout("start_read %p, error getting cap\n", inode);
>             return ret;
>     }
>
>     if (!(got & want)) {
>             dout("start_read %p, no cache cap\n", inode);
>             return -EACCES;
>     }
>     if (ret == 0)
>             return -EACCES;
>
> in ceph_init_request().  Neither called ceph_put_cap_refs() before
> bailing.  It was commit 49870056005c ("ceph: convert ceph_readpages to
> ceph_readahead") that turned a direct call to ceph_put_cap_refs() in
> start_read() to one in ceph_readahead_cleanup() (later renamed to
> ceph_netfs_free_request()).
>
> The actual problem is that netfs_alloc_request() just frees rreq if
> init_request() callout fails and ceph_netfs_free_request() is never
> called, right?  If so, I'd mention that explicitly and possibly also
> reference commit 2de160417315 ("netfs: Change ->init_request() to
> return an error code") which introduced that.

Yes, this looks right. To be clear, we were passing "got" as the
"priv" pointer but it was thrown out when 2de160417315 changed the
error handling. Furthermore, a5c9dc445 made it even worse by
discarding "got" completely on error.

-- 
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[Ilya Dryomov]

On Thu, Oct 3, 2024 at 3:05 AM Patrick Donnelly <batrick@batbytes.com> wrote:
>
> From: Patrick Donnelly <pdonnell@redhat.com>
>
> 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_FORCE
>
> The 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.
>
> Closes: https://tracker.ceph.com/issues/67008
> 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 <pdonnell@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/ceph/addr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 53fef258c2bc..702c6a730b70 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -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
> --
> Patrick Donnelly, Ph.D.
> He / Him / His
> Red Hat Partner Engineer
> IBM, Inc.
> GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
>

Applied.

Thanks,

                Ilya

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[David Howells]

Ilya Dryomov <idryomov@gmail.com> wrote:

> The actual problem is that netfs_alloc_request() just frees rreq if
> init_request() callout fails and ceph_netfs_free_request() is never
> called, right?

I could make it call ->free_request() in the case that ->init_request()
returns an error, though I'd prefer that the cleanup be done in
->init_request() rather than passing a partially set-up state to
->free_request().

David

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[David Howells]

Patrick Donnelly <batrick@batbytes.com> wrote:

> 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_FORCE
> 
> The 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.
> 
> Closes: https://tracker.ceph.com/issues/67008
> 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.

> Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
> Cc: stable@vger.kernel.org

Reviewed-by: David Howells <dhowells@redhat.com>

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[David Howells]

Hi Ilya,

Are you going to pick this up, or should I ask Christian to take it through
the vfs tree?

David

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[Ilya Dryomov]

On Fri, Oct 4, 2024 at 5:37 PM David Howells <dhowells@redhat.com> wrote:
>
> 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

Re: [PATCH] ceph: fix cap ref leak via netfs init_request

[Xiubo Li]

On 10/3/24 09:05, Patrick Donnelly wrote:
> From: Patrick Donnelly <pdonnell@redhat.com>
>
> 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_FORCE
>
> The 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.
>
> Closes: https://tracker.ceph.com/issues/67008
> 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 <pdonnell@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>   fs/ceph/addr.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 53fef258c2bc..702c6a730b70 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -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

Good catch!

Reviewed-by: Xiubo Li <xiubli@redhat.com>