Allow inherited properties in zfs_check_settable()

[loli10K]

Motivation and Context

Some properties ("checksum", "dedup") cause issues when received with zfs recv -o:

root@debug:~# modinfo -F version zfs
0.7.0-1489_gdae3e9ea2
root@debug:~# # setup
root@debug:~# POOLNAME='testpool'
root@debug:~# TMPDIR='/var/tmp'
root@debug:~# mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
root@debug:~# zpool destroy $POOLNAME
root@debug:~# rm -f $TMPDIR/disk
root@debug:~# truncate -s 128m $TMPDIR/disk
root@debug:~# zpool create $POOLNAME $TMPDIR/disk
root@debug:~# #
root@debug:~# zfs create $POOLNAME/send
root@debug:~# zfs create $POOLNAME/send/sub
root@debug:~# zfs snap -r $POOLNAME/send@snap
root@debug:~# zfs send -R $POOLNAME/send@snap | zfs recv -Fv -o checksum=sha512 $POOLNAME/recv
receiving full stream of testpool/send@snap into testpool/recv@snap
received 46.4K stream in 1 seconds (46.4K/sec)
receiving full stream of testpool/send/sub@snap into testpool/recv/sub@snap
internal error: Invalid argument
Aborted (core dumped)

Fix #7755
Fix #7576

Description

This change modifies how 'checksum' and 'dedup' properties are verified in zfs_check_settable() handling the case where they are explicitly inherited (nvpair type = DATA_TYPE_BOOLEAN) in the dataset hierarchy when receiving a recursive send stream.

How Has This Been Tested?

Used systemtap to find where the error is coming from, tested fix on local development Debian8 running the updated functional/cli_root/zfs_receive/receive-o-x_props_override test case.

Slightly off topic question, does anyone know how to inspect nvlist/nvpair data structures from systemtap/dtrace?

root@debug:~# stap -e '
probe
module("zfs").function("zfs_set_prop_nvlist").call,
module("zfs").function("zfs_check_settable").call {
   printf(" -> %s %s\n", ppfunc(), $$parms$);
}
probe
module("zfs").function("zfs_set_prop_nvlist").return,
module("zfs").function("zfs_check_settable").return {
   printf(" <- %s %s\n", ppfunc(), $$return$);
}'
 -> zfs_set_prop_nvlist dsname="testpool/recv" source=32 nvl={.nvl_version=0, .nvl_nvflag=1, .nvl_priv=18446612135649350336, .nvl_flag=0, .nvl_pad=0} errlist={.nvl_version=0, .nvl_nvflag=1, .nvl_priv=18446612135986611072, .nvl_flag=0, .nvl_pad=0}
 <- zfs_set_prop_nvlist return=0
[...]
 -> zfs_set_prop_nvlist dsname="testpool/recv/sub" source=16 nvl={.nvl_version=0, .nvl_nvflag=1, .nvl_priv=18446612135487296384, .nvl_flag=0, .nvl_pad=0} errlist={.nvl_version=0, .nvl_nvflag=1, .nvl_priv=18446612135649339840, .nvl_flag=0, .nvl_pad=0}
 -> zfs_check_settable dsname="testpool/recv/sub" pair={.nvp_size=32, .nvp_name_sz=9, .nvp_reserve=0, .nvp_value_elem=0, .nvp_type=1} cr={.usage={...}, .uid={...}, .gid={...}, .suid={...}, .sgid={...}, .euid={...}, .egid={...}, .fsuid={...}, .fsgid={...}, .securebits=0, .cap_inheritable={...}, .cap_permitted={...}, .cap_effective={...}, .cap_bset={...}, .cap_ambient={...}, .jit_keyring='\000', .session_keyring=0x0, .process_keyring=0x0, .thread_keyring=0x0, .request_key_auth=0x0, .security=0x0, .user=0xffffffff81a41580, .user_ns
 <- zfs_check_settable return=22
 <- zfs_set_prop_nvlist return=22

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.

[codecov]

Codecov Report

Merging #7757 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #7757      +/-   ##
==========================================
+ Coverage   78.46%   78.46%   +<.01%     
==========================================
  Files         368      368              
  Lines      112301   112142     -159     
==========================================
- Hits        88114    87995     -119     
+ Misses      24187    24147      -40

Flag	Coverage Δ
#kernel	78.86% <77.77%> (+15.49%)	⬆️
#user	67.51% <ø> (-0.17%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 492f64e...bf425ad. Read the comment docs.

[tcaputi]

This patch looks good and definitely seems to solve the problem you found. I think we may have some further work to do here, in that I don't see any place where nvpair type validation is done. zfs_check_settable() seems to assume (for all properties it looks at) that it will be the correct type and if not it defaults to allow it, which seems non-ideal. I am also aware of some things you can do with zfs set that will allow you to avoid some value checks.

[loli10K]

I am also aware of some things you can do with zfs set that will allow you to avoid some value checks.

@tcaputi are you referring to #5229 or something else?

[tcaputi]

@loli10K Yes, that is what I was remembering. I'm not sure how relevant it is to this PR, but I thought I'd mention it.

[behlendorf]

nit: could we instead structure this in the same way as the other case blocks. The explicit check for DATA_TYPE_BOOLEAN I find a little non-intuitive.

                if (nvpair_type(pair) == DATA_TYPE_UINT64 &&
                    nvpair_value_uint64(pair, &intval) == 0) {
                        ...
                }

[rincebrain]

Any chance this could get backported for an 0.7.X point release, so we don't have to wait until 0.8.0 to do -x checksum? :)

[tonyhutter]

@behlendorf is this low-risk enough to include in 0.7.10? I'm putting together the patchset right now.

[behlendorf]

@tonyhutter yes.

[tonyhutter]

@behlendorf ok I'll include it.