From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757103AbbCEP6V (ORCPT ); Thu, 5 Mar 2015 10:58:21 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:36498 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756953AbbCEP6U (ORCPT ); Thu, 5 Mar 2015 10:58:20 -0500 Date: Thu, 5 Mar 2015 15:58:19 +0000 From: Al Viro To: linux-kernel@vger.kernel.org Subject: Re: [RFC] ->poll() bugs Message-ID: <20150305155818.GA29656@ZenIV.linux.org.uk> References: <20150305154921.GZ29656@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150305154921.GZ29656@ZenIV.linux.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.