client: do not allow zero‑length reads by dparmar18 · Pull Request #65656 · ceph/ceph

dparmar18 · 2025-09-24T11:49:43Z

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

You must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.

src/client/Client.cc

chrisphoffman · 2025-11-04T21:12:28Z

src/client/Client.cc

 {
  ceph_assert(ceph_mutex_is_locked_by_me(client_lock));

+  // zero bytes read is not supported by osd


Hmm, I'm thinking more about this approach. If we put this up this high, we'll miss the check for f->mode & CEPH_FILE_MODE_RD) == 0.

I can move that check above before declaring all those variables

In this current form, we still exit if size == 0, but f->mode doesn't have CEPH_FILE_MODE_RD. It should fail in that case regardless of size. We should probably still do this.

can you take a look now? I've moved the ldout statement to top, first check the file handle mode and then the size.

chrisphoffman · 2025-11-04T21:17:40Z

src/client/Client.cc

    // is duplicated and modified and exists in
    // C_Read_Sync_NonBlocking::finish().

-    // trim read based on file size?


I'm trying to keep things consistent. Right now case 1 (_read_async) and case 3 (_read_sync) both check again if the size/len is 0 within the functions. They both are only called from _read(). Do we want to remove those checks, or add a check to C_Read_Sync_NonBlocking to complete immediately in retry() if len == 0? That way we can potentially avoid bugs later on.

Right now case 1 (_read_async) and case 3 (_read_sync) both check again if the size/len is 0 within the functions. They both are only called from _read()

I did thought about this initially but then wondered - what if in future these member functions are called independently from some new API? I felt that considering they are helper functions to carry out async/sync read; i kept those functions intact. WDYT?

add a check to C_Read_Sync_NonBlocking to complete immediately in retry() if len == 0? That way we can potentially avoid bugs later on.

Yeah, doable. It's called from case 2 right now and fixing the retry() could help prevent future bugs.

Also what about C_Read_Async_Finisher::finish? In Client::do_readahead - pair<uint64_t, uint64_t> readahead_extent = f->readahead.update(off, len, in->size); returns an extent and the extent.second would be readahead_length and then is passed in

int r2 = objectcacher->file_read(&in->oset, &in->layout, in->snapid, readahead_extent.first, readahead_extent.second, NULL, 0, onfinish2);

this could again crash

some new API? I felt that considering they are helper functions to carry out async/sync read; i kept those functions intact. WDYT?

Yeah we can keep those, if _read_sync and _read_async both bail if the size is zero that will keep it from failing on unchecked new code paths.

Also what about C_Read_Async_Finisher::finish?

Unless I'm missing something, that has a check, that skips do_readahead if size is 0.

ceph/src/client/Client.cc

Lines 11825 to 11826 in 9d82c94

if (r != 0)

clnt->do_readahead(f, in, off, len);

Also what about C_Read_Async_Finisher::finish?

Unless I'm missing something, that has a check, that skips do_readahead if size is 0.

ceph/src/client/Client.cc

Lines 11825 to 11826 in 9d82c94

if (r != 0)

clnt->do_readahead(f, in, off, len);

oh glad it is, wonder how i missed it but thanks for pointing out!

github-actions · 2025-11-06T14:36:39Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

vshankar · 2025-11-17T09:37:08Z

@dparmar18 please rebase

dparmar18 · 2025-12-02T13:35:45Z

@dparmar18 please rebase

@vshankar this is ready

anoopcs9

Please fix the commit message subject and PR title: client: do not allow zero‑length reads or client: disallow zero length reads

Please fix the typo in the commit message subject: src/test: test zero-length async-fsync read using ll_preadv_pwritev -> s/async/sync.

dparmar18 · 2025-12-03T12:00:17Z

Please fix the typo in the commit message subject: src/test: test zero-length async-fsync read using ll_preadv_pwritev -> s/async/sync.

from the https://tracker.ceph.com/issues/73037 description, i think it should be async

Requested changes have been made in the updated version.

chrisphoffman

all my comments were addressed. lgtm

chrisphoffman · 2025-12-05T13:23:14Z

src/client/Client.cc

    ldout(cct, 10) << " nothing to flush" << dendl;
    return;
  }
+  if (size == 0) {


nit: a new line before here

github-actions · 2025-12-10T05:45:53Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

dparmar18 · 2025-12-18T12:48:31Z

@vshankar request for a teuthology run

dparmar18 · 2026-01-05T13:59:15Z

@vshankar ping

vshankar · 2026-01-05T14:36:16Z

@vshankar request for a teuthology run

I don't have access to the new lab yet. Working it out...

Fixes: https://tracker.ceph.com/issues/73037 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

…_readv_writev Fixes: https://tracker.ceph.com/issues/73037 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

fixing this in Client::_read which is called by both async and sync code paths. Fixes: https://tracker.ceph.com/issues/73037 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

github-actions bot added cephfs Ceph File System tests labels Sep 24, 2025

vshankar assigned vshankar and chrisphoffman Sep 24, 2025

dparmar18 requested a review from a team September 24, 2025 11:52

chrisphoffman reviewed Sep 30, 2025

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

src/client/Client.cc Outdated Show resolved Hide resolved

dparmar18 force-pushed the i73037 branch 5 times, most recently from 6232c76 to b31d6a8 Compare October 31, 2025 08:12

dparmar18 requested review from chrisphoffman and vshankar October 31, 2025 08:13

chrisphoffman reviewed Nov 4, 2025

View reviewed changes

github-actions bot added the needs-rebase label Nov 6, 2025

vshankar requested a review from chrisphoffman November 17, 2025 09:36

dparmar18 force-pushed the i73037 branch 2 times, most recently from 7cb3d4a to 4b82629 Compare November 17, 2025 12:08

github-actions bot removed the needs-rebase label Nov 17, 2025

dparmar18 force-pushed the i73037 branch from 4b82629 to 9d82c94 Compare November 17, 2025 12:59

anoopcs9 previously requested changes Dec 3, 2025

View reviewed changes

dparmar18 force-pushed the i73037 branch 2 times, most recently from fd60702 to d3ca82b Compare December 3, 2025 11:32

dparmar18 changed the title ~~client: no do allow zero length read~~ client: do not allow zero‑length reads Dec 3, 2025

dparmar18 force-pushed the i73037 branch from d3ca82b to a219296 Compare December 3, 2025 12:01

chrisphoffman approved these changes Dec 5, 2025

View reviewed changes

github-actions bot added the needs-rebase label Dec 10, 2025

dparmar18 force-pushed the i73037 branch from a219296 to 84f0adb Compare December 10, 2025 07:31

github-actions bot removed the needs-rebase label Dec 10, 2025

dparmar18 added the needs-qa label Dec 10, 2025

dparmar18 force-pushed the i73037 branch from 84f0adb to 6456553 Compare December 18, 2025 12:48

dparmar18 added 3 commits February 19, 2026 10:19

src/test: test zero-length async-fsync read using ll_preadv_pwritev

b64f5d2

Fixes: https://tracker.ceph.com/issues/73037 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

src/test: test zero-length async-fsync read using ceph_ll_nonblocking…

0f25d4c

…_readv_writev Fixes: https://tracker.ceph.com/issues/73037 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

client: do not allow zero‑length reads

a31e339

fixing this in Client::_read which is called by both async and sync code paths. Fixes: https://tracker.ceph.com/issues/73037 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the i73037 branch from 6456553 to a31e339 Compare February 19, 2026 04:49

Conversation

dparmar18 commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisphoffman Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 6, 2025

Uh oh!

vshankar commented Nov 17, 2025

Uh oh!

dparmar18 commented Dec 2, 2025

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Dec 3, 2025

Uh oh!

chrisphoffman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 10, 2025

Uh oh!

dparmar18 commented Dec 18, 2025

Uh oh!

dparmar18 commented Jan 5, 2026

Uh oh!

vshankar commented Jan 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dparmar18 commented Sep 24, 2025 •

edited

Loading

chrisphoffman Nov 4, 2025 •

edited

Loading