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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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