* [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: [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
* 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
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;
as well as URLs for NNTP newsgroup(s).