* [RFC] ->poll() bugs
@ 2015-03-05 15:49 Al Viro
2015-03-05 15:58 ` Al Viro
2015-03-07 20:44 ` Al Viro
0 siblings, 2 replies; 8+ messages in thread
From: Al Viro @ 2015-03-05 15:49 UTC (permalink / raw)
To: linux-kernel
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:
sound/oss/dmasound/dmasound_core.c:
static unsigned int sq_poll(struct file *file, struct poll_table_struct *wait)
{
unsigned int mask = 0;
int retVal;
if (write_sq.locked == 0) {
if ((retVal = sq_setup(&write_sq)) < 0)
return retVal;
sound/core/pcm_native.c:
static unsigned int snd_pcm_capture_poll(struct file *file, poll_table * wait)
{
struct snd_pcm_file *pcm_file;
struct snd_pcm_substream *substream;
struct snd_pcm_runtime *runtime;
unsigned int mask;
snd_pcm_uframes_t avail;
pcm_file = file->private_data;
substream = pcm_file->substream;
if (PCM_RUNTIME_CHECK(substream))
return -ENXIO;
arch/powerpc/platforms/cell/spufs/file.c:
static unsigned int spufs_switch_log_poll(struct file *file, poll_table *wait)
{
struct inode *inode = file_inode(file);
struct spu_context *ctx = SPUFS_I(inode)->i_ctx;
unsigned int mask = 0;
int rc;
poll_wait(file, &ctx->switch_log->wait, wait);
rc = spu_acquire(ctx);
if (rc)
return rc;
(spu_acquire() can return -EINTR), etc.
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. Will be quite a bit of
churn, but then it doesn't have to be done all at once - e.g.
#ifdef CHECKER_POLL
typedef unsigned int __bitwise poll_t;
#else
typedef unsigned int poll_t;
#endif
in linux/types.h,
poll_t (*poll)(.....)
in linux/fs.h
and
#ifdef CHECKER_POLL
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [RFC] ->poll() bugs 2015-03-05 15:49 [RFC] ->poll() bugs Al Viro @ 2015-03-05 15:58 ` Al Viro 2015-03-07 20:44 ` Al Viro 1 sibling, 0 replies; 8+ messages in thread From: Al Viro @ 2015-03-05 15:58 UTC (permalink / raw) To: linux-kernel 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: > > sound/oss/dmasound/dmasound_core.c: > static unsigned int sq_poll(struct file *file, struct poll_table_struct *wait) > { > unsigned int mask = 0; > int retVal; > > if (write_sq.locked == 0) { > if ((retVal = sq_setup(&write_sq)) < 0) > return retVal; > > sound/core/pcm_native.c: > static unsigned int snd_pcm_capture_poll(struct file *file, poll_table * wait) > { > struct snd_pcm_file *pcm_file; > struct snd_pcm_substream *substream; > struct snd_pcm_runtime *runtime; > unsigned int mask; > snd_pcm_uframes_t avail; > > pcm_file = file->private_data; > > substream = pcm_file->substream; > if (PCM_RUNTIME_CHECK(substream)) > return -ENXIO; > > arch/powerpc/platforms/cell/spufs/file.c: > static unsigned int spufs_switch_log_poll(struct file *file, poll_table *wait) > { > struct inode *inode = file_inode(file); > struct spu_context *ctx = SPUFS_I(inode)->i_ctx; > unsigned int mask = 0; > int rc; > > poll_wait(file, &ctx->switch_log->wait, wait); > > rc = spu_acquire(ctx); > if (rc) > return rc; > (spu_acquire() can return -EINTR), etc. > > 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. Will be quite a bit of > churn, but then it doesn't have to be done all at once - e.g. > #ifdef CHECKER_POLL > typedef unsigned int __bitwise poll_t; > #else > typedef unsigned int poll_t; > #endif > in linux/types.h, > poll_t (*poll)(.....) > in linux/fs.h > and > #ifdef CHECKER_POLL Grr.... I wonder WTF has truncated it... Anyway, that was supposed to be #ifdef CHECKER_POLL #define __POLL(x) ((__force poll_t)(x)) #else #define __POLL(x) x #endif in uapi/asm-generic/poll.h, with POLLERR et.al. defined as __POLL(0x...) instead of 0x... That way we can avoid the noise on partially annotated tree - to get the warnings from those we'd just add CF="-DCHECKER_POLL" to make arguments. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ->poll() bugs 2015-03-05 15:49 [RFC] ->poll() bugs Al Viro 2015-03-05 15:58 ` Al Viro @ 2015-03-07 20:44 ` Al Viro 2015-03-07 20:53 ` Linus Torvalds 1 sibling, 1 reply; 8+ messages in thread From: Al Viro @ 2015-03-07 20:44 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ->poll() bugs 2015-03-07 20:44 ` Al Viro @ 2015-03-07 20:53 ` Linus Torvalds 2015-03-07 21:03 ` Al Viro ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Linus Torvalds @ 2015-03-07 20:53 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List On Sat, Mar 7, 2015 at 12:44 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > 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. Hmm. I do wonder if we should just *allow* ->poll() to return an error, and just turn it into "all bits set"? But if getting sparse to catch them all isn't *too* painful and the patch doesn't end up being horribly big, then I guess that's ok. > Linus, what do you think about putting those annotations into mainline during > the next cycle? Just how big is that annotation patch? We have a *lot* of poll functions, don't we? If they all need to be changed, just how bad is the noise for that? Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ->poll() bugs 2015-03-07 20:53 ` Linus Torvalds @ 2015-03-07 21:03 ` Al Viro 2015-03-07 21:08 ` [PATCH] sunrpc: fix braino in ->poll() Al Viro 2015-03-08 1:33 ` [RFC] ->poll() bugs Al Viro 2 siblings, 0 replies; 8+ messages in thread From: Al Viro @ 2015-03-07 21:03 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List On Sat, Mar 07, 2015 at 12:53:14PM -0800, Linus Torvalds wrote: > On Sat, Mar 7, 2015 at 12:44 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > 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. > > Hmm. I do wonder if we should just *allow* ->poll() to return an > error, and just turn it into "all bits set"? > > But if getting sparse to catch them all isn't *too* painful and the > patch doesn't end up being horribly big, then I guess that's ok. > > > Linus, what do you think about putting those annotations into mainline during > > the next cycle? > > Just how big is that annotation patch? We have a *lot* of poll > functions, don't we? Several hundreds over the entire tree. > If they all need to be changed, just how bad is > the noise for that? One or two lines per function. Either just the return type (i.e. s/unsigned int/__poll_t/ in declaration) or that plus local variable used to build the return value - something like unsigned int mask = 0; turning into __poll_t mask = 0. See vfs.git#poll; I'm NOT proposing to pull it right now, but pull request would contain the following: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git poll Shortlog: Al Viro (20): switch poll.h to generic-y where possible annotate POLL... constants and ->poll() return value annotate poll_table_struct->_key and poll_table_entry->key annotate wake_..._poll() last argument fs: annotate ->poll() instances annotate proto_ops ->poll() annotate proto_ops ->poll() instances net: annotate file_operations ->poll() instances kernel: annotate ->poll() instances annotate posix clock annotate security/ annotate mm/ annotate ipc/ annotate drivers/media and its users annotate sound/ annotate tty annotate assorted drivers missed tomoyo annotation VFS annotations annotations around wakeup callbacks Diffstat: arch/alpha/include/asm/Kbuild | 1 + arch/alpha/include/uapi/asm/poll.h | 1 - arch/blackfin/include/uapi/asm/poll.h | 4 +-- arch/cris/include/asm/Kbuild | 1 + arch/cris/include/uapi/asm/poll.h | 1 - arch/frv/include/uapi/asm/poll.h | 2 +- arch/ia64/include/asm/Kbuild | 1 + arch/ia64/include/uapi/asm/poll.h | 1 - arch/m32r/include/asm/Kbuild | 1 + arch/m32r/include/uapi/asm/poll.h | 1 - arch/m68k/include/uapi/asm/poll.h | 2 +- arch/microblaze/include/asm/Kbuild | 1 + arch/microblaze/include/uapi/asm/poll.h | 1 - arch/mips/include/uapi/asm/poll.h | 2 +- arch/mn10300/include/asm/Kbuild | 1 + arch/mn10300/include/uapi/asm/poll.h | 1 - arch/powerpc/include/asm/Kbuild | 1 + arch/powerpc/include/uapi/asm/poll.h | 1 - arch/s390/include/asm/Kbuild | 1 + arch/s390/include/uapi/asm/poll.h | 1 - arch/score/include/asm/Kbuild | 1 + arch/score/include/uapi/asm/poll.h | 6 ---- arch/sparc/include/uapi/asm/poll.h | 8 ++--- arch/x86/include/asm/Kbuild | 1 + arch/x86/include/uapi/asm/poll.h | 1 - arch/xtensa/include/uapi/asm/poll.h | 4 +-- block/bsg.c | 4 +-- crypto/algif_skcipher.c | 6 ++-- drivers/android/binder.c | 2 +- drivers/bluetooth/hci_ldisc.c | 2 +- drivers/bluetooth/hci_vhci.c | 2 +- drivers/char/hpet.c | 2 +- drivers/char/ipmi/ipmi_devintf.c | 4 +-- drivers/char/ipmi/ipmi_watchdog.c | 4 +-- drivers/char/pcmcia/cm4040_cs.c | 4 +-- drivers/char/ppdev.c | 4 +-- drivers/char/random.c | 4 +-- drivers/char/virtio_console.c | 4 +-- drivers/char/xillybus/xillybus_core.c | 4 +-- drivers/dma-buf/dma-buf.c | 6 ++-- drivers/firewire/core-cdev.c | 4 +-- drivers/firewire/nosy.c | 4 +-- drivers/gpu/drm/drm_fops.c | 4 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 2 +- drivers/gpu/vga/vgaarb.c | 2 +- drivers/hid/hid-debug.c | 2 +- drivers/hid/hid-roccat.c | 2 +- drivers/hid/hidraw.c | 2 +- drivers/hid/uhid.c | 2 +- drivers/hid/usbhid/hiddev.c | 2 +- drivers/iio/iio_core.h | 2 +- drivers/iio/industrialio-buffer.c | 2 +- drivers/iio/industrialio-event.c | 4 +-- drivers/infiniband/core/ucm.c | 4 +-- drivers/infiniband/core/ucma.c | 4 +-- drivers/infiniband/core/user_mad.c | 4 +-- drivers/infiniband/core/uverbs_main.c | 4 +-- drivers/infiniband/hw/ipath/ipath_file_ops.c | 23 +++++++------- drivers/infiniband/hw/qib/qib_file_ops.c | 14 ++++----- drivers/input/evdev.c | 4 +-- drivers/input/input.c | 2 +- drivers/input/joydev.c | 2 +- drivers/input/misc/uinput.c | 2 +- drivers/input/mousedev.c | 4 +-- drivers/input/serio/serio_raw.c | 4 +-- drivers/isdn/capi/capi.c | 4 +-- drivers/isdn/divert/divert_procfs.c | 4 +-- drivers/isdn/hardware/eicon/divamnt.c | 4 +-- drivers/isdn/hardware/eicon/divasi.c | 4 +-- drivers/isdn/hardware/eicon/divasmain.c | 2 +- drivers/isdn/hardware/eicon/divasproc.c | 2 +- drivers/isdn/hysdn/hysdn_proclog.c | 4 +-- drivers/isdn/i4l/isdn_common.c | 4 +-- drivers/isdn/i4l/isdn_ppp.c | 4 +-- drivers/isdn/i4l/isdn_ppp.h | 2 +- drivers/isdn/mISDN/timerdev.c | 4 +-- drivers/md/md.c | 4 +-- drivers/media/common/saa7146/saa7146_fops.c | 8 ++--- drivers/media/common/siano/smsdvb-debugfs.c | 7 ++--- drivers/media/dvb-core/dmxdev.c | 8 ++--- drivers/media/dvb-core/dvb_ca_en50221.c | 4 +-- drivers/media/dvb-core/dvb_frontend.c | 2 +- drivers/media/firewire/firedtv-ci.c | 2 +- drivers/media/media-devnode.c | 2 +- drivers/media/pci/bt8xx/bttv-driver.c | 12 ++++---- drivers/media/pci/cx18/cx18-fileops.c | 4 +-- drivers/media/pci/cx18/cx18-fileops.h | 2 +- drivers/media/pci/ddbridge/ddbridge-core.c | 4 +-- drivers/media/pci/ivtv/ivtv-fileops.c | 10 +++--- drivers/media/pci/ivtv/ivtv-fileops.h | 4 +-- drivers/media/pci/meye/meye.c | 4 +-- drivers/media/pci/saa7134/saa7134-video.c | 4 +-- drivers/media/pci/saa7164/saa7164-encoder.c | 4 +-- drivers/media/pci/saa7164/saa7164-vbi.c | 4 +-- drivers/media/pci/ttpci/av7110_av.c | 8 ++--- drivers/media/pci/ttpci/av7110_ca.c | 4 +-- drivers/media/pci/zoran/zoran_driver.c | 5 +-- drivers/media/platform/blackfin/bfin_capture.c | 4 +-- drivers/media/platform/davinci/vpfe_capture.c | 2 +- drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +-- drivers/media/platform/fsl-viu.c | 4 +-- drivers/media/platform/m2m-deinterlace.c | 4 +-- drivers/media/platform/marvell-ccic/mcam-core.c | 4 +-- drivers/media/platform/mx2_emmaprp.c | 4 +-- drivers/media/platform/omap/omap_vout.c | 2 +- drivers/media/platform/omap3isp/ispvideo.c | 4 +-- drivers/media/platform/s3c-camif/camif-capture.c | 4 +-- drivers/media/platform/s5p-mfc/s5p_mfc.c | 4 +-- drivers/media/platform/s5p-tv/mixer_video.c | 4 +-- drivers/media/platform/sh_veu.c | 2 +- drivers/media/platform/sh_vou.c | 4 +-- drivers/media/platform/soc_camera/atmel-isi.c | 2 +- drivers/media/platform/soc_camera/mx2_camera.c | 2 +- drivers/media/platform/soc_camera/mx3_camera.c | 2 +- drivers/media/platform/soc_camera/rcar_vin.c | 2 +- .../platform/soc_camera/sh_mobile_ceu_camera.c | 2 +- drivers/media/platform/soc_camera/soc_camera.c | 4 +-- drivers/media/platform/timblogiw.c | 2 +- drivers/media/platform/via-camera.c | 2 +- drivers/media/platform/vivid/vivid-core.c | 2 +- drivers/media/platform/vivid/vivid-radio-rx.c | 2 +- drivers/media/platform/vivid/vivid-radio-rx.h | 2 +- drivers/media/platform/vivid/vivid-radio-tx.c | 2 +- drivers/media/platform/vivid/vivid-radio-tx.h | 2 +- drivers/media/radio/radio-cadet.c | 4 +-- drivers/media/radio/radio-si476x.c | 6 ++-- drivers/media/radio/radio-wl1273.c | 2 +- drivers/media/radio/si470x/radio-si470x-common.c | 6 ++-- drivers/media/radio/wl128x/fmdrv_v4l2.c | 2 +- drivers/media/rc/lirc_dev.c | 4 +-- drivers/media/usb/cpia2/cpia2.h | 4 +-- drivers/media/usb/cpia2/cpia2_core.c | 4 +-- drivers/media/usb/cpia2/cpia2_v4l.c | 4 +-- drivers/media/usb/cx231xx/cx231xx-417.c | 6 ++-- drivers/media/usb/cx231xx/cx231xx-video.c | 6 ++-- drivers/media/usb/gspca/gspca.c | 6 ++-- drivers/media/usb/hdpvr/hdpvr-video.c | 6 ++-- drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 +-- drivers/media/usb/stkwebcam/stk-webcam.c | 4 +-- drivers/media/usb/tm6000/tm6000-video.c | 10 +++--- drivers/media/usb/uvc/uvc_queue.c | 4 +-- drivers/media/usb/uvc/uvc_v4l2.c | 2 +- drivers/media/usb/uvc/uvcvideo.h | 2 +- drivers/media/usb/zr364xx/zr364xx.c | 4 +-- drivers/media/v4l2-core/v4l2-ctrls.c | 2 +- drivers/media/v4l2-core/v4l2-dev.c | 4 +-- drivers/media/v4l2-core/v4l2-mem2mem.c | 10 +++--- drivers/media/v4l2-core/v4l2-subdev.c | 2 +- drivers/media/v4l2-core/videobuf-core.c | 10 +++--- drivers/media/v4l2-core/videobuf2-core.c | 10 +++--- drivers/misc/hpilo.c | 2 +- drivers/misc/lis3lv02d/lis3lv02d.c | 2 +- drivers/misc/mei/amthif.c | 4 +-- drivers/misc/mei/main.c | 4 +-- drivers/misc/mei/mei_dev.h | 2 +- drivers/misc/mic/host/mic_fops.c | 4 +-- drivers/misc/mic/host/mic_fops.h | 2 +- drivers/misc/phantom.c | 4 +-- drivers/misc/vmw_vmci/vmci_host.c | 4 +-- drivers/net/macvtap.c | 4 +-- drivers/net/ppp/ppp_async.c | 2 +- drivers/net/ppp/ppp_generic.c | 4 +-- drivers/net/ppp/ppp_synctty.c | 2 +- drivers/net/tun.c | 4 +-- drivers/net/wireless/rt2x00/rt2x00debug.c | 2 +- drivers/platform/goldfish/goldfish_pipe.c | 4 +-- drivers/platform/x86/sony-laptop.c | 2 +- drivers/pps/pps.c | 2 +- drivers/ptp/ptp_chardev.c | 2 +- drivers/ptp/ptp_private.h | 2 +- drivers/rtc/rtc-dev.c | 2 +- drivers/scsi/mpt2sas/mpt2sas_ctl.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +- drivers/scsi/sg.c | 4 +-- drivers/staging/android/sync.c | 2 +- drivers/staging/comedi/comedi_fops.c | 4 +-- drivers/staging/comedi/drivers/serial2002.c | 2 +- drivers/staging/ft1000/ft1000-usb/ft1000_debug.c | 4 +-- drivers/staging/media/bcm2048/radio-bcm2048.c | 4 +-- drivers/staging/media/davinci_vpfe/vpfe_video.c | 2 +- drivers/staging/media/dt3155v4l/dt3155v4l.c | 4 +-- drivers/staging/media/lirc/lirc_parallel.c | 2 +- drivers/staging/media/lirc/lirc_sir.c | 4 +-- drivers/staging/media/lirc/lirc_zilog.c | 4 +-- drivers/staging/media/omap4iss/iss_video.c | 2 +- drivers/staging/ozwpan/ozcdev.c | 4 +-- drivers/staging/speakup/speakup_soft.c | 4 +-- drivers/tty/n_gsm.c | 4 +-- drivers/tty/n_hdlc.c | 6 ++-- drivers/tty/n_r3964.c | 6 ++-- drivers/tty/n_tty.c | 4 +-- drivers/tty/tty_io.c | 8 ++--- drivers/tty/vt/vc_screen.c | 4 +-- drivers/uio/uio.c | 2 +- drivers/usb/class/cdc-wdm.c | 4 +-- drivers/usb/class/usblp.c | 4 +-- drivers/usb/core/devices.c | 2 +- drivers/usb/core/devio.c | 4 +-- drivers/usb/gadget/function/f_fs.c | 4 +-- drivers/usb/gadget/function/f_hid.c | 4 +-- drivers/usb/gadget/function/uvc_queue.c | 4 +-- drivers/usb/gadget/function/uvc_queue.h | 2 +- drivers/usb/gadget/function/uvc_v4l2.c | 2 +- drivers/usb/gadget/legacy/inode.c | 4 +-- drivers/usb/gadget/legacy/printer.c | 4 +-- drivers/usb/misc/iowarrior.c | 4 +-- drivers/usb/misc/ldusb.c | 4 +-- drivers/usb/misc/legousbtower.c | 6 ++-- drivers/usb/mon/mon_bin.c | 4 +-- drivers/vfio/pci/vfio_pci_intrs.c | 4 +-- drivers/vhost/vhost.c | 8 ++--- drivers/vhost/vhost.h | 4 +-- drivers/xen/evtchn.c | 4 +-- drivers/xen/mcelog.c | 2 +- drivers/xen/xenbus/xenbus_dev_frontend.c | 2 +- fs/cachefiles/daemon.c | 10 +++--- fs/coda/psdev.c | 4 +-- fs/dlm/plock.c | 4 +-- fs/dlm/user.c | 2 +- fs/ecryptfs/miscdev.c | 4 +-- fs/eventfd.c | 4 +-- fs/eventpoll.c | 6 ++-- fs/fcntl.c | 4 +-- fs/fuse/dev.c | 4 +-- fs/fuse/file.c | 2 +- fs/fuse/fuse_i.h | 2 +- fs/kernfs/file.c | 2 +- fs/notify/fanotify/fanotify_user.c | 4 +-- fs/notify/inotify/inotify_user.c | 4 +-- fs/ocfs2/dlmfs/dlmfs.c | 4 +-- fs/pipe.c | 4 +-- fs/proc/inode.c | 6 ++-- fs/proc/kmsg.c | 2 +- fs/proc/proc_sysctl.c | 4 +-- fs/proc_namespace.c | 4 +-- fs/select.c | 23 ++++++-------- fs/signalfd.c | 4 +-- fs/timerfd.c | 4 +-- include/drm/drmP.h | 2 +- include/linux/dma-buf.h | 2 +- include/linux/fs.h | 2 +- include/linux/net.h | 2 +- include/linux/poll.h | 10 +++--- include/linux/posix-clock.h | 2 +- include/linux/ring_buffer.h | 2 +- include/linux/skbuff.h | 4 +-- include/linux/tty_ldisc.h | 2 +- include/linux/wait.h | 10 +++--- include/media/lirc_dev.h | 2 +- include/media/media-devnode.h | 2 +- include/media/soc_camera.h | 2 +- include/media/v4l2-ctrls.h | 2 +- include/media/v4l2-dev.h | 2 +- include/media/v4l2-mem2mem.h | 4 +-- include/media/videobuf-core.h | 2 +- include/media/videobuf2-core.h | 4 +-- include/net/bluetooth/bluetooth.h | 2 +- include/net/inet_connection_sock.h | 2 +- include/net/iucv/af_iucv.h | 4 +-- include/net/sctp/sctp.h | 3 +- include/net/sock.h | 4 +-- include/net/tcp.h | 4 +-- include/net/udp.h | 2 +- include/sound/hwdep.h | 2 +- include/sound/info.h | 2 +- include/uapi/asm-generic/poll.h | 36 +++++++++++++--------- include/uapi/linux/types.h | 6 ++++ ipc/mqueue.c | 4 +-- kernel/events/core.c | 4 +-- kernel/printk/printk.c | 4 +-- kernel/relay.c | 4 +-- kernel/time/posix-clock.c | 4 +-- kernel/trace/ring_buffer.c | 2 +- kernel/trace/trace.c | 6 ++-- mm/memcontrol.c | 2 +- mm/swapfile.c | 2 +- net/atm/common.c | 4 +-- net/atm/common.h | 2 +- net/batman-adv/debugfs.c | 2 +- net/batman-adv/icmp_socket.c | 2 +- net/bluetooth/af_bluetooth.c | 7 ++--- net/caif/caif_socket.c | 6 ++-- net/core/datagram.c | 7 ++--- net/core/sock.c | 2 +- net/dccp/dccp.h | 3 +- net/dccp/proto.c | 5 ++- net/decnet/af_decnet.c | 4 +-- net/ipv4/tcp.c | 4 +-- net/ipv4/udp.c | 4 +-- net/irda/af_irda.c | 6 ++-- net/irda/irnet/irnet_ppp.c | 8 ++--- net/irda/irnet/irnet_ppp.h | 2 +- net/iucv/af_iucv.c | 8 ++--- net/netlink/af_netlink.c | 6 ++-- net/nfc/llcp_sock.c | 8 ++--- net/packet/af_packet.c | 6 ++-- net/phonet/socket.c | 6 ++-- net/rds/af_rds.c | 6 ++-- net/rfkill/core.c | 4 +-- net/rxrpc/af_rxrpc.c | 6 ++-- net/sctp/socket.c | 4 +-- net/socket.c | 7 ++--- net/sunrpc/cache.c | 10 +++--- net/sunrpc/rpc_pipe.c | 4 +-- net/tipc/socket.c | 6 ++-- net/unix/af_unix.c | 15 ++++----- net/vmw_vsock/af_vsock.c | 6 ++-- security/tomoyo/audit.c | 2 +- security/tomoyo/common.c | 4 +-- security/tomoyo/common.h | 6 ++-- security/tomoyo/securityfs_if.c | 2 +- sound/core/compress_offload.c | 6 ++-- sound/core/control.c | 4 +-- sound/core/hwdep.c | 2 +- sound/core/info.c | 4 +-- sound/core/init.c | 2 +- sound/core/oss/pcm_oss.c | 4 +-- sound/core/pcm_native.c | 8 ++--- sound/core/rawmidi.c | 4 +-- sound/core/seq/oss/seq_oss.c | 4 +-- sound/core/seq/oss/seq_oss_device.h | 2 +- sound/core/seq/oss/seq_oss_rw.c | 4 +-- sound/core/seq/seq_clientmgr.c | 4 +-- sound/core/timer.c | 4 +-- sound/firewire/bebob/bebob_hwdep.c | 4 +-- sound/firewire/dice/dice-hwdep.c | 4 +-- sound/firewire/fireworks/fireworks_hwdep.c | 4 +-- sound/firewire/oxfw/oxfw-hwdep.c | 4 +-- sound/oss/dmabuf.c | 6 ++-- sound/oss/midibuf.c | 4 +-- sound/oss/sequencer.c | 4 +-- sound/oss/sound_calls.h | 6 ++-- sound/oss/soundcard.c | 2 +- sound/usb/mixer_quirks.c | 2 +- sound/usb/usx2y/us122l.c | 4 +-- sound/usb/usx2y/usX2Yhwdep.c | 4 +-- virt/kvm/eventfd.c | 4 +-- 338 files changed, 668 insertions(+), 668 deletions(-) delete mode 100644 arch/alpha/include/uapi/asm/poll.h delete mode 100644 arch/cris/include/uapi/asm/poll.h delete mode 100644 arch/ia64/include/uapi/asm/poll.h delete mode 100644 arch/m32r/include/uapi/asm/poll.h delete mode 100644 arch/microblaze/include/uapi/asm/poll.h delete mode 100644 arch/mn10300/include/uapi/asm/poll.h delete mode 100644 arch/powerpc/include/uapi/asm/poll.h delete mode 100644 arch/s390/include/uapi/asm/poll.h delete mode 100644 arch/score/include/uapi/asm/poll.h delete mode 100644 arch/x86/include/uapi/asm/poll.h ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] sunrpc: fix braino in ->poll() 2015-03-07 20:53 ` Linus Torvalds 2015-03-07 21:03 ` Al Viro @ 2015-03-07 21:08 ` Al Viro 2015-03-09 19:41 ` J. Bruce Fields 2015-03-08 1:33 ` [RFC] ->poll() bugs Al Viro 2 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2015-03-07 21:08 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, J. Bruce Fields POLL_OUT isn't what callers of ->poll() are expecting to see; it's actually __SI_POLL | 2 and it's a siginfo code, not a poll bitmap bit... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 33fb105..5199bb1 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -921,7 +921,7 @@ static unsigned int cache_poll(struct file *filp, poll_table *wait, poll_wait(filp, &queue_wait, wait); /* alway allow write */ - mask = POLL_OUT | POLLWRNORM; + mask = POLLOUT | POLLWRNORM; if (!rp) return mask; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] sunrpc: fix braino in ->poll() 2015-03-07 21:08 ` [PATCH] sunrpc: fix braino in ->poll() Al Viro @ 2015-03-09 19:41 ` J. Bruce Fields 0 siblings, 0 replies; 8+ messages in thread From: J. Bruce Fields @ 2015-03-09 19:41 UTC (permalink / raw) To: Al Viro; +Cc: Linus Torvalds, Linux Kernel Mailing List On Sat, Mar 07, 2015 at 09:08:46PM +0000, Al Viro wrote: > POLL_OUT isn't what callers of ->poll() are expecting to see; it's > actually __SI_POLL | 2 and it's a siginfo code, not a poll bitmap > bit... Thanks!--b. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > index 33fb105..5199bb1 100644 > --- a/net/sunrpc/cache.c > +++ b/net/sunrpc/cache.c > @@ -921,7 +921,7 @@ static unsigned int cache_poll(struct file *filp, poll_table *wait, > poll_wait(filp, &queue_wait, wait); > > /* alway allow write */ > - mask = POLL_OUT | POLLWRNORM; > + mask = POLLOUT | POLLWRNORM; > > if (!rp) > return mask; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ->poll() bugs 2015-03-07 20:53 ` Linus Torvalds 2015-03-07 21:03 ` Al Viro 2015-03-07 21:08 ` [PATCH] sunrpc: fix braino in ->poll() Al Viro @ 2015-03-08 1:33 ` Al Viro 2 siblings, 0 replies; 8+ messages in thread From: Al Viro @ 2015-03-08 1:33 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List On Sat, Mar 07, 2015 at 12:53:14PM -0800, Linus Torvalds wrote: > On Sat, Mar 7, 2015 at 12:44 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > 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. > > Hmm. I do wonder if we should just *allow* ->poll() to return an > error, and just turn it into "all bits set"? Doable, but... we do have a bunch of ->poll() call sites (AFAICS, right now it's drivers/staging/comedi/drivers/serial2002.c:142: drivers/vfio/pci/vfio_pci_intrs.c:191: drivers/vhost/vhost.c:95: fs/eventpoll.c:800: fs/proc/inode.c:229: fs/select.c:461: fs/select.c:764: mm/memcontrol.c:4285: net/9p/trans_fd.c:249: net/9p/trans_fd.c:254: virt/kvm/eventfd.c:418: ) and while they could be turned into calls of a wrapper, I really don't like returning negatives as a part of ->poll() calling conventions. I mean, really - "you should return a bitmap of POLL... flags *or* -E...; doesn't matter which error number it is, it'll be lost anyway"... It's OK to have the caller(s) catch such bugs (and report them, IMO), but silently treating it that way as a part of calling conventions... Besides, that doesn't catch the bugs like one in net/9p; sparse annotations do. > But if getting sparse to catch them all isn't *too* painful and the > patch doesn't end up being horribly big, then I guess that's ok. Getting sparse to catch them all is as trivial as * introduce a __bitwise typedef __poll_t => unsigned int * say that ->poll() (in file_operations, as well as in proto_ops, tty_ldisc_ops and several in other subsystems) returns __poll_t * switch the return type of instances from unsigned int to __poll_t * do the same to local variable(s) (if any) used to hold the return value to be. Again, from cc(1) POV it's a no-op - unannotated foo_poll() will be just fine as it is. The only place where I did something more fancy than such type change actually had been buggy - drivers/media/common/siano/smsdvb-debugfs.c:smsdvb_stats_poll() relied upon smsdvb_stats_wait_read() never returning negatives and it needs s/__poll_t rc/& = 0/ for correctness (fixed and folded). ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-03-09 19:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-05 15:49 [RFC] ->poll() bugs Al Viro 2015-03-05 15:58 ` Al Viro 2015-03-07 20:44 ` Al Viro 2015-03-07 20:53 ` Linus Torvalds 2015-03-07 21:03 ` Al Viro 2015-03-07 21:08 ` [PATCH] sunrpc: fix braino in ->poll() Al Viro 2015-03-09 19:41 ` J. Bruce Fields 2015-03-08 1:33 ` [RFC] ->poll() bugs Al Viro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox