> 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/. > > Saso Kiselkov wrote: > 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. > > Robert Mustacchi wrote: > It's true that it's not different from that special case; however, none > of those have any userland specific mechansims in them. There's no use of > #ifdef KERNEL or not. In fact, I thought the whole point of libzpool is to > provide those implementations there. Do we even need a special user version > here? I would have thought all the kmem_alloc, etc. was already stubbed out > there due to the rest of how ZFS and libzpool works. So perhaps none of that > is actually necessary.
You're right, we don't need it. I've added a stub definition of FKIOCTL and ddi_copyin to zfs_context.h in libzpool and now it builds fine in user space. > 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? > > Saso Kiselkov wrote: > 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. > > Robert Mustacchi wrote: > I have mixed feelings. In general, I would tend towards the VERIFY; > however, I've been bit a lot lately by things that were overzealous kernel > things assuming this would never happen and then it'd only end up happening > in production. Maybe a compromise would be to ASSERT / panic on debug and > EINAVL on non-debug? Sounds reasonable, built that in. > 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. > > Saso Kiselkov wrote: > 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... > > Robert Mustacchi wrote: > Yeah, if you phrase it that way, I guess it makes sense. I usually just > think of there always being an explicit / different case for overflow to make > it easier for the developer to understand what happened. But Ic an go either > way. dfl_copyin interface changed, now it returns an errno. > 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? > > Saso Kiselkov wrote: > 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. > > Robert Mustacchi wrote: > I agree on generally holding the kernel to a higher standard, I'd like to > as well. How about what I suggested up above where EINVAL on non-debug and > ASSERT/panic on debug? Good idea. In case the call is from the kernel, I've added ASSERTs for both a NULL pointer and the number of extents being reasonable. > 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. > > Saso Kiselkov wrote: > 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. > > Robert Mustacchi wrote: > The difference I was trying to get at is that usually EFAULT is reserved > for the case where the user address space was unmapped. Where as in the case > of an overflow, often a different error is used. But it ties into the broader > group. I can't speak for everyone, but if I was debugging this and I got an > EFAULT, I'd assume a copyin/copyout failed as opposed to it never having been > run. Alright, interface to dfl_copyin changed. Now it returns an errno and takes an extra result pointer arg. > 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. > > Saso Kiselkov wrote: > 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. Changed to return errno. - 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
