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

Added error for writing to /dev/ on Linux. #11991

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Conversation

rincebrain
Copy link
Contributor

@rincebrain rincebrain commented May 3, 2021

Motivation and Context

Starting in Linux 5.10, trying to write to /dev/ files errors out.
Prefer to inform people when this happens rather than hoping they
guess what's wrong.

Description

Plumbed an error check in for all the zfs_send_* calls in zfs_do_send, that checks

  • am I on Linux?
  • did I just get back EINVAL?
  • is stdout /dev/zero or /dev/null?
    and if all of the above are true, prints an additional message.

(I also had an implementation that just detects and rejects attempting to write /dev on
Linux, but I like this approach better. If people disagree, I'll open that PR instead.)

How Has This Been Tested?

Lightly on my Debian bullseye VM where #11445 was reproducing before; it prints the correct message on trying to overwrite dev nodes in /dev, not for attempting to write things in /proc or /sys or on the root FS.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 6, 2021
@rincebrain
Copy link
Contributor Author

Ack, I just found another case - zfs send -R [snapshot] > /dev/null dies with internal error: warning: cannot send [...]: Invalid argument then Aborted, which is nasty, since it means we're dying before we come back to zfs_do_send().

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind rebasing this cleanly on the latest master branch. I think it makes sense to add these error message for now. It may turn out that #11992 is a better long term fix for this, but I don't think that needs to prevent us from making this improvement now.

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
@rincebrain
Copy link
Contributor Author

There, a fresh commit atop latest master. (I have, admittedly, not tested the result at all, but the merge required no manual massaging at all, so I'm reasonably confident it won't secretly burn the fields and salt the earth.)

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
@rincebrain rincebrain force-pushed the 11445_1 branch 2 times, most recently from 45f4283 to 2224d2d Compare May 27, 2021 15:08
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this. Looks like it just needs another rebase.

@rincebrain rincebrain force-pushed the 11445_1 branch 2 times, most recently from 83fa3f7 to b536b38 Compare June 5, 2021 09:10
@rincebrain
Copy link
Contributor Author

I only realized when I went to rebase all my PRs after the fun with #12072 that my pushing a rebase wouldn't have sent any notification, so, done.

@behlendorf
Copy link
Contributor

@aerusso since you noted this issue originally I thought you might have a few minutes to review this stop gap. It's not quite a fix, but the least we can do is properly detect the issue and warn people.

@aerusso
Copy link
Contributor

aerusso commented Jun 8, 2021

This looks straightforward. The only thing I can suggest is that the error message,

Error: Stream cannot be written to /dev/{null,zero} files.

sounds like a problem with the system (user: "Why the heck can't the stream be written to /dev/null?!"). I suggest something like

Error: Stream output to /dev/{null,zero} files is not implemented.

which makes it very clear that it's no fault of the user.

@rincebrain
Copy link
Contributor Author

rincebrain commented Jun 8, 2021

This looks straightforward. The only thing I can suggest is that the error message,

Error: Stream cannot be written to /dev/{null,zero} files.

sounds like a problem with the system (user: "Why the heck can't the stream be written to /dev/null?!"). I suggest something like

Error: Stream output to /dev/{null,zero} files is not implemented.

which makes it very clear that it's no fault of the user.

That gives me a thought - how would you feel about something like:

Error: Stream output to /dev/{null,zero} files is not implemented.
(As a workaround, try "zfs send [...] | cat > /dev/null")

?

That way, we can minimize the number of people objecting to this/posting bugs about this, until some more permanent fix is merged.

Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Signed-off-by: Rich Ercolani <[email protected]>
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated error message looks good to me. Thanks.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 9, 2021
@jwk404 jwk404 merged commit 860051f into openzfs:master Jun 10, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Kennedy <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes:  openzfs#11991
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Kennedy <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes:  openzfs#11991
rincebrain added a commit to rincebrain/zfs that referenced this pull request Oct 28, 2021
Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Kennedy <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes:  openzfs#11991
tonyhutter pushed a commit that referenced this pull request Nov 1, 2021
Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Kennedy <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes:  #11991
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Kennedy <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes:  openzfs#11991
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
Starting in Linux 5.10, trying to write to /dev/{null,zero} errors out.
Prefer to inform people when this happens rather than hoping they guess
what's wrong.

Reviewed-by: Antonio Russo <[email protected]>
Reviewed-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Kennedy <[email protected]>
Signed-off-by: Rich Ercolani <[email protected]>
Closes:  openzfs#11991
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants