Skip to content

Comments

client: Fix opening and reading of symlinks#59503

Merged
vshankar merged 2 commits intoceph:mainfrom
anoopcs9:client-symlink-fixes
Oct 16, 2024
Merged

client: Fix opening and reading of symlinks#59503
vshankar merged 2 commits intoceph:mainfrom
anoopcs9:client-symlink-fixes

Conversation

@anoopcs9
Copy link
Contributor

@anoopcs9 anoopcs9 commented Aug 29, 2024 •
edited
Loading

Following are the two misbehaviours observed:

  • Opening an already existing symlink with O_PATH and O_NOFOLLOW results in ELOOP.
    • after path_walk() we shouldn't blindly return ELOOP without checking for O_PATH in may_open().
    • see man open(2) for O_PATH explanation.
  • Reading a symlink using ceph_readlinkat() with empty path results in SIGABRT.
    • there is a missing check for empty pathname
    • see man readlinkat(2) under DESCRIPTION.

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

@github-actions github-actions bot added the cephfs Ceph File System label Aug 29, 2024
@anoopcs9
Copy link
Contributor Author

anoopcs9 commented Aug 29, 2024

  • Reading a symlink using ceph_readlinkat() with empty path results in SIGABRT.
(gdb) bt
#0  0x00007f9f8ac01664 in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007f9f8aba8c4e in raise () from /lib64/libc.so.6
#2  0x00007f9f8ab90902 in abort () from /lib64/libc.so.6
#3  0x00007f9f89cdabf0 in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib64/libstdc++.so.6
#4  0x00007f9f8add16df in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=<optimized out>, __pos=<optimized out>) at /usr/include/c++/14/string_view:254
#5  std::basic_string_view<char, std::char_traits<char> >::operator[] (this=<synthetic pointer>, __pos=0) at /usr/include/c++/14/string_view:254
#6  filepath::set_path (this=0x7ffea6894140, s=...) at /usr/src/debug/ceph-18.2.4-1.fc40.x86_64/src/include/filepath.h:101
#7  filepath::filepath (this=<optimized out>, s=<optimized out>, this=<optimized out>, s=<optimized out>) at /usr/src/debug/ceph-18.2.4-1.fc40.x86_64/src/include/filepath.h:94
#8  0x00007f9f8ae23b9a in Client::readlinkat (this=0x35bc950, dirfd=<optimized out>, relpath=0x402107 "", buf=0x7ffea68941e0 "\b5\361\212\237\177", size=256, perms=...)
    at /usr/src/debug/ceph-18.2.4-1.fc40.x86_64/src/client/Client.cc:7899
#9  0x00000000004013dd in main ()

@anoopcs9 anoopcs9 marked this pull request as ready for review August 29, 2024 09:49
batrick
batrick previously requested changes Aug 29, 2024
View reviewed changes
Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a tracker issue so that this issue can be tracked for backporting.

Then, please annotate the commit which fixes/resolves a Ceph tracker issue with:

Fixes: http://tracker.ceph.com/issues/...

This is essential when examining the history of the repository (this commit fixes what) and helps merge scripts identify issues that have been resolved by a merge. See this article on GitHub on how to amend commits and update your pull request.

@anoopcs9 anoopcs9 force-pushed the client-symlink-fixes branch from 2fe4a91 to 3d326b6 Compare August 30, 2024 08:54
@anoopcs9
Copy link
Contributor Author

anoopcs9 commented Aug 30, 2024

Please create a tracker issue so that this issue can be tracked for backporting.

Then, please annotate the commit which fixes/resolves a Ceph tracker issue with:

Fixes: http://tracker.ceph.com/issues/...

This is essential when examining the history of the repository (this commit fixes what) and helps merge scripts identify issues that have been resolved by a merge. See this article on GitHub on how to amend commits and update your pull request.

Done, thanks.

@anoopcs9 anoopcs9 requested a review from batrick August 30, 2024 11:43
@anoopcs9
Copy link
Contributor Author

anoopcs9 commented Sep 13, 2024

jenkins test make check arm64

@vshankar vshankar requested a review from a team September 17, 2024 12:24
@dparmar18
Copy link
Contributor

dparmar18 commented Sep 18, 2024 •
edited
Loading

@anoopcs9 this looks to be easily reproducible, a test case can be added in src/test/libcephfs/test.cc.

dparmar18
dparmar18 reviewed Sep 18, 2024
View reviewed changes
dparmar18
dparmar18 reviewed Sep 18, 2024
View reviewed changes
anoopcs9
anoopcs9 commented Sep 19, 2024
View reviewed changes
Copy link
Contributor Author

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anoopcs9 this looks to be easily reproducible, a test case can be added in src/test/libcephfs/test.cc.

I've added the bare minimum to handle the cases addressed by the changes in the pull request. Let me know if something more is required.

@anoopcs9 anoopcs9 force-pushed the client-symlink-fixes branch from 3d326b6 to 58c6cfa Compare September 19, 2024 09:34
@github-actions github-actions bot added the tests label Sep 19, 2024
dparmar18
dparmar18 reviewed Sep 19, 2024
View reviewed changes
vshankar
vshankar reviewed Sep 20, 2024
View reviewed changes
@vshankar
Copy link
Contributor

vshankar commented Sep 24, 2024

jenkins retest this please

dparmar18
dparmar18 reviewed Sep 24, 2024
View reviewed changes
@anoopcs9 anoopcs9 force-pushed the client-symlink-fixes branch from 58c6cfa to 1db772b Compare September 24, 2024 07:36
dparmar18
dparmar18 approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@dparmar18 dparmar18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

vshankar
vshankar reviewed Sep 25, 2024
View reviewed changes
vshankar
vshankar requested changes Sep 25, 2024
View reviewed changes
@anoopcs9 anoopcs9 force-pushed the client-symlink-fixes branch from 1db772b to 9e29f49 Compare September 25, 2024 06:49
@anoopcs9 anoopcs9 requested a review from vshankar September 25, 2024 06:51
@vshankar
Copy link
Contributor

vshankar commented Oct 4, 2024

This PR is under test in https://tracker.ceph.com/issues/68382.

vshankar added a commit to vshankar/ceph that referenced this pull request Oct 7, 2024
@vshankar
Merge PR ceph#59503 into wip-vshankar-testing-20241007.154038
5d1229f 
* refs/pull/59503/head:
	client: Resolve symlink from dirfd for empty pathname
	client: Fix symlink open with O_PATH and O_NOFOLLOW

Reviewed-by: Venky Shankar <[email protected]>
Reviewed-by: Dhairya Parmar <[email protected]>
vshankar
vshankar approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://tracker.ceph.com/projects/cephfs/wiki/Main#wip-vshankar-testing-20241004070739-debug

@vshankar
Copy link
Contributor

vshankar commented Oct 10, 2024

jenkins retest this please

@vshankar
Copy link
Contributor

vshankar commented Oct 15, 2024

These jenkins test aren't running to completion. @anoopcs9 mind pushing a rebase please?

@anoopcs9
client: Fix symlink open with O_PATH and O_NOFOLLOW
8443821 
man open(2)[1] says the following for O_PATH:

. . .
If  pathname is a symbolic link and the O_NOFOLLOW flag is also
specified, then the call returns a file descriptor referring to the
symbolic link.  This file descriptor can be used as the dirfd argument
in calls to fchownat(2), fstatat(2), linkat(2), and readlinkat(2) with
an empty pathname to have the calls operate on the symbolic link.
. . .

symlink check within may_open() failed to consider the O_PATH flag
resulting in a ELOOP error to the client. In order to return a valid
file descriptor we introduce a check for the presence of O_PATH in
the client provided flags.

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

[1] https://www.man7.org/linux/man-pages/man2/open.2.html

Signed-off-by: Anoop C S <[email protected]>
@anoopcs9
client: Resolve symlink from dirfd for empty pathname
24f453d 
man readlinkat(2)[1] points at a special case for readlinkat() syscall
as follows:

. . .
Since Linux 2.6.39, pathname can be an empty string, in which case the
call operates on the symbolic link referred to by dirfd (which should
have been obtained using open(2) with the O_PATH and O_NOFOLLOW flags).
. . .

man open(2)[2] further explains the need for such a special case when
a symlink is opened with O_PATH and O_NOFOLLOW:

. . .
If  pathname is a symbolic link and the O_NOFOLLOW flag is also
specified, then the call returns a file descriptor referring to the
symbolic link.  This file descriptor can be used as the dirfd argument
in calls to fchownat(2), fstatat(2), linkat(2), and readlinkat(2) with
an empty pathname to have the calls operate on the symbolic link.
. . .

Accordingly have a check to resolve symlinks out of dirfd when empty
pathnames are encountered within readlinkat(). In addition to that
match the standard file system behavior to return ENOENT instead of
EINVAL when the inode pointed to by dirfd is not a symbolic link with
empty pathnames.

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

[1] https://www.man7.org/linux/man-pages/man2/readlinkat.2.html
[2] https://www.man7.org/linux/man-pages/man2/open.2.html

Signed-off-by: Anoop C S <[email protected]>
@anoopcs9 anoopcs9 force-pushed the client-symlink-fixes branch from 9e29f49 to 24f453d Compare October 15, 2024 10:51
@anoopcs9
Copy link
Contributor Author

anoopcs9 commented Oct 15, 2024

These jenkins test aren't running to completion. @anoopcs9 mind pushing a rebase please?

All set.

@vshankar vshankar dismissed batrick’s stale review October 16, 2024 13:35

Comment addressed.

@vshankar vshankar merged commit 68e73b3 into ceph:main Oct 16, 2024
@anoopcs9 anoopcs9 deleted the client-symlink-fixes branch October 16, 2024 13:39
This was referenced Oct 17, 2024
Merged
Merged
Closed
@anoopcs9 anoopcs9 mentioned this pull request Oct 25, 2024
Merged
This was referenced Nov 13, 2024
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vshankar vshankar vshankar approved these changes

@dparmar18 dparmar18 dparmar18 approved these changes

@batrick batrick Awaiting requested review from batrick

Assignees

No one assigned

Labels

cephfs Ceph File System tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anoopcs9 @dparmar18 @vshankar @batrick