zfs promote .../%recv should be an error #6339

loli10K · 2017-07-12T05:15:50Z

Description

Add more input validation in zfs_ioc_promote() (and its userland counterpart) so we don't promote temporary ".../%recv" datasets.

NOTE: I have not added tests to cover my changes yet: should we update "zfs_promote_006_neg.ksh" ('zfs promote' will fail with invalid arguments) or add a new script?

Motivation and Context

Fix #4843

If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child).

Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename().

How Has This Been Tested?

POOLNAME='testpool'
TMPDIR='/var/tmp'
mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
zpool destroy $POOLNAME
rm -f $TMPDIR/zpool.dat
fallocate -l 256m $TMPDIR/zpool.dat
zpool create -O mountpoint=$TMPDIR/$POOLNAME $POOLNAME $TMPDIR/zpool.dat
#
zfs create $POOLNAME/src -o mountpoint=$TMPDIR/$POOLNAME/src
#
dd if=/dev/urandom of=/$TMPDIR/$POOLNAME/src/file.dat bs=1M count=10
zfs snap $POOLNAME/src@snap1
#
dd if=/dev/urandom of=/$TMPDIR/$POOLNAME/src/file.dat bs=1M count=10
zfs snap $POOLNAME/src@snap2
#
dd if=/dev/urandom of=/$TMPDIR/$POOLNAME/src/file.dat bs=1M count=10
zfs snap $POOLNAME/src@snap3
#
zfs send -p $POOLNAME/src@snap1 | zfs recv -vu $POOLNAME/dst
zfs send -pI $POOLNAME/src@snap1 $POOLNAME/src@snap3 | dd bs=1M count=15 iflag=fullblock | zfs recv -svu $POOLNAME/dst
#
zfs get name,inconsistent,mountpoint,origin $POOLNAME/dst/%recv
#
zfs promote $POOLNAME/dst/%recv

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)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

behlendorf

I'd prefer to extended the existing zfs_promote_006_neg.ksh test case. It wouldn't be a terrible idea to extend the other *_neg test cases where a '%' in the name isn't allowed to check for this.

behlendorf · 2017-07-12T18:49:26Z

lib/libzfs/libzfs_dataset.c

@@ -3751,6 +3751,10 @@ zfs_promote(zfs_handle_t *zhp)
 		return (zfs_error(hdl, EZFS_BADTYPE, errbuf));
 	}

+	if (!zfs_validate_name(hdl, zhp->zfs_name, zhp->zfs_type, B_TRUE)) {
+		return (zfs_error(hdl, EZFS_INVALIDNAME, errbuf));
+	}


nit: no need for the { } here.

loli10K · 2017-07-14T18:25:33Z

@behlendorf i'll update zfs_promote_006_neg.ksh and the other nit ASAP: I'm still trying to fix kstat collisions.

behlendorf

@loli10K no problem, and don't worry I know this isn't quite right to merge. I've reverted the approved status for now.

dinatale2 · 2017-07-15T00:27:10Z

tests/zfs-tests/tests/functional/cli_root/zfs_promote/zfs_promote_006_neg.ksh

@@ -50,30 +51,44 @@
 verify_runnable "both"

 snap=$TESTPOOL/$TESTFS@$TESTSNAP
+incr=$TESTPOOL/$TESTFS@incr$TESTSNAP
+clone=$TESTPOOL/$TESTCLONE
+destfs=$TESTPOOL/destfs


nit: maybe recvfs instead of destfs?

recvfs is objectively more appropriate here

dinatale2

Thanks for the corrections! Noticed one more thing. Do you mind squashing your commits as well?

dinatale2 · 2017-07-15T22:15:10Z

tests/zfs-tests/include/libtest.shlib

+	typeset mountpoint="$TESTDIR/create_recv_clone"
+	typeset sendfile="$TESTDIR/create_recv_clone.zsnap"
+
+	 [[ -z $recvfs ]] && log_fail "Recv filesystem's name is undefined."


You could enforce recvfs doesn't exist by calling datasetexists and failing if that returns true. Likewise with sendfs, you'd want to make sure it doesn't exist.

behlendorf · 2017-07-25T17:32:54Z

@loli10K when you get a chance could you rebase this on master.

If we are in the middle of an incremental 'zfs receive', the child .../%recv will exist. If we run 'zfs promote' .../%recv, it will "work", but then zfs gets confused about the status of the new dataset. Attempting to do this promote should be an error. Similarly renaming .../%recv datasets should not be allowed. Signed-off-by: loli10K <ezomori.nozomu@gmail.com>

behlendorf

Thanks for rebasing this and extending the test coverage. This LGTM!

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com> git-svn-id: svn+ssh://svn.freebsd.org/base/head@329783 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com>

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com> git-svn-id: https://svn.freebsd.org/base/vendor-sys/illumos/dist@329781 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com> git-svn-id: https://svn.freebsd.org/base/head@329783 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com> git-svn-id: https://svn.freebsd.org/base/vendor/illumos/dist@329781 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com> git-svn-id: svn+ssh://svn.freebsd.org/base/head@329783 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com>

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com> git-svn-id: https://svn.freebsd.org/base/stable/11@332535 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

illumos/illumos-gate@add927f Reported on the ZFSonLinux openzfs/zfs#4843, fixed by openzfs/zfs#6339: If we are in the middle of an incremental zfs receive, the child .../%recv will exist. If you concurrently run zfs promote .../%recv, it will "work", but then zfs gets confused. For example, there's no obvious way to destroy the containing filesystem (because it is now a clone of its invisible child). Attempting to do this promote should be an error. We could fix this by having zfs_ioc_promote() check if zc_name contains a %, similar to zfs_ioc_rename(). Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Matthew Ahrens <mahrens@delphix.com> Approved by: Dan McDonald <danmcd@joyent.com> Author: loli10K <ezomori.nozomu@gmail.com>

If we are in the middle of an incremental 'zfs receive', the child .../%recv will exist. If we run 'zfs promote' .../%recv, it will "work", but then zfs gets confused about the status of the new dataset. Attempting to do this promote should be an error. Similarly renaming .../%recv datasets should not be allowed. Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: loli10K <ezomori.nozomu@gmail.com> Closes openzfs#4843 Closes openzfs#6339

behlendorf requested changes Jul 12, 2017

View reviewed changes

behlendorf approved these changes Jul 14, 2017

View reviewed changes

behlendorf requested changes Jul 14, 2017

View reviewed changes

loli10K force-pushed the issue-4843 branch from 6a807b8 to 2a96bb6 Compare July 14, 2017 20:18

dinatale2 suggested changes Jul 15, 2017

View reviewed changes

loli10K force-pushed the issue-4843 branch from 846f38d to 945ecee Compare July 15, 2017 22:31

dinatale2 approved these changes Jul 18, 2017

View reviewed changes

loli10K force-pushed the issue-4843 branch from 945ecee to 778f2cf Compare July 25, 2017 17:34

behlendorf approved these changes Jul 28, 2017

View reviewed changes

behlendorf merged commit 650258d into openzfs:master Jul 28, 2017

loli10K deleted the issue-4843 branch August 3, 2017 17:59

loli10K mentioned this pull request Jan 3, 2018

8942 zfs promote .../%recv should be an error openzfs/openzfs#512

Closed

loli10K restored the issue-4843 branch November 1, 2018 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zfs promote .../%recv should be an error #6339

zfs promote .../%recv should be an error #6339

loli10K commented Jul 12, 2017

behlendorf left a comment

behlendorf Jul 12, 2017

loli10K commented Jul 14, 2017

behlendorf left a comment

dinatale2 Jul 15, 2017

loli10K Jul 15, 2017

dinatale2 left a comment •

edited

Loading

dinatale2 Jul 15, 2017

behlendorf commented Jul 25, 2017

behlendorf left a comment

zfs promote .../%recv should be an error #6339

zfs promote .../%recv should be an error #6339

Conversation

loli10K commented Jul 12, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Jul 12, 2017

Choose a reason for hiding this comment

loli10K commented Jul 14, 2017

behlendorf left a comment

Choose a reason for hiding this comment

dinatale2 Jul 15, 2017

Choose a reason for hiding this comment

loli10K Jul 15, 2017

Choose a reason for hiding this comment

dinatale2 left a comment • edited Loading

Choose a reason for hiding this comment

dinatale2 Jul 15, 2017

Choose a reason for hiding this comment

behlendorf commented Jul 25, 2017

behlendorf left a comment

Choose a reason for hiding this comment

dinatale2 left a comment •

edited

Loading