| Message ID | 20241127212027.2704515-1-max.kellermann@ionos.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/ceph/file: fix memory leaks in __ceph_sync_read() | expand |
Pages are freed in `ceph_osdc_put_request`, trying to release them this way will end badly. On Wed, Nov 27, 2024 at 11:20 PM Max Kellermann <max.kellermann@ionos.com> wrote: > > In two `break` statements, the call to ceph_release_page_vector() was > missing, leaking the allocation from ceph_alloc_page_vector(). > > Cc: stable@vger.kernel.org > Signed-off-by: Max Kellermann <max.kellermann@ionos.com> > --- > fs/ceph/file.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 4b8d59ebda00..24d0f1cc9aac 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1134,6 +1134,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > extent_cnt = __ceph_sparse_read_ext_count(inode, read_len); > ret = ceph_alloc_sparse_ext_map(op, extent_cnt); > if (ret) { > + ceph_release_page_vector(pages, num_pages); > ceph_osdc_put_request(req); > break; > } > @@ -1168,6 +1169,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > op->extent.sparse_ext_cnt); > if (fret < 0) { > ret = fret; > + ceph_release_page_vector(pages, num_pages); > ceph_osdc_put_request(req); > break; > } > -- > 2.45.2 >
On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@redhat.com> wrote: > Pages are freed in `ceph_osdc_put_request`, trying to release them > this way will end badly. I don't get it. If this ends badly, why does the other ceph_release_page_vector() call after ceph_osdc_put_request() in that function not end badly?
On Thu, Nov 28, 2024 at 1:28 PM Max Kellermann <max.kellermann@ionos.com> wrote: > > On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@redhat.com> wrote: > > Pages are freed in `ceph_osdc_put_request`, trying to release them > > this way will end badly. > > I don't get it. If this ends badly, why does the other > ceph_release_page_vector() call after ceph_osdc_put_request() in that > function not end badly? Look at this piece: osd_req_op_extent_osd_data_pages(req, 0, pages, read_len, offset_in_page(read_off), false, false); The last parameter is "own_pages". Ownership of these pages is NOT transferred to the osdc request, therefore ceph_osdc_put_request() will NOT free them, and this is really a leak bug and my patch fixes it. I just saw this piece of code for the first time, I have no idea. What am I missing?
The ergonomics and error handling on this function seem awkward. I'll take a closer look on how to arrange it better. On Thu, Nov 28, 2024 at 2:31 PM Max Kellermann <max.kellermann@ionos.com> wrote: > > On Thu, Nov 28, 2024 at 1:28 PM Max Kellermann <max.kellermann@ionos.com> wrote: > > > > On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@redhat.com> wrote: > > > Pages are freed in `ceph_osdc_put_request`, trying to release them > > > this way will end badly. > > > > I don't get it. If this ends badly, why does the other > > ceph_release_page_vector() call after ceph_osdc_put_request() in that > > function not end badly? > > Look at this piece: > > osd_req_op_extent_osd_data_pages(req, 0, pages, read_len, > offset_in_page(read_off), > false, false); > > The last parameter is "own_pages". Ownership of these pages is NOT > transferred to the osdc request, therefore ceph_osdc_put_request() > will NOT free them, and this is really a leak bug and my patch fixes > it. > > I just saw this piece of code for the first time, I have no idea. What > am I missing? >
On Thu, Nov 28, 2024 at 1:49 PM Alex Markuze <amarkuze@redhat.com> wrote: > The ergonomics and error handling on this function seem awkward. I'll > take a closer look on how to arrange it better. Does that mean you misunderstood page ownership, or do you still believe my patch is wrong? To me, "pages must not be freed manually" and "pages must be freed manually" can't both be right at the same time.
On Thu, Nov 28, 2024 at 1:18 PM Alex Markuze <amarkuze@redhat.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.
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 4b8d59ebda00..24d0f1cc9aac 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1134,6 +1134,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, extent_cnt = __ceph_sparse_read_ext_count(inode, read_len); ret = ceph_alloc_sparse_ext_map(op, extent_cnt); if (ret) { + ceph_release_page_vector(pages, num_pages); ceph_osdc_put_request(req); break; } @@ -1168,6 +1169,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, op->extent.sparse_ext_cnt); if (fret < 0) { ret = fret; + ceph_release_page_vector(pages, num_pages); ceph_osdc_put_request(req); break; }
In two `break` statements, the call to ceph_release_page_vector() was missing, leaking the allocation from ceph_alloc_page_vector(). Cc: stable@vger.kernel.org Signed-off-by: Max Kellermann <max.kellermann@ionos.com> --- fs/ceph/file.c | 2 ++ 1 file changed, 2 insertions(+)