----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/263/#review889 -----------------------------------------------------------
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. usr/src/uts/common/fs/zfs/zvol.c (line 1788) <https://reviews.csiden.org/r/263/#comment742> This is misleading. EFAULT shouldn't be returned in the case of overflow. usr/src/uts/common/fs/zfs/zvol.c (line 1795) <https://reviews.csiden.org/r/263/#comment746> 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? usr/src/uts/common/io/scsi/targets/sd.c (line 21756) <https://reviews.csiden.org/r/263/#comment745> This return code isn't always correct in the case of overflow. usr/src/uts/common/io/scsi/targets/sd.c (line 23510) <https://reviews.csiden.org/r/263/#comment744> 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? usr/src/uts/common/os/dkioc_free_util.c (line 35) <https://reviews.csiden.org/r/263/#comment743> 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. usr/src/uts/common/os/dkioc_free_util.c (line 42) <https://reviews.csiden.org/r/263/#comment738> 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. usr/src/uts/common/os/dkioc_free_util.c (line 50) <https://reviews.csiden.org/r/263/#comment741> In general, prefer explicit comparisons here, as in the ddi_copyin != 0. usr/src/uts/common/os/dkioc_free_util.c (line 77) <https://reviews.csiden.org/r/263/#comment739> 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/. usr/src/uts/common/os/dkioc_free_util.c (line 87) <https://reviews.csiden.org/r/263/#comment740> Presumably it's just as important that userland catch overflow conditions as the kernel, due to how a lot of tets are run. usr/src/uts/common/sys/dkio.h (line 543) <https://reviews.csiden.org/r/263/#comment737> 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. usr/src/uts/common/sys/dkio.h (line 551) <https://reviews.csiden.org/r/263/#comment747> 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. usr/src/uts/common/sys/dkio.h (line 552) <https://reviews.csiden.org/r/263/#comment736> Can we not hardcode 16 and instead use the sizoef (dkioc_free_list_ext_t) which is where this seems to be derived from? usr/src/uts/common/sys/dkioc_free_util.h (line 25) <https://reviews.csiden.org/r/263/#comment735> These should probably be wrapped in an ifdef _KERNEL block. - Robert Mustacchi 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
