Project

General

Profile

Actions

Bug #63619

closed

client: check for negative value of iovcnt before passing it to internal functions during async I/O

Added by Dhairya Parmar 6 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Category:
Correctness/Safety
Target version:
% Done:

0%

Source:
Development
Tags:
Backport:
quincy,reef
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(FS):
Client
Labels (FS):
Pull request ID:
Crash signature (v1):
Crash signature (v2):

Description

While running async I/O with negative iovcnt in Client::ll_preadv_pwritev(), I experienced a crash:

 ceph version 18.0.0-7275-g8bc4cf1939c (8bc4cf1939ce76918542dbb5433652de295e5916) reef (dev)
 1: ./bin/ceph_test_client(+0x259796) [0x5607247f4796]
 2: /lib64/libc.so.6(+0x3cb60) [0x7f624905fb60]
 3: (Client::_preadv_pwritev_locked(Fh*, iovec const*, unsigned int, long, bool, bool, Context*, ceph::buffer::v15_2_0::list*, bool, bool)+0xa6) [0x56072478ada4]
 4: (Client::ll_preadv_pwritev(Fh*, iovec const*, int, long, bool, Context*, ceph::buffer::v15_2_0::list*, bool, bool)+0xbb) [0x56072478b64f]
 5: (TestClient_LlreadvLlwritev_Test::TestBody()+0x50a) [0x560724721f2c]
 6: (void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x1b) [0x560724811e58]
 7: (void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x80) [0x560724819def]
 8: (testing::Test::Run()+0xb4) [0x56072480b96a]
 9: (testing::TestInfo::Run()+0x104) [0x56072480ba70]
 10: (testing::TestSuite::Run()+0xb2) [0x56072480bb24]
 11: (testing::internal::UnitTestImpl::RunAllTests()+0x36b) [0x56072480cfad]
 12: (bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x1b) [0x5607248120c8]
 13: (bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)+0x80) [0x56072481a235]
 14: (testing::UnitTest::Run()+0x63) [0x56072480bc3f]
 15: (RUN_ALL_TESTS()+0x11) [0x56072472085f]
 16: main()
 17: /lib64/libc.so.6(+0x27550) [0x7f624904a550]
 18: __libc_start_main()
 19: _start()

This is because the Client::_preadv_pwritev_locked() accepts iovcnt as unsigned int and so a negative value get's converted to a huge unsigned int value and the loop for finding total length iov structures fails

loff_t totallen = 0;
for (unsigned i = 0; i < iovcnt; i++) {
    totallen += iov[i].iov_len;
}

This can be fixed by checking if the iovcnt is negative in the functions itself before passing the iovcnt value to the internal functions. There are two other functions Client::ll_writev() and Client::ll_readv() that needs the fix as well.


Related issues 1 (0 open1 closed)

Related to CephFS - Bug #63734: client: handle callback when async io failsResolvedDhairya Parmar

Actions
Actions #1

Updated by Dhairya Parmar 6 months ago

At first glance I thought that maybe it is the way coded (function taking care of edge cases before calling internal functions) but looking at the the man page [0] where preadv() and pwritev() params have signed int iov; having this handled in all the functions seems unnecessary. Instead I'd propose to make change to Client::_preadv_pwritev_locked(), which will eliminate from all the read/write methods that need to check for negative value of iovcnt

[0] https://man7.org/linux/man-pages/man2/pwritev.2.html

Actions #2

Updated by Dhairya Parmar 6 months ago

  • Status changed from New to In Progress
  • Pull request ID set to 54640
Actions #3

Updated by Dhairya Parmar 6 months ago

  • Backport set to quincy,reef
Actions #4

Updated by Dhairya Parmar 6 months ago

  • Target version set to v19.0.0
Actions #5

Updated by Xiubo Li 5 months ago

  • Status changed from In Progress to Fix Under Review
Actions #6

Updated by Dhairya Parmar 5 months ago

  • Subject changed from client: check for negative value of iovcnt before passing it to internal functions to client: check for negative value of iovcnt before passing it to internal functions during async I/O
Actions #7

Updated by Dhairya Parmar 5 months ago

  • Status changed from Fix Under Review to Closed
Actions #8

Updated by Dhairya Parmar 5 months ago

  • Related to Bug #63734: client: handle callback when async io fails added
Actions

Also available in: Atom PDF