Re: [RFC] ->poll() bugs

[Al Viro <viro@ZenIV.linux.org.uk>]

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.