linux-kernel - Re: [PATCH] fs/ceph/file: fix memory leaks in __ceph_sync

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOi1vP-SSyTtLJ1_YVCxQeesY35TPxud8T=Wiw8Fk7QWEpu7jw@mail.gmail.com>
Date: Thu, 5 Dec 2024 12:24:42 +0100
From: Ilya Dryomov <idryomov@...il.com>
To: Max Kellermann <max.kellermann@...os.com>
Cc: Alex Markuze <amarkuze@...hat.com>, xiubli@...hat.com, ceph-devel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] fs/ceph/file: fix memory leaks in __ceph_sync_read()

On Thu, Dec 5, 2024 at 9:32 AM Max Kellermann <max.kellermann@...os.com> wrote:
>
> On Fri, Nov 29, 2024 at 9:06 AM Max Kellermann <max.kellermann@...os.com> wrote:
> >
> > On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@...hat.com> 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