Bug 229616 - bin/dd: add conv=fsync, handle errors in close(2)
Summary: bin/dd: add conv=fsync, handle errors in close(2)
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: Normal Affects Many People
Assignee: freebsd-bugs mailing list
URL:
Keywords: easy, needs-qa, patch
Depends on:
Blocks:
 
Reported: 2018-07-08 21:27 UTC by Thomas Hurst
Modified: 2019-08-22 20:34 UTC (History)
4 users (show)

See Also:
koobs: mfc-stable11?
koobs: mfc-stable12?


Attachments
Patch against head/bin/dd/dd.c (342 bytes, patch)
2019-05-22 01:41 UTC, Thomas Hurst
no flags Details | Diff
Patch against head/bin/dd (2.47 KB, patch)
2019-08-19 02:39 UTC, Thomas Hurst
no flags Details | Diff
Patch against releng/12/bin/dd (2.47 KB, patch)
2019-08-19 02:39 UTC, Thomas Hurst
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Hurst 2018-07-08 21:27:30 UTC
In dd's main():

	dd_close();
	/*
	 * Some devices such as cfi(4) may perform significant amounts
	 * of work when a write descriptor is closed.  Close the out
	 * descriptor explicitly so that the summary handler (called
	 * from an atexit() hook) includes this work.
	 */
	close(out.fd);
	exit(0);

dd_close() comments *claim* it flushes output, but this appears to be a lie - it just finishes off any pending calls to write(), it doesn't ask they be flushed to disk.

IO errors from previous writes that were later flushed to disk can end up reported in close(), for example as mentioned in close(2):

    [ENOSPC]           The underlying object did not fit, cached data was lost

Obviously a dd that can exit 0 while also losing data is.. unfortunate.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-05-21 04:28:05 UTC
Given the potential for data loss, and the relative, apparently, simplicity of addressing it such as checking for close(2) succeeds or not, mark this as importance:high

CC original committer of code block (base r249063) who may know the best course of action, or someone who might.
Comment 2 Thomas Hurst 2019-05-22 01:41:34 UTC
Created attachment 204530 [details]
Patch against head/bin/dd/dd.c
Comment 3 Thomas Hurst 2019-08-18 16:24:49 UTC
So, did a bit of research to see what everyone else is doing.

NetBSD:

 * Checks close() on output
 * fsync() on output if it's stdout (where close() may simply decrement a refcount)

OpenBSD:

 * Does not close() at all
 * Supports conv=fsync

Illumos:

 * Checks close() on output, and also fclose(stdout)
 * Supports conv=fsync and fdatasync
 * Supports oflag=sync and dsync for synchronous writes

Coreutils:

 * Checks close() on both input and output
 * Checks ferror()/fclose() on stdout if arg parsing fails
 * Supports conv=fsync and fdatasync
 * Supports oflag=sync, dsync, and direct
 * EINTR-checking loops for close() and f*sync()

So FreeBSD's bit of an outlier, being the only dd without any fsync support at all, and calling close() explicitly but not checking it succeeds.

I'm thinking we could do a hybrid of NetBSD and OpenBSD's approaches:

1. Check close()
2. Also check fsync() if output is stdout
3. Add conv=fsync support

Other options might be nice to have, but aren't needed to ensure data is on stable storage after dd is finished, which is the most important point.
Comment 4 Thomas Hurst 2019-08-19 02:39:20 UTC
Created attachment 206684 [details]
Patch against head/bin/dd

This adds conv=fsync, implicit fsync when output is stdout, and checking of close(2) return code.
Comment 5 Thomas Hurst 2019-08-19 02:39:59 UTC
Created attachment 206685 [details]
Patch against releng/12/bin/dd
Comment 6 Kyle Evans freebsd_committer 2019-08-19 15:52:54 UTC
CC imp@ for dd(1) patch
Comment 7 Jilles Tjoelker freebsd_committer 2019-08-22 20:29:37 UTC
I agree with checking close() and conv=fsync/fdatasync but not implicit fsync(). Implicit fsync might cause unexpected poor performance.

For conv=fsync see review https://reviews.freebsd.org/D21370
Comment 8 Warner Losh freebsd_committer 2019-08-22 20:34:01 UTC
This patch looks better than the code review posted.