Page MenuHomeFreeBSD

sendfile: Return the object size from sendfile_getobj()
ClosedPublic

Authored by markj on Feb 19 2021, 9:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 28, 2:12 AM
Unknown Object (File)
Fri, Oct 18, 8:53 AM
Unknown Object (File)
Sep 30 2024, 11:10 PM
Unknown Object (File)
Sep 30 2024, 12:18 AM
Unknown Object (File)
Sep 29 2024, 9:44 PM
Unknown Object (File)
Sep 29 2024, 4:40 PM
Unknown Object (File)
Sep 29 2024, 4:40 PM
Unknown Object (File)
Sep 25 2024, 1:03 PM
Subscribers

Details

Summary

I believe this is more correct, since subsequent bounds checking, e.g.
by vm_pager_has_page(), is done using the object size. It also fixes an
unlocked read of the object size when a shared memory object is used.
I also changed to using the object read lock instead of the write lock,
as I cannot see a reason for the latter.

As a side effect, this change mitigates a problem observed on ZFS when
transmitting a file with sendfile() at the same time that the file is
being extended by a writer. The basic problem is that the file size (as
returned by VOP_GETATTR()) and the object size (updated by
vnode_pager_setsize()) may be out of sync unless the exclusive vnode
lock is held. In particular, there may be a window where the file size
is larger than the object size. So, if sendfile() is transmitting the
whole file, it will transmit <file size> bytes, but vm_pager_has_page()
may return false during this window. In this case, sendfile() will
zero-fill the page, incorrectly assuming that there is a hole in the
file. This can manifest as apparent data corruption if the file is read
back from the page cache.

Following a suggestion from kib I also started looking at using a
rangelock to ensure that the length of the file does not change as we
are paging in. (Use of async getpages means that the file contents
cannot be kept consistent, I believe.) However, I would like to have a
simpler mitigation for 13.0, so I propose this change first.

Test Plan

@dumbbell reported the problem and provided a test case to
demonstrate it. I would ask Peter to test it before committing.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 37208
Build 34097: arc lint + arc unit

Event Timeline

markj requested review of this revision.Feb 19 2021, 9:46 PM
markj added reviewers: glebius, kib, chs.
markj added a subscriber: dumbbell.
This revision is now accepted and ready to land.Feb 20 2021, 12:51 PM

Hi!

I tested this patch with the testsuite at work which failed because of the issue and it works perfectly now.

Thank you very much!

  • Fixes for tmpfs and shm objects.

Tested by: pho

This revision now requires review to proceed.Feb 24 2021, 3:23 PM

Not calling VOP_GETATTR was a feature of the patch. At least, you can do it based on the object type?

This revision is now accepted and ready to land.Feb 24 2021, 6:39 PM
  • Avoid VOP_GETATTR when the object type is OBJT_VNODE.
  • Add a short comment explaining why we prefer the pager size.
This revision now requires review to proceed.Feb 24 2021, 7:13 PM
This revision is now accepted and ready to land.Feb 24 2021, 7:33 PM