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: Matt Macy
URL:
Keywords: easy
Depends on:
Blocks:
 
Reported: 2018-07-08 21:27 UTC by Thomas Hurst
Modified: 2019-10-01 02:28 UTC (History)
4 users (show)

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


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.
Comment 9 commit-hook freebsd_committer 2019-09-30 21:54:24 UTC
A commit references this bug:

Author: mmacy
Date: Mon Sep 30 21:53:26 UTC 2019
New revision: 352921
URL: https://svnweb.freebsd.org/changeset/base/352921

Log:
  dd: Check result of close(2) for errors

  close(2) can return errors from previous operations which should not be ignored.

  PR: 229616
  Submitted by:	Thomas Hurst
  Reported by:	Thomas Hurst
  Reviewed by:	mmacy@
  Obtained from:	Ryan Moeller
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D21376

Changes:
  head/bin/dd/dd.c
Comment 10 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-01 02:28:16 UTC
If this (or any other issue, eg: the other recent dd changes) should be merged to releng/12.1, please add bug 240700 to this issues Blocks field

Merges should also include this PR: <ID> in the commit log messages for tracking purposes