Page MenuHomeFreeBSD

sendfile: Return the object size from sendfile_getobj()
AcceptedPublic

Authored by markj on Fri, Feb 19, 9:46 PM.

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 37353
Build 34242: arc lint + arc unit

Event Timeline

markj requested review of this revision.Fri, Feb 19, 9:46 PM
markj added reviewers: glebius, kib, chs.
markj added a subscriber: dumbbell.
This revision is now accepted and ready to land.Sat, Feb 20, 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.Wed, Feb 24, 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.Wed, Feb 24, 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.Wed, Feb 24, 7:13 PM
This revision is now accepted and ready to land.Wed, Feb 24, 7:33 PM