Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

splice usage may be incorrect if it is possible for the source to be mutated #116451

Open
safinaskar opened this issue Oct 5, 2023 · 6 comments
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. O-linux Operating system: Linux S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@safinaskar
Copy link
Contributor

safinaskar commented Oct 5, 2023

It seems std::io::copy sometimes invokes splice on Linux. Don't do this! Using splice can lead to data corruption (one user reported that his backups was damaged). Also, Linus Torvalds said that splice should not be used as general-purpose copy tool. See details here: systemd/systemd#29044 . There are situations where splice is okay, but it is not good as general-purpose copy mechanism. Use of sendfile and copy_file_range should be questioned, too. Any other use of splice in std should be audited, too

Rust version: 90f3a6f

@safinaskar safinaskar added the C-bug Category: This is a bug. label Oct 5, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 5, 2023
@the8472
Copy link
Member

the8472 commented Oct 5, 2023

#108283 already reduced the use of splice. Do you have a concrete scenario in mind that I missed?

@safinaskar
Copy link
Contributor Author

@rustbot label +O-linux +T-libs

@rustbot rustbot added O-linux Operating system: Linux T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 5, 2023
@safinaskar
Copy link
Contributor Author

safinaskar commented Oct 5, 2023

Oops, I missed that pull request. Still I suggest reading lkml thread I mention ( https://lore.kernel.org/lkml/[email protected]/ ). It happened after the8472's PR. My understanding of Linus words is so: all uses of splice can cause all kinds of problems, so it should be used only when you fully control your whole data pipeline.

So, okay, feel free to close this bug report if you think that the work is already done

@the8472
Copy link
Member

the8472 commented Oct 5, 2023

My understanding of Linus words is so: all uses of splice can cause all kinds of problems

That does not match my understanding because there are multiple scenarios and at least two of them seem safe:

  • splicing into a pipe from a source that won't be mutated (such as a socket receive buffer)
  • splicing from a pipe (rather than into it), because either the data was copied into the pipe or the other end itself already opted into zero-copy behavior.

I don't see anything in the thread that directly contradicts this, but, well... people are arguing about a lot of things.

See https://lore.kernel.org/lkml/[email protected]/


I do wish Linus had provided a clearer answer on the semantics of SPLICE_F_MOVE. Because I previously believed that all the problematic behavior could only happen when it is set (which Rust never did).

The documentation continues to be misleading or confusing:

    SPLICE_F_MOVE
          Attempt to move pages instead of copying.  This is only a
          hint to the kernel: pages may still be copied if the
          kernel cannot move the pages from the pipe, or if the pipe
          buffers don't refer to full pages.  The initial
          implementation of this flag was buggy: therefore starting
          in Linux 2.6.21 it is a no-op (but is still permitted in a
          splice() call); in the future, a correct implementation
          may be restored.

here it distinguishes between copying and moving and yet further down it says

   Though we talk of copying, actual copies are generally avoided.
   The kernel does this by implementing a pipe buffer as a set of
   reference-counted pointers to pages of kernel memory.  The kernel
   creates "copies" of pages in a buffer by creating new pointers
   (for the output buffer) referring to the pages, and increasing
   the reference counts for the pages: only pointers are copied, not
   the pages of the buffer.

so I guess there's "moving", "not-really-copying by passing references" and "real copy" and the difference between the first two is never clarified and currently does nothing anyway? 😩

I'd appreciate a SPLICE_F_BYTECOPY flag, just to avoid the userspace copies.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 5, 2023

I would like to see either

  • a clear answer from Linus on this
  • proof of corruption in Rust
  • a better-in-all-ways implementation that avoids splice

I have no problem with us doing whatever Linus says. It's his kernel, after all. But I don't think we should be reading his words like tea leaves.

@workingjubilee
Copy link
Member

I should explain what I mean by reading tea leaves. From that LKML thread, according to him, the documentation is basically wrong:

First off, when documentation and reality disagree, it's the
documentation that is garbage.

So I guess the only actual, correct documentation is whatever he just said, which is:

A splice from a file acts like a scatter-gather mmap() in the kernel.
It's the original intent, and it's the whole reason it's noticeably
faster than doing a write.

Now, do I then agree that splice() has turned out to be a nasty morass
of problems? Yes.

And I even agree that while I actually think that your test program
should output "new" (because that is the whole point of the exercise),
it also means that people who use splice() need to understand that,
and it's much too easy to get things wrong if you don't understand
that the whole point of splice is to act as a kind of ad-hoc in-kernel
mmap thing.

And to make matters worse, for mmap() we actually do have some
coherency helpers. For splice(), the page ref stays around.

and

The ONLY reason for splice() existing is for zero-copy. If you don't
want zero-copy (aka "copy by reference"), don't use splice.

...

And yes, this has been documented forever. It may not have been
documented on the first line, because IT WAS SO OBVIOUS. The whole
reason splice() is fast is because it avoids the actual copy, and does
a copy-by-reference.

This suggests the first case described by the8472 is basically correct usage.

But Linus also says:

Another use-case would be if you want to send the same stable stream
to two different network connections, while still only having one
copy. You can't do that with plain splice() - because the data isn't
guaranteed to be stable, and the two network connections might see
different streams. You can't do that with the 'mmap and then
write-to-socket' approach, because the two writes not only copy twice,
they might copy different data.

And Linus acknowledged this suggestion by Matt Whitlock:

On Wed, 19 Jul 2023 at 16:41, Matt Whitlock [email protected] wrote:

"If out_fd refers to a socket or pipe with zero-copy support, callers must
ensure the transferred portions of the file referred to by in_fd remain
unmodified until the reader on the other end of out_fd has consumed the
transferred data."

That is a clear warning of the perils of the implementation under the hood,
and it could/should be copied, more or less verbatim, to splice(2).

Ack. Internally in the kernel, the two really have always been more or
less of intermingled.

This might complicate the second case? It feels pointlessly ambiguous, though. Linus shouldn't nack arbitrary swathes of the documentation and then pretend it's clear what the documentation says.

@workingjubilee workingjubilee changed the title Don't use splice in std::io::copy: data corruption reported splice usage may be incorrect if it is possible for the source to be mutated Oct 5, 2023
@workingjubilee workingjubilee added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. O-linux Operating system: Linux S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants