On Thu, Mar 05, 2015 at 03:49:21PM +0000, Al Viro wrote: > Several days ago there was an interesting bug in a gadgetfs patch > posted on linux-usb - ->poll() instance returning a negative value on > what it considered an error. The trouble is, callers of ->poll() expect > a bitmap, not an integer. Reaction to small negative integer returned > by it is quite bogus. The bug wasn't hard to spot and fix (the value we > wanted had been the same as if ->poll() had been NULL, i.e. DEFAULT_POLLMASK). > However, it looked as a very easily repeated error. > > I went looking through other ->poll() instances searching for more > of the same and caught sveral more. Some random examples:
[snip] > We obviously want to return something saner for all of those. The interesting > question is what value to return; AFAICS it really depends upon the driver. > > I wonder what could be done to catch the crap of that sort; an obvious > solution would be to declare a __bitwise type (poll_t, or something like > that), > add force-casts to that type into definitions of constants (conditional upon > __CHECKER__ - these guys are in uapi/linux/poll.h) have ->poll() made to > return that and let sparse catch such places. Done. See vfs.git#poll; most of noise is gone and patch queue itself is fairly non-invasive. FWIW, here's the remaining sparse output with comments: kernel/events/core.c:3817:24: warning: incorrect type in assignment (different base types) kernel/events/core.c:3817:24: expected restricted __poll_t [usertype] events kernel/events/core.c:3817:24: got int kernel/events/ring_buffer.c:22:39: warning: incorrect type in argument 2 (different base types) kernel/events/ring_buffer.c:22:39: expected int [signed] i kernel/events/ring_buffer.c:22:39: got restricted __poll_t [usertype] <noident> false positives; perf_output_handle->rb->poll is used to store __poll_t values, with atomic_set()/atomic_xchg() used for access. kernel/time/posix-clock.c:75:24: warning: incorrect type in return expression (different base types) kernel/time/posix-clock.c:75:24: expected restricted __poll_t kernel/time/posix-clock.c:75:24: got int bug: -ENODEV from ->poll() kernel/trace/ring_buffer.c:663:32: warning: incorrect type in return expression (different base types) kernel/trace/ring_buffer.c:663:32: expected restricted __poll_t kernel/trace/ring_buffer.c:663:32: got int bug: -EINVAL from ->poll() fs/select.c:762:62: warning: restricted __poll_t degrades to integer fs/select.c:762:70: warning: restricted __poll_t degrades to integer fs/select.c:762:45: warning: incorrect type in assignment (different base types) fs/select.c:762:45: expected restricted __poll_t [usertype] _key fs/select.c:762:45: got unsigned int fs/select.c:769:50: warning: restricted __poll_t degrades to integer fs/select.c:769:60: warning: restricted __poll_t degrades to integer fs/select.c:769:30: warning: invalid assignment: &= fs/select.c:769:30: left side has type restricted __poll_t fs/select.c:769:30: right side has type unsigned int fs/select.c:773:25: warning: incorrect type in assignment (different base types) fs/select.c:773:25: expected short [signed] revents fs/select.c:773:25: got restricted __poll_t [assigned] [usertype] mask pollfd->events used to store __poll_t. The subtle part is that it's declared as short. As long as all POLL... constants do not exceed 0x8000, those can be considered as false positives, but we are right at that limit. fs/fuse/file.c:2726:25: warning: cast from restricted __poll_t fs/fuse/file.c:2748:30: warning: incorrect type in return expression (different base types) fs/fuse/file.c:2748:30: expected restricted __poll_t fs/fuse/file.c:2748:30: got unsigned int [unsigned] [addressable] [usertype] revents fuse_poll_in.events and fuse_poll_out.revents used to store __poll_t; both are u32. False positives, but we'd better document that they are supposed to match the encoding in pollfd.{events,revents}, despite different size. fs/eventpoll.c:798:18: warning: incorrect type in assignment (different base types) fs/eventpoll.c:798:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:798:18: got unsigned int [unsigned] [usertype] events fs/eventpoll.c:800:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:813:39: warning: incorrect type in return expression (different base types) fs/eventpoll.c:813:39: expected int fs/eventpoll.c:813:39: got restricted __poll_t fs/eventpoll.c:869:32: warning: incorrect type in return expression (different base types) fs/eventpoll.c:869:32: expected restricted __poll_t fs/eventpoll.c:869:32: got int fs/eventpoll.c:798:18: warning: incorrect type in assignment (different base types) fs/eventpoll.c:798:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:798:18: got unsigned int [unsigned] [usertype] events fs/eventpoll.c:800:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:798:18: warning: incorrect type in assignment (different base types) fs/eventpoll.c:798:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:798:18: got unsigned int [unsigned] [usertype] events fs/eventpoll.c:800:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:798:18: warning: incorrect type in assignment (different base types) fs/eventpoll.c:798:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:798:18: got unsigned int [unsigned] [usertype] events fs/eventpoll.c:800:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:1920:37: warning: invalid assignment: |= fs/eventpoll.c:1920:37: left side has type unsigned int fs/eventpoll.c:1920:37: right side has type restricted __poll_t fs/eventpoll.c:1935:37: warning: invalid assignment: |= fs/eventpoll.c:1935:37: left side has type unsigned int fs/eventpoll.c:1935:37: right side has type restricted __poll_t A lot of noise, all false positives. drivers/gpu/vga/vgaarb.c:1160:24: warning: incorrect type in return expression (different base types) drivers/gpu/vga/vgaarb.c:1160:24: expected restricted __poll_t drivers/gpu/vga/vgaarb.c:1160:24: got int bug: -ENODEV from ->poll() drivers/iio/industrialio-event.c:87:24: warning: incorrect type in return expression (different base types) drivers/iio/industrialio-event.c:87:24: expected restricted __poll_t drivers/iio/industrialio-event.c:87:24: got int bug: -ENODEV from ->poll() drivers/iio/industrialio-buffer.c:96:24: warning: incorrect type in return expression (different base types) drivers/iio/industrialio-buffer.c:96:24: expected restricted __poll_t drivers/iio/industrialio-buffer.c:96:24: got int bug: -ENODEV from ->poll() drivers/media/i2c/saa6588.c:418:35: warning: invalid assignment: |= drivers/media/i2c/saa6588.c:418:35: left side has type int drivers/media/i2c/saa6588.c:418:35: right side has type restricted __poll_t drivers/media/pci/bt8xx/bttv-driver.c:3327:20: warning: incorrect type in assignment (different base types) drivers/media/pci/bt8xx/bttv-driver.c:3327:20: expected int [signed] [assigned] result drivers/media/pci/bt8xx/bttv-driver.c:3327:20: got restricted __poll_t [assigned] [usertype] res drivers/media/pci/bt8xx/bttv-driver.c:3330:19: warning: incorrect type in return expression (different base types) drivers/media/pci/bt8xx/bttv-driver.c:3330:19: expected restricted __poll_t drivers/media/pci/bt8xx/bttv-driver.c:3330:19: got int [signed] [addressable] [assigned] result drivers/media/pci/saa7134/saa7134-video.c:1180:16: warning: restricted __poll_t degrades to integer drivers/media/pci/saa7134/saa7134-video.c:1180:19: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7134/saa7134-video.c:1180:19: expected restricted __poll_t drivers/media/pci/saa7134/saa7134-video.c:1180:19: got unsigned int false positives: saa6588_command.result is normally an int, but for one command (SAA6588_CMD_POLL) it's used to store __poll_t. drivers/media/pci/saa7164/saa7164-encoder.c:1266:24: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7164/saa7164-encoder.c:1266:24: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-encoder.c:1266:24: got int bug: -EIO from ->poll() drivers/media/pci/saa7164/saa7164-encoder.c:1271:40: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7164/saa7164-encoder.c:1271:40: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-encoder.c:1271:40: got int bug: -EINVAL from ->poll() drivers/media/pci/saa7164/saa7164-encoder.c:1281:40: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7164/saa7164-encoder.c:1281:40: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-encoder.c:1281:40: got int bug: -ERESTARTSYS from ->poll() drivers/media/pci/saa7164/saa7164-vbi.c:1213:24: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7164/saa7164-vbi.c:1213:24: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-vbi.c:1213:24: got int bug: -EIO from ->poll() drivers/media/pci/saa7164/saa7164-vbi.c:1218:40: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7164/saa7164-vbi.c:1218:40: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-vbi.c:1218:40: got int bug: -EINVAL from ->poll() drivers/media/pci/saa7164/saa7164-vbi.c:1228:40: warning: incorrect type in return expression (different base types) drivers/media/pci/saa7164/saa7164-vbi.c:1228:40: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-vbi.c:1228:40: got int bug: -ERESTARTSYS from ->poll() drivers/media/platform/exynos-gsc/gsc-m2m.c:706:24: warning: incorrect type in return expression (different base types) drivers/media/platform/exynos-gsc/gsc-m2m.c:706:24: expected restricted __poll_t drivers/media/platform/exynos-gsc/gsc-m2m.c:706:24: got int bug: -ERESTARTSYS from ->poll() drivers/media/platform/s3c-camif/camif-capture.c:616:21: warning: incorrect type in assignment (different base types) drivers/media/platform/s3c-camif/camif-capture.c:616:21: expected restricted __poll_t [usertype] ret drivers/media/platform/s3c-camif/camif-capture.c:616:21: got int bug: -EBUSY from ->poll() drivers/media/radio/radio-wl1273.c:1085:24: warning: incorrect type in return expression (different base types) drivers/media/radio/radio-wl1273.c:1085:24: expected restricted __poll_t drivers/media/radio/radio-wl1273.c:1085:24: got int bug: -EBUSY from ->poll() drivers/scsi/sg.c:1170:9: warning: cast from restricted __poll_t false positive: debugging printk getting __poll_t value with %d drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:365:24: warning: incorrect type in return expression (different base types) drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:365:24: expected restricted __poll_t drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:365:24: got int bug: -EBADF from ->poll() drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:379:24: warning: incorrect type in return expression (different base types) drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:379:24: expected restricted __poll_t drivers/staging/ft1000/ft1000-usb/ft1000_debug.c:379:24: got int bug: -EACCES from ->poll() drivers/tty/n_r3964.c:1237:24: warning: incorrect type in assignment (different base types) drivers/tty/n_r3964.c:1237:24: expected restricted __poll_t [assigned] [usertype] result drivers/tty/n_r3964.c:1237:24: got int bug: -EINVAL from ->poll() drivers/uio/uio.c:497:24: warning: incorrect type in return expression (different base types) drivers/uio/uio.c:497:24: expected restricted __poll_t drivers/uio/uio.c:497:24: got int bug: -EIO from ->poll() sound/core/seq/seq_clientmgr.c:1102:24: warning: incorrect type in return expression (different base types) sound/core/seq/seq_clientmgr.c:1102:24: expected restricted __poll_t sound/core/seq/seq_clientmgr.c:1102:24: got int bug: -ENXIO from ->poll() sound/core/seq/oss/seq_oss.c:199:24: warning: incorrect type in return expression (different base types) sound/core/seq/oss/seq_oss.c:199:24: expected restricted __poll_t sound/core/seq/oss/seq_oss.c:199:24: got int bug: -ENXIO from ->poll() sound/core/pcm_native.c:3118:24: warning: incorrect type in return expression (different base types) sound/core/pcm_native.c:3118:24: expected restricted __poll_t sound/core/pcm_native.c:3118:24: got int bug: -ENXIO from ->poll() sound/core/pcm_native.c:3157:24: warning: incorrect type in return expression (different base types) sound/core/pcm_native.c:3157:24: expected restricted __poll_t sound/core/pcm_native.c:3157:24: got int bug: -ENXIO from ->poll() sound/core/compress_offload.c:381:24: warning: incorrect type in return expression (different base types) sound/core/compress_offload.c:381:24: expected restricted __poll_t sound/core/compress_offload.c:381:24: got int bug: -EFAULT from ->poll() sound/core/compress_offload.c:384:24: warning: incorrect type in return expression (different base types) sound/core/compress_offload.c:384:24: expected restricted __poll_t sound/core/compress_offload.c:384:24: got int bug: -EFAULT from ->poll() sound/core/compress_offload.c:388:24: warning: incorrect type in assignment (different base types) sound/core/compress_offload.c:388:24: expected restricted __poll_t [usertype] retval sound/core/compress_offload.c:388:24: got int bug: -EBADFD from ->poll() net/9p/trans_fd.c:249:13: warning: incorrect type in assignment (different base types) net/9p/trans_fd.c:249:13: expected int [signed] ret net/9p/trans_fd.c:249:13: got restricted __poll_t net/9p/trans_fd.c:254:19: warning: incorrect type in assignment (different base types) net/9p/trans_fd.c:254:19: expected int [signed] n net/9p/trans_fd.c:254:19: got restricted __poll_t net/9p/trans_fd.c:257:30: warning: restricted __poll_t degrades to integer net/9p/trans_fd.c:257:47: warning: restricted __poll_t degrades to integer mess: p9_fd_poll() assuming that ->poll() might return -E... *and* returning such itself in several cases. Along with the normal POLL.. bitmaps. net/9p/trans_fd.c:389:27: warning: incorrect type in assignment (different base types) net/9p/trans_fd.c:389:27: expected int [signed] [assigned] n net/9p/trans_fd.c:389:27: got restricted __poll_t [usertype] <noident> net/9p/trans_fd.c:393:26: warning: restricted __poll_t degrades to integer fallout from the above - caller of p9_fd_poll() blissfully checking the result for POLLIN, _without_ bothering to check for negatives. net/9p/trans_fd.c:503:27: warning: incorrect type in assignment (different base types) net/9p/trans_fd.c:503:27: expected int [signed] n net/9p/trans_fd.c:503:27: got restricted __poll_t [usertype] <noident> net/9p/trans_fd.c:507:26: warning: restricted __poll_t degrades to integer ditto, with s/POLLIN/POLLOUT/ net/9p/trans_fd.c:597:17: warning: restricted __poll_t degrades to integer net/9p/trans_fd.c:602:17: warning: restricted __poll_t degrades to integer ditto, with both at once. net/9p/trans_fd.c:622:45: warning: restricted __poll_t degrades to integer net/9p/trans_fd.c:629:17: warning: restricted __poll_t degrades to integer net/9p/trans_fd.c:638:17: warning: restricted __poll_t degrades to integer really nasty mess. We *do* check for negatives there, only to proceed with them down into checks ofr POLLIN and POLLOUT. Worse, POLLHUP/ POLLERR/POLLNVAL are replaced with -ECONNRESET and *also* fed to checks for POLLIN/POLLOUT. To make things even more amusing, -ECONNRESET has bits 0 and 2 set on mips and clear on everything else... AFAICS, it should have return added right after p9_conn_reset(). net/9p/trans_fd.c:677:19: warning: incorrect type in assignment (different base types) net/9p/trans_fd.c:677:19: expected int [signed] n net/9p/trans_fd.c:677:19: got restricted __poll_t [usertype] <noident> net/9p/trans_fd.c:681:17: warning: restricted __poll_t degrades to integer more p9_fd_poll() mess - negatives passed to check for POLLOUT net/sunrpc/cache.c:924:27: warning: restricted __poll_t degrades to integer net/sunrpc/cache.c:924:14: warning: incorrect type in assignment (different base types) net/sunrpc/cache.c:924:14: expected restricted __poll_t [usertype] mask net/sunrpc/cache.c:924:14: got unsigned int very amusing bug: /* alway allow write */ mask = POLL_OUT | POLLWRNORM; Took me a bit to spot what was wrong there - the thing is, POLL_OUT has nothing in common with POLLOUT... That was what got caught by allmodconfig build on amd64; there's definitely a bit more elsewhere in arch/* (at least one on powerpc), but I think that it has got most of that crap and the S/N ratio looks pretty good. Most of the catch consists of ->poll() instances that return -E...; there's also an unpleasant mess in net/9p/trans_fd.c and a braino in sunrpc unexpectedly caught by the same annotations. Linus, what do you think about putting those annotations into mainline during the next cycle? ->poll() bugs of that sort seem to be pretty common and new instances are ceratainly going to be added; it would be nice to have them caught automatically. It's not a flagday change - from C point of view __poll_t is unsigned int, so the code that hadn't been annotated yet will do just fine. Comments? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/