Re: [PATCH] fs/ceph/file: fix memory leaks in __ceph_sync_read()

[Ilya Dryomov]

On Thu, Dec 5, 2024 at 9:32 AM Max Kellermann <max.kellermann@xxxxxxxxx> wrote:
>
> On Fri, Nov 29, 2024 at 9:06 AM Max Kellermann <max.kellermann@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@xxxxxxxxxx> wrote:
> > > Pages are freed in `ceph_osdc_put_request`, trying to release them
> > > this way will end badly.
> >
> > Is there anybody else who can explain this to me?
> > I believe Alex is wrong and my patch is correct, but maybe I'm missing
> > something.
>
> It's been a week. Is there really nobody who understands this piece of
> code? I believe I do understand it, but my understanding conflicts
> with Alex's, and he's the expert (and I'm not).

Hi Max,

Your understanding is correct.  Pages would be freed automatically
together with the request only if the ownership is transferred by
passing true for own_pages to osd_req_op_extent_osd_data_pages(), which
__ceph_sync_read() doesn't do.

These error path leaks were introduced in commits 03bc06c7b0bd ("ceph:
add new mount option to enable sparse reads") and f0fe1e54cfcf ("ceph:
plumb in decryption during reads") with support for fscrypt.  Looking
at the former commit, it looks like a similar leak was introduced in
ceph_direct_read_write() too -- on bvecs instead of pages.

I have applied this patch and will take care of the leak on bvecs
myself because I think I see other issues there.

Thanks,

                Ilya