> On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > I'm not very familiar with the internals of the ZFS kernel module, so I > > focused myself to the manual pages, SCSI code, and the related changes > > there. Hopefully this is useful.
It most definitely is! Some of your suggestions left multiple avenues open, so I'd like to get your take before I post the corrections to all the items in one big wad. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/fs/zfs/zvol.c, line 1788 > > <https://reviews.csiden.org/r/263/diff/9/?file=18554#file18554line1788> > > > > This is misleading. EFAULT shouldn't be returned in the case of > > overflow. I'm not following. We are using EFAULT here exactly as before when we were using ddi_copyin. I've only replaced ddi_copyin with dfl_copyin to auto-determine the amount we need to copy. Under the hood, it's still ddi_copyin's resposibility to check if the user space buffer passed to us here is sufficiently large to copy from. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/sys/dkioc_free_util.h, line 25 > > <https://reviews.csiden.org/r/263/diff/9/?file=18563#file18563line25> > > > > These should probably be wrapped in an ifdef _KERNEL block. Good point, fixed. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/sys/dkio.h, line 559 > > <https://reviews.csiden.org/r/263/diff/9/?file=18562#file18562line559> > > > > Can we not hardcode 16 and instead use the sizoef > > (dkioc_free_list_ext_t) which is where this seems to be derived from? Done. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/sys/dkio.h, line 550 > > <https://reviews.csiden.org/r/263/diff/9/?file=18562#file18562line550> > > > > Seems like the commend should be updated or the code should. I don't > > know if this is still relevant or if it is, seems like we should wrap it up > > in an ifdef debug, given that the only callers seem to use it that way. I've removed the "remove before GA" comment, because it's not valid anymore. I'd like to keep these two fields there. It allows one to run a non-debug kernel with any combination of debug ZFS & sd modules and not have the structure change size. IMO we can afford to waste the 16 bytes here to simplify testing and not have the worry about blowing up the kernel in various hard to debug ways. Worst case, sd or zfs will simply ignore the fields. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/os/dkioc_free_util.c, line 50 > > <https://reviews.csiden.org/r/263/diff/9/?file=18560#file18560line50> > > > > In general, prefer explicit comparisons here, as in the ddi_copyin != 0. Changed. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/os/dkioc_free_util.c, line 42 > > <https://reviews.csiden.org/r/263/diff/9/?file=18560#file18560line42> > > > > Why are we trusting the kernel on overflow? Given that you're passing > > in the ddi_flags into both cases, seems like you can eliminate the FKIOCTL > > case and simply look at it around the assignment at +62-64. Added a check for the kernel case as well. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/os/dkioc_free_util.c, line 35 > > <https://reviews.csiden.org/r/263/diff/9/?file=18560#file18560line35> > > > > I'm not sure we want to have allocation success or failure be the only > > return code, given that this can overflow. It seems most callers of this > > are then straight up returning EFAULT, which would confuse anyone trying to > > debug this. I don't think it's really possible to mess it up quite as bad, as even user space is supposed to use DFL_SZ from dkio.h for this. I don't feel strongly either way, but I'd prefer to keep the interface simple(r) rather than with trying to split hairs about specific caller coding mistakes. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/io/scsi/targets/sd.c, line 23511 > > <https://reviews.csiden.org/r/263/diff/9/?file=18559#file18559line23511> > > > > It'd be good if kernel space ioctls didn't cause us to panic either! Is > > there a reason we're not failing it in that case? This is an oversight on my part. How about we make kernel callers more stringent and panic on a NULL value? I typically hold kernel code to a higher standard, but I don't feel strongly. I can either drop the FKIOCTL check (so kernel callers will get EINVAL) or add an assertion to check that kernel callers never supply NULL. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/fs/zfs/zvol.c, line 1795 > > <https://reviews.csiden.org/r/263/diff/9/?file=18554#file18554line1795> > > > > In the case where this comes from a buggy kernel caller we seem to just > > blindly trust the size and not bound it, meaning we could easily overflow > > and look at garbage data. What prevents that from happening? Don't we need > > to still be checking that the number of entries isn't pathological? Good idea. Do you think we should VERIFY here or just return EINVAL to the kernel caller? I like the idea of enforcing this more firmly in the case of kernel code. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/sys/dkio.h, line 558 > > <https://reviews.csiden.org/r/263/diff/9/?file=18562#file18562line558> > > > > In a lot of places we seem to just be using DFL_SZ to perform memory > > allocations, but it's not clear to me how we've insured that there's no > > overflow of the memory allocation buffer there. Well we can only check when copying in from userspace (and even then only grossly mis-sized buffers). For kernel, we have no other way of knowing than by the field passed in dfl_num_exts. The added sanity checks of dfl_num_exts for kernel callers you suggested will definitely help. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/os/dkioc_free_util.c, line 87 > > <https://reviews.csiden.org/r/263/diff/9/?file=18560#file18560line87> > > > > Presumably it's just as important that userland catch overflow > > conditions as the kernel, due to how a lot of tets are run. Indeed, added the check. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/os/dkioc_free_util.c, line 77 > > <https://reviews.csiden.org/r/263/diff/9/?file=18560#file18560line77> > > > > If this file really needs to be shared between userland and the kernel > > then it shouldn't be sitting in uts/common/os and should instead be moved > > into somewhere under common/. It's only ever used for ztest, where we need a dfl_copyin implementation, since ztest directly compiles and runs kernel module code. IOW, it's "kinda" kernel code, but invoked in a special context. In that sense, it's no more userland than e.g. vdev.c or spa.c. This is never called directly from pure userland programs such as administrative tools, but instead only ever invoked from other kernel code. > On Aug. 7, 2016, 6:40 p.m., Robert Mustacchi wrote: > > usr/src/uts/common/io/scsi/targets/sd.c, line 21757 > > <https://reviews.csiden.org/r/263/diff/9/?file=18559#file18559line21757> > > > > This return code isn't always correct in the case of overflow. Well it kind of is. If you didn't pass us enough buffer to the amount of data you've indicated, then it's still a bad buffer you're asking us to copy. That having been said, I don't think mis-sized buffers are gonna really occur, since callers of DKIOCFREE also should use DFL_SZ to allocate. I think the more common case would be either passing NULL (for which we would return EINVAL) or something completely crazy, like a non-address value or something... - Saso ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/263/#review889 ----------------------------------------------------------- On Aug. 7, 2016, 7:40 a.m., Saso Kiselkov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/263/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2016, 7:40 a.m.) > > > Review request for OpenZFS Developer Mailing List and Christopher Siden. > > > Repository: illumos-gate > > > Description > ------- > > Adds support for issuing abstract DKIOCFREE ioctls from ZFS and adds support > for translating these ioctls into SCSI UNMAP commands to sd. > This is an upstream of work by Nexenta. > > > Diffs > ----- > > usr/src/uts/common/sys/fs/zfs.h 55f73868d6424270c50feec736a7149dec426546 > usr/src/uts/common/sys/dkio.h a5b0c312f9df59a7171778411ccaff654c5b27e8 > usr/src/uts/common/sys/Makefile d5dd20bff9f44fc0f6eb50d112a336c771a5a3a7 > usr/src/uts/common/io/sata/impl/sata.c > 66c141bf8309825a3087a2a60283b8471a5a139c > usr/src/uts/common/io/comstar/lu/stmf_sbd/sbd_scsi.c > cb6e115fe949145d39865333353ef50baf49c7da > usr/src/uts/common/fs/zfs/sys/zio_impl.h > a36749a308d675fb77fae43923304a954d97c086 > usr/src/uts/common/fs/zfs/sys/vdev.h > cd221e07ab2998b3594284acb45257359eb4b1a4 > usr/src/uts/common/fs/zfs/sys/metaslab_impl.h > 1c8993aca55ab0e9ceec1e3f20ede2c679efa8e4 > usr/src/uts/common/fs/zfs/sys/metaslab.h > f7271f08ad4e219e3f90c0216519d59d20e05032 > usr/src/uts/common/fs/zfs/sys/dmu.h > d68651b11635616989bc5ccfc1287012c3fbb668 > usr/src/uts/common/fs/zfs/zvol.c 95bb26c2119dc1a331a324be18db2391f29cd78f > usr/src/uts/common/fs/zfs/zio.c be7087d4db48ccdc92f21823b82059031ab09922 > usr/src/uts/common/fs/zfs/zfs_ioctl.c > e52b8f9b827b7d54087a65e6eeb19563614a27bc > usr/src/uts/common/fs/zfs/vdev_root.c > a5442a55eb5a23fbd6f5cf76f8b348670ad2e621 > usr/src/uts/common/fs/zfs/vdev_raidz.c > ff06896e8d7de034bb9d7d9d82379fa6fd26d7bd > usr/src/uts/common/fs/zfs/vdev_missing.c > 228757334234d241f980058397438d3a80716dcf > usr/src/uts/common/fs/zfs/vdev_label.c > 866046315cc157b92de2b44277bddee865a487b3 > usr/src/uts/common/fs/zfs/vdev_disk.c > 60075c4c15d8c2c9d916d69b02905196793b6f51 > usr/src/uts/common/fs/zfs/vdev.c d57c1b31f9cbba38d16bb709e4ee825c82340639 > usr/src/uts/common/sys/scsi/targets/sddef.h > 39c0ed9d0fb2c2d6c20fa793c3b5f9168a844552 > usr/src/uts/common/sys/dkioc_free_util.h PRE-CREATION > usr/src/uts/common/os/dkioc_free_util.c PRE-CREATION > usr/src/uts/common/io/scsi/targets/sd.c > ae1e7e0fc3e51957dd66158c960251550ed9890d > usr/src/uts/common/io/comstar/lu/stmf_sbd/stmf_sbd.h > efbc7268ea7aab11b1d726551058d38e71bf376d > usr/src/uts/common/io/comstar/lu/stmf_sbd/sbd.c > 81e63367c55e49e0ea30850ca19ccea3bafbe529 > usr/src/uts/common/fs/zfs/sys/zio.h > 25bb167147d2044a779a9a8e224a271ed6a5afc4 > usr/src/uts/common/fs/zfs/sys/vdev_impl.h > ff14aa80c8767d6d3d22879f27f0e0b1cc850f90 > usr/src/uts/common/fs/zfs/sys/spa_impl.h > b19863c4d8979f67d21d4707deedd8fc29422fd9 > usr/src/uts/common/fs/zfs/sys/range_tree.h > 9f3ead537165f3a7b8c52fe58eedef66c1b1952e > usr/src/uts/common/fs/zfs/vdev_mirror.c > b038ef6f67caeb552e64430fe903b44f270cb611 > usr/src/uts/common/fs/zfs/spa_misc.c > 7e00d9f42a152e8e3986cf23f660135792997d56 > usr/src/uts/common/fs/zfs/spa_config.c > 6f44dfa270b47b7479b410a1acd7a5c974952a7f > usr/src/uts/common/fs/zfs/spa.c bc15615a14078f1737a15d4eac5433347695485d > usr/src/uts/common/fs/zfs/range_tree.c > 6422fd1c1fa6ecf2a9283fefeabca772f6b0a76a > usr/src/uts/common/fs/zfs/metaslab.c > e7624f040eda714d1acd9d46af0e475fffdacee4 > usr/src/uts/common/fs/zfs/dsl_scan.c > 0a382ee1d9db5f41af72475adfecca782c74386f > usr/src/uts/common/Makefile.files fc68987bbe8d4eda18429943da9dffaffc773d60 > usr/src/pkg/manifests/system-header.mf > d14c524fc7135061dee7251d131356edb504f8c4 > usr/src/man/man1m/zpool.1m eadb5482be975f0f43603b1ef920d23c6eda7b13 > usr/src/lib/libzpool/common/sys/zfs_context.h > 94d59874d30ae59ce9094f2abc2c54984a1492df > usr/src/lib/libzpool/Makefile.com b016ffaa7054bb052e0436a260a23311bb7f0b47 > usr/src/lib/libzfs/common/mapfile-vers > bc616be0c29741b8c22ab7ae3acebf164e5abe1b > usr/src/lib/libzfs/common/libzfs_util.c > 7dd587a537585170e3f5d262e57779bc59cfadaa > usr/src/lib/libzfs/common/libzfs_pool.c > 69b7314dcfeced8b1fc5ec1e47430e1d65505b1b > usr/src/common/zfs/zpool_prop.c 9c717442ed7a07772f7b29dad45ced1cbba78b6a > usr/src/cmd/zpool/zpool_main.c 82b9672a44cf8809967281d20d451f0b04a111e3 > usr/src/uts/common/sys/sysevent/eventdefs.h > 25401cec53047a5686726a67bfcdb93592de768f > usr/src/uts/common/fs/zfs/vdev_file.c > 633621b0dd9becef4e4a9b0db9769bed9459f4eb > usr/src/uts/common/fs/zfs/sys/spa.h > da63812831fd3fd6acbe627e46212c68a658a339 > usr/src/uts/common/fs/zfs/dsl_synctask.c > e66e931726f0cf6a509b0b8d97d1707c87d5fd9e > usr/src/lib/libzfs/common/libzfs.h 9baff24146b58d77a3d18dbac57d2f60b2e2f894 > > Diff: https://reviews.csiden.org/r/263/diff/ > > > Testing > ------- > > Run on assortment of raidz, mirrors and straight vdevs. > > > Thanks, > > Saso Kiselkov > > ------------------------------------------- openzfs-developer Archives: https://www.listbox.com/member/archive/274414/=now RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa Modify Your Subscription: https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c Powered by Listbox: http://www.listbox.com
