client: fix leak of file handles by xhernandez · Pull Request #55619 · ceph/ceph

xhernandez · 2024-02-16T15:03:12Z

Based on posix specification, the fd passed to fdopendir() will be closed by closedir(). However CephFS client wasn't doing that. If the user opened a directory using ceph_openat(), for example, and then passed the returned fd to ceph_fdopendir(), the created Fh associated with the new open was never destroyed.

This patch records the fd used in ceph_fdopendir() so that it can be closed when ceph_closedir() is called.

Fixes: https://tracker.ceph.com/issues/64479

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 retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

anoopcs9

As mentioned below, commit message also needs the following correction:
s/ceph_fdopenat()/ceph_fdopendir

anoopcs9 · 2024-02-16T17:47:08Z

src/client/Client.h

  std::vector<dentry> buffer;
  struct dirent de;
+
+  int fd;                // fd attached using fdopenat (-1 if none)


Suggested change

int fd; // fd attached using fdopenat (-1 if none)

int fd; // fd attached using fdopendir (-1 if none)

Good catch. Thanks :)

vshankar · 2024-02-17T06:02:00Z

@xhernandez Thanks for the fix. We intent to backport this fix, so, this requires a redmine tracker (and link it in the commit message with Fixes: http://.... tag.

xhernandez · 2024-02-17T07:21:07Z

@xhernandez Thanks for the fix. We intent to backport this fix, so, this requires a redmine tracker (and link it in the commit message with Fixes: http://.... tag.

Done

src/client/Client.h

Based on posix specification, the fd passed to fdopendir() will be closed by closedir(). However CephFS client wasn't doing that. If the user opened a directory using ceph_openat(), for example, and then passed the returned fd to ceph_fdopendir(), the created Fh associated with the new open was never destroyed. This patch records the fd used in ceph_fdopendir() so that it can be closed when ceph_closedir() is called. Fixes: https://tracker.ceph.com/issues/64479 Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

dparmar18 · 2024-02-19T10:02:09Z

it would be good to have a test case for this, you can try use Client::get_filehandle to check if the fd is still valid post closedir

xhernandez · 2024-02-19T11:16:21Z

it would be good to have a test case for this, you can try use Client::get_filehandle to check if the fd is still valid post closedir

@dparmar18 How should I write this test ? I've seen that there are some CephFS tests that use libcephfs in src/test/libcephfs, but Client::get_filehandle() is not exported by libcephfs. Should I write a test in src/test/client ?

dparmar18 · 2024-02-19T11:27:22Z

it would be good to have a test case for this, you can try use Client::get_filehandle to check if the fd is still valid post closedir

@dparmar18 How should I write this test ? I've seen that there are some CephFS tests that use libcephfs in src/test/libcephfs, but Client::get_filehandle() is not exported by libcephfs. Should I write a test in src/test/client ?

yeah that would be the place

xhernandez · 2024-02-19T12:11:20Z

it would be good to have a test case for this, you can try use Client::get_filehandle to check if the fd is still valid post closedir

@dparmar18 How should I write this test ? I've seen that there are some CephFS tests that use libcephfs in src/test/libcephfs, but Client::get_filehandle() is not exported by libcephfs. Should I write a test in src/test/client ?

yeah that would be the place

I've just realized that Client::get_filehandle() is a protected method. Is there any other way to write tests that can access internal methods ?

dparmar18 · 2024-02-19T14:11:32Z

it would be good to have a test case for this, you can try use Client::get_filehandle to check if the fd is still valid post closedir

@dparmar18 How should I write this test ? I've seen that there are some CephFS tests that use libcephfs in src/test/libcephfs, but Client::get_filehandle() is not exported by libcephfs. Should I write a test in src/test/client ?

yeah that would be the place

I've just realized that Client::get_filehandle() is a protected method. Is there any other way to write tests that can access internal methods ?

Right, that is a blocker then. I guess the what looks possible is to write a test case via a script and run it using a YAML

xhernandez · 2024-02-19T14:20:34Z

I've just realized that Client::get_filehandle() is a protected method. Is there any other way to write tests that can access internal methods ?

Right, that is a blocker then. I guess the what looks possible is to write a test case via a script and run it using a YAML

I'm sorry, but I'm very new to Ceph development and I don't know much about it, specially for testing. Can you point me to some place where it's described how to write these scripts and yaml files, and how to integrate them into the regular testing procedures ?

dparmar18 · 2024-02-19T14:30:22Z

I've just realized that Client::get_filehandle() is a protected method. Is there any other way to write tests that can access internal methods ?

Right, that is a blocker then. I guess the what looks possible is to write a test case via a script and run it using a YAML

I'm sorry, but I'm very new to Ceph development and I don't know much about it, specially for testing. Can you point me to some place where it's described how to write these scripts and yaml files, and how to integrate them into the regular testing procedures ?

it should be simple, checkout qa/workunits/, you should find tons of scripts, and I hope this isn't getting sort of an overkill, any thoughts @vshankar

vshankar · 2024-02-19T16:00:09Z

it would be good to have a test case for this, you can try use Client::get_filehandle to check if the fd is still valid post closedir

Its a bit tricky to have test cases for thing like this fix since the way the client is written. I'd defer that for now...

dparmar18

LGTM

rosinL · 2024-02-22T07:59:46Z

jenkins test make check arm64

vshankar · 2024-02-27T04:36:27Z

https://pulpito.ceph.com/?branch=wip-vshankar-testing-20240226.124304

vshankar · 2024-02-27T05:59:33Z

https://pulpito.ceph.com/vshankar-2024-02-27_04:05:06-fs-wip-vshankar-testing-20240226.124304-testing-default-smithi/

* refs/pull/55619/head: client: fix leak of file handles Reviewed-by: Dhairya Parmar <dparmar@redhat.com> Reviewed-by: Venky Shankar <vshankar@redhat.com>

vshankar · 2024-03-04T10:15:49Z

Reruning tests with #55909 and #55908 included. See - https://pulpito.ceph.com/vshankar-2024-03-04_08:26:39-fs-wip-vshankar-testing-20240304.042522-testing-default-smithi/

ceph/ceph#55619 eliminates the need for ceph_close() call after successful ceph_fdopendir() one. And introduces automatic file descriptor's close when corresponding ceph_closedir() is called. That hasty ceph_close() call makes file descriptor available for a new allocation which might conflict with the automatic fd close in the above ceph_closedir(). Full PeerReplayer::do_synchronize() has been reworked to close fds properly, depending on whether ceph_fdopendir() has been already applied to them. Additionally for the sake of uniformity this reworks incremental do_synchronize() in a way to do final fd closings similar to full implementation. Plus this effectively reverts ceph/ceph@bd78bdc as it looks like a wrong approach to fight broken file descriptor references. No much sense in reopening of the current snapshot's root folder on each new entry processing. Instead this patch just doesn't close it from the beginning. Fixes: https://tracker.ceph.com/issues/68853 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 7a747bc)

Likely ceph#55619 missed that. Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 6f1d21c) (cherry picked from commit 442e645) Resolves: rhbz#2331338

ceph#55619 eliminates the need for ceph_close() call after successful ceph_fdopendir() one. And introduces automatic file descriptor's close when corresponding ceph_closedir() is called. That hasty ceph_close() call makes file descriptor available for a new allocation which might conflict with the automatic fd close in the above ceph_closedir(). Full PeerReplayer::do_synchronize() has been reworked to close fds properly, depending on whether ceph_fdopendir() has been already applied to them. Additionally for the sake of uniformity this reworks incremental do_synchronize() in a way to do final fd closings similar to full implementation. Plus this effectively reverts ceph@bd78bdc as it looks like a wrong approach to fight broken file descriptor references. No much sense in reopening of the current snapshot's root folder on each new entry processing. Instead this patch just doesn't close it from the beginning. Fixes: https://tracker.ceph.com/issues/68853 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 7a747bc) (cherry picked from commit c4ce3c0) Resolves: rhbz#2331338

Likely ceph#55619 missed that. Resolves: rhbz#2317735 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 6f1d21c) (cherry picked from commit 442e645)

ceph#55619 eliminates the need for ceph_close() call after successful ceph_fdopendir() one. And introduces automatic file descriptor's close when corresponding ceph_closedir() is called. That hasty ceph_close() call makes file descriptor available for a new allocation which might conflict with the automatic fd close in the above ceph_closedir(). Full PeerReplayer::do_synchronize() has been reworked to close fds properly, depending on whether ceph_fdopendir() has been already applied to them. Additionally for the sake of uniformity this reworks incremental do_synchronize() in a way to do final fd closings similar to full implementation. Plus this effectively reverts ceph@bd78bdc as it looks like a wrong approach to fight broken file descriptor references. No much sense in reopening of the current snapshot's root folder on each new entry processing. Instead this patch just doesn't close it from the beginning. Resolves: rhbz#2317735 Fixes: https://tracker.ceph.com/issues/68853 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 7a747bc) (cherry picked from commit c4ce3c0)

Likely ceph#55619 missed that. Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 6f1d21c)

ceph#55619 eliminates the need for ceph_close() call after successful ceph_fdopendir() one. And introduces automatic file descriptor's close when corresponding ceph_closedir() is called. That hasty ceph_close() call makes file descriptor available for a new allocation which might conflict with the automatic fd close in the above ceph_closedir(). Full PeerReplayer::do_synchronize() has been reworked to close fds properly, depending on whether ceph_fdopendir() has been already applied to them. Additionally for the sake of uniformity this reworks incremental do_synchronize() in a way to do final fd closings similar to full implementation. Plus this effectively reverts ceph@bd78bdc as it looks like a wrong approach to fight broken file descriptor references. No much sense in reopening of the current snapshot's root folder on each new entry processing. Instead this patch just doesn't close it from the beginning. Fixes: https://tracker.ceph.com/issues/68853 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io> (cherry picked from commit 7a747bc)