Race condition between 'zpool export' and 'zfs create' can crash the latter

[loli10K]

Description

Discovered while investigating another issue: if we manage to export the pool on which we are creating a dataset (filesystem or zvol) between libzfs`zfs_create() entrypoint and libzfs`zpool_open() call (for which we never check the return value) we end up dereferencing a NULL pointer in libzfs`zpool_close()

Motivation and Context

root@debian-8-zfs:~# dmesg
[  152.674516] zfs[25793]: segfault at 120 ip 00007f537848cc44 sp 00007ffdb35037f0 error 4 in libzfs.so.2.0.0[7f537846e000+46000]
[  153.855647] zfs[26429]: segfault at 120 ip 00007fceb10f9c44 sp 00007ffc398a3c40 error 4 in libzfs.so.2.0.0[7fceb10db000+46000]
root@debian-8-zfs:~# gdb -q `which zfs` ./core 
Reading symbols from /usr/sbin/zfs...done.
[New LWP 26429]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `zfs create testpool/fs'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  zpool_close (zhp=0x0) at libzfs_pool.c:1117
1117		nvlist_free(zhp->zpool_config);
(gdb) bt
#0  zpool_close (zhp=0x0) at libzfs_pool.c:1117
#1  0x00007fceb10ed2cf in zfs_create (hdl=0xbeb060, path=0x7ffc398a4e98 "testpool/fs", type=ZFS_TYPE_FILESYSTEM, props=0xbebff0) at libzfs_dataset.c:3419
#2  0x000000000040da88 in zfs_do_create (argc=1, argv=0x7ffc398a4858) at zfs_main.c:913
#3  0x0000000000404de8 in main (argc=3, argv=0x7ffc398a4848) at zfs_main.c:7081
(gdb) list
1112	 * Close the handle.  Simply frees the memory associated with the handle.
1113	 */
1114	void
1115	zpool_close(zpool_handle_t *zhp)
1116	{
1117		nvlist_free(zhp->zpool_config);
1118		nvlist_free(zhp->zpool_old_config);
1119		nvlist_free(zhp->zpool_props);
1120		free(zhp);
1121	}
(gdb) p zhp->zpool_config
Cannot access memory at address 0x120
(gdb) 

Minimal reproducer (requires systemtap):

POOLNAME='testpool'
TMPDIR='/var/tmp'
mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
zpool destroy $POOLNAME
rm -f $TMPDIR/zpool.dat
fallocate -l 128m $TMPDIR/zpool.dat
zpool create -O mountpoint=none $POOLNAME $TMPDIR/zpool.dat
#
stap -g -v -d `which zfs` --ldd \
   -e '
global probe_enable = 0
probe process("/lib*/libzfs.so*").function("zfs_create").call
{
   probe_enable = 1
}
probe process("/lib*/libzfs.so*").function("zfs_dataset_exists").return
{
   if (probe_enable) {
      system("zpool export testpool");
      mdelay(5000);
   }
}
' -c 'zfs create testpool/fs'
#
gdb -q -batch `which zfs` ./core -ex 'bt'

It would be nice if we could leverage systemtap (or any other equivalent technology) to add more regression tests since it's much easier to simulate race conditions this way.

Also i wonder if we should try to bring this change to Illumos since they are affected too:

[root@52-54-00-d3-7a-01 /cores]# mdb core.zfs.4244 
Loading modules: [ libumem.so.1 libc.so.1 libtopo.so.1 libavl.so.1 libnvpair.so.1 ld.so.1 ]
> ::stack
libzfs.so.1`zpool_close+0x17(0, 0, 0, 8047450)
libzfs.so.1`zfs_create+0x1bb(8090548, 8047e6f, 1, 808cba8)
zfs_do_create+0x545(2, 8047d74, 80778a0, 801, 0, 3)
main+0x22c(8047d2c, fef5c6e8, 8047d64, 8055a17, 3, 8047d70)
_start+0x83(3, 8047e64, 8047e68, 8047e6f, 0, 8047e7b)
> 

How Has This Been Tested?

Tested manually (with systemtap to reproduce the race condition)

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)

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]

It would be nice if we could leverage systemtap (or any other equivalent technology) to add more regression tests since it's much easier to simulate race conditions this way.

There's nothing stopping us from adding test cases which require systemtap. There are already a few test cases we've added from Illumos which require dtrace. It simply that whenever possible we want to keep the test cases as portable as possible.

Also i wonder if we should try to bring this change to Illumos since they are affected too:

Definitely. For a non controversial change like this you can open a matching PR against OpenZFS on GitHub.