fs/ceph/file: fix memory leaks in __ceph_sync_read()

In two break statements, the call to ceph_release_page_vector() was missing, leaking the allocation from ceph_alloc_page_vector().

Cc [email protected]

Signed-off-by Max Kellermann <[email protected]>

---
 fs/ceph/file.c | 2 ++
 1 file changed, 2 insertions(+)

fs/ceph/file.c +2 -1

index 4b8d59ebda00..24d0f1cc9aac 100644
@@ -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

Pages are freed in ceph_osdc_put_request, trying to release them this way will end badly.

Quoted 2 lines · Alex Markuze

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?

Quoted 8 lines · Max Kellermann

Quoted 2 lines · Alex Markuze

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.

Pages are freed in ceph_osdc_put_request, trying to release them this way will end badly.

Quoted 2 lines · Alex Markuze

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.

Quoted 5 lines · Max Kellermann

Quoted 2 lines · Alex Markuze

Pages are freed in ceph_osdc_put_request, trying to release them this way will end badly.

Quoted 3 lines

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

Quoted 7 lines · Max Kellermann

Quoted 5 lines · Max Kellermann

Quoted 2 lines · Alex Markuze

Pages are freed in ceph_osdc_put_request, trying to release them this way will end badly.

Quoted 7 lines

Quoted 3 lines

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

This is a bad patch, I don't appreciate partial fixes that introduce unnecessary complications to the code, and it conflicts with the complete fix in the other thread.

Quoted 3 lines · Alex Markuze

This is a bad patch, I don't appreciate partial fixes that introduce unnecessary complications to the code, and it conflicts with the complete fix in the other thread.

Alex, and I don't appreciate the unnecessary complications you introduce to the Ceph contribution process!

The mistake you made in your first review ("will end badly") is not a big deal; happens to everybody - but you still don't admit the mistake and you ghosted me for a week. But then saying you don't appreciate the work of somebody who found a bug and posted a simple fix is not good communication. You can say you prefer a different patch and explain the technical reasons; but saying you don't appreciate it is quite condescending.

Now back to the technical facts:

- What exactly about my patch is "bad"?
- Do you believe my patch is not strictly an improvement?
- Why do you believe my fix is only "partial"?
- What unnecessary complications are introduced by my two-line patch in your opinion?
- What "other thread"? I can't find anything on LKML and ceph-devel.

Max

The full fix is now in the testing branch.

Max, please follow the mailing list, I posted the patch last week on the initial thread regarding this issue. Please, comment on the correct thread, having two threads regarding the same issue introduces unnecessary confusion.

- What "other thread"? I can't find anything on LKML and ceph-devel.

Quoted 1 line · Max Kellermann

- What "other thread"? I can't find anything on LKML and ceph-devel.

Just in case you mean this patch authored by you:
https://github.com/ceph/ceph-client/commit/2a802a906f9c89f8ae492dbfcd82ff41272abab1

I don't think that's a good patch, and if I had the power, I would certainly reject it, because:

- it's big and confusing; hard to review
- it's badly documented; the commit message is just "fixing a race condition when a file shrinks" but other than that, doesn't explain anything; a proper explanation is necessary for such a complicated
diff
- the patch changes many distinct things in one patch, but it should really be split
- this patch is about the buffer overflow for which my patch is much
simpler: https://lore.kernel.org/lkml/[email protected]/
which I suggested merging instead of all the other candicate patches
https://lore.kernel.org/lkml/CAKPOu+9kdcjMf36bF3HAW4K8v0mHxXQX3_oQfGSshmXBKtS43A@mail.gmail.com/
but you did not reply (as usual, sigh!)
- deeply hidden in this patch is also a fix for the memory leak, but instead of making one large patch which fixes everything, you should
first merge my trivial leak bug fix and then the fix for the buffer overflow on top

I will explain the process for ceph client patches. It's important to
note: The process itself and part of the automation is still evolving and so many things have to be done manually.

I will break it up into three separate patches, as it addresses three different issues.

Quoted 4 lines · Alex Markuze

I will explain the process for ceph client patches. It's important to
note: The process itself and part of the automation is still evolving and so many things have to be done manually.

None of this answers any of my questions on your negative review comments.

Quoted 2 lines

I will break it up into three separate patches, as it addresses three different issues.

... one of which will be my patch.

Max, would you be willing to redo this patch to pass true for own_pages and post a v2? There is nothing "bad", "partial" or otherwise wrong with this version, but having the pages be taken care of automatically is a bit nicer and a conflict with Alex's ongoing work would be avoided.

btw. Alex's patch
(https://github.com/ceph/ceph-client/commit/2a802a906f9c89f8ae4)
introduces another memory leak

In two break statements, the call to ceph_release_page_vector() was missing, leaking the allocation from ceph_alloc_page_vector().

Instead of adding the missing ceph_release_page_vector() calls, the Ceph maintainers preferred to transfer page ownership to the ceph_osd_request by passing own_pages=true to osd_req_op_extent_osd_data_pages(). This requires postponing the ceph_osdc_put_request() call until after the block that accesses the pages.

Cc [email protected]

Signed-off-by Max Kellermann <[email protected]>

---
 fs/ceph/file.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

fs/ceph/file.c +3 -5

index 4b8d59ebda00..ce342a5d4b8b 100644
@@ -1127,7 +1127,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
 
                osd_req_op_extent_osd_data_pages(req, 0, pages, read_len,
                                                 offset_in_page(read_off),
-                                                false, false);
+                                                false, true);
 
                op = &req->r_ops[0];
                if (sparse) {

@@ -1186,8 +1186,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
                        ret = min_t(ssize_t, fret, len);
                }
 
-               ceph_osdc_put_request(req);
-
                /* Short read but not EOF? Zero out the remainder. */
                if (ret >= 0 && ret < len && (off + ret < i_size)) {
                        int zlen = min(len - ret, i_size - off - ret);

@@ -1221,7 +1219,8 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
                                break;
                        }
                }
-               ceph_release_page_vector(pages, num_pages);
+
+               ceph_osdc_put_request(req);
 
                if (ret < 0) {
                        if (ret == -EBLOCKLISTED)
--

2.45.2

Good.
This sequence has not been tested independently, but it should be fine.

Quoted 52 lines

In two break statements, the call to ceph_release_page_vector() was missing, leaking the allocation from ceph_alloc_page_vector().

Instead of adding the missing ceph_release_page_vector() calls, the Ceph maintainers preferred to transfer page ownership to the ceph_osd_request by passing own_pages=true to osd_req_op_extent_osd_data_pages(). This requires postponing the ceph_osdc_put_request() call until after the block that accesses the pages.

Cc [email protected]

Signed-off-by Max Kellermann <[email protected]>

---
 fs/ceph/file.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

fs/ceph/file.c +3 -5

index 4b8d59ebda00..ce342a5d4b8b 100644
@@ -1127,7 +1127,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
 
                osd_req_op_extent_osd_data_pages(req, 0, pages, read_len,
                                                 offset_in_page(read_off),
-                                                false, false);
+                                                false, true);
 
                op = &req->r_ops[0];
                if (sparse) {

@@ -1186,8 +1186,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
                        ret = min_t(ssize_t, fret, len);
                }
 
-               ceph_osdc_put_request(req);
-
                /* Short read but not EOF? Zero out the remainder. */
                if (ret >= 0 && ret < len && (off + ret < i_size)) {
                        int zlen = min(len - ret, i_size - off - ret);

@@ -1221,7 +1219,8 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
                                break;
                        }
                }
-               ceph_release_page_vector(pages, num_pages);
+
+               ceph_osdc_put_request(req);
 
                if (ret < 0) {
                        if (ret == -EBLOCKLISTED)
--

2.45.2