Re: [RFC] ->poll() bugs

From: Al Viro
Date: Thu Mar 05 2015 - 10:58:23 EST


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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/