Message-ID: <20150307204405.GC29656@ZenIV.linux.org.uk>
Date:	Sat, 7 Mar 2015 20:44:06 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [RFC] ->poll() bugs

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists