Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752190AbdGRBgM (ORCPT ); Mon, 17 Jul 2017 21:36:12 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:38970 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751603AbdGRBgK (ORCPT ); Mon, 17 Jul 2017 21:36:10 -0400 Date: Tue, 18 Jul 2017 02:36:05 +0100 From: Al Viro To: Linus Torvalds Cc: Christoph Hellwig , Linux Kernel Mailing List , linux-fsdevel , David Miller Subject: [RFC] ->poll() sparse annotations Message-ID: <20170718013605.GA2063@ZenIV.linux.org.uk> References: <20170705071446.GA10672@ZenIV.linux.org.uk> <20170705223821.GF10672@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170705223821.GF10672@ZenIV.linux.org.uk> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5781 Lines: 108 [davem Cc'd, due to sparc/sockets problem involved] On Wed, Jul 05, 2017 at 11:38:21PM +0100, Al Viro wrote: > A side note right back at you - POLL... stuff. I'd redone the old > "hunt the buggy ->poll() instances down" series (took about 12 hours > total), got it to the point where all remaining sparse warnings about > that type are for genuine bugs. It goes like that: > > define __poll_t, annotate constants > Type is controlled by ifdef - it's unsigned int unless CHECK_POLL is > defined and a bitwise type otherwise. > ->poll() methods should return __poll_t > anntotate the places where ->poll() return values go > annotate poll-related wait keys > annotate poll_table_struct ->_key > That ends all infrastructure work. Methods declarations are annotated, > instances are *not*. Due to that ifdef CHECK_POLL, normal builds, including > normal sparse builds, are unaffected; with CF=-DCHECK_POLL you get __poll_t > warnings. FWIW, I've just updated that queue to -rc1. The branch is currently at #misc.poll and it's fairly close to "all warnings left are genuine". drivers/media/pci/saa7164/saa7164-vbi.c:632:24: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-vbi.c:637:40: expected restricted __poll_t drivers/media/pci/saa7164/saa7164-vbi.c:647:40: expected restricted __poll_t drivers/media/platform/exynos-gsc/gsc-m2m.c:718:24: expected restricted __poll_t drivers/media/platform/s3c-camif/camif-capture.c:602:21: expected restricted __poll_t [usertype] ret drivers/media/radio/radio-wl1273.c:1088:24: expected restricted __poll_t drivers/platform/goldfish/goldfish_pipe.c:549:24: expected restricted __poll_t drivers/tty/n_r3964.c:1241:24: expected restricted __poll_t [assigned] [usertype] result drivers/uio/uio.c:505:24: expected restricted __poll_t kernel/trace/ring_buffer.c:640:32: expected restricted __poll_t sound/core/seq/oss/seq_oss.c:206:24: expected restricted __poll_t sound/core/seq/seq_clientmgr.c:1092:24: expected restricted __poll_t These are ->poll() instances returning -E... in some cases. All genuine bugs. kernel/events/core.c:4561:24: expected restricted __poll_t [usertype] events kernel/events/ring_buffer.c:22:39: got restricted __poll_t [usertype] atomic_{set,xchg}() used on __poll_t; false positives, these two. drivers/media/i2c/saa6588.c:416:35: right side has type restricted __poll_t drivers/media/pci/bt8xx/bttv-driver.c:3347:20: got restricted __poll_t [assigned] [usertype] res drivers/media/pci/bt8xx/bttv-driver.c:3350:19: expected restricted __poll_t drivers/media/pci/saa7134/saa7134-video.c:1243:16: warning: restricted __poll_t degrades to integer drivers/media/pci/saa7134/saa7134-video.c:1243:19: expected restricted __poll_t saa6588_ioctl(, SAA6588_CMD_POLL, ) stores POLL... bitmap in the same field where other subfunctions store int. Could be annotated away (union in the structure being filled), but... not much point, TBH. Ugly misannotation, but no more than that. fs/fuse/file.c:2761:25: warning: cast from restricted __poll_t fs/fuse/file.c:2783:30: expected restricted __poll_t fuse puts POLL... bitmaps on the wire. That's a problem waiting to happen, in theory - different architectures have different encodings. fs/eventpoll.c:1168:28: warning: restricted __poll_t degrades to integer fs/eventpoll.c:1208:57: warning: restricted __poll_t degrades to integer fs/eventpoll.c:1212:57: warning: restricted __poll_t degrades to integer fs/eventpoll.c:2054:49: warning: restricted __poll_t degrades to integer fs/eventpoll.c:2114:37: right side has type restricted __poll_t fs/eventpoll.c:2130:45: right side has type restricted __poll_t fs/eventpoll.c:880:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:880:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:880:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:880:18: expected restricted __poll_t [usertype] _key fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:882:41: warning: restricted __poll_t degrades to integer fs/eventpoll.c:895:39: got restricted __poll_t fs/eventpoll.c:951:32: expected restricted __poll_t A bloody mess. This is a genuine ABI problem, and I'm not sure if it _can_ be fixed. struct epoll_event contains a field with or'ed EPOLL... constants. They are defined in libc and are arch-independent. The kernel assumes that POLL... constants are equal to corresponding EPOLL... ones. Unfortunately, that is not true. First POLL... are universal and do match EPOLL...; however, starting with POLLWRNORM they diverge on quite a few architectures. common bfin,frv,m68k,mips xtensa sparc WRNORM bit 8 2 2 2 WRBAND bit 9 8 8 8 MSG bit 10 10 10 9 REMOVE bit 12 12 11 10 RDHUP bit 13 13 13 11 Now, POLLREMOVE doesn't have EPOLL... equivalent, but others do. As the result, blackfin, frv, m68k, mips and xtensa have EPOLLWRNORM matching POLLWRBAND and EPOLLWRBAND not matching anything. sparc has EPOLLWRNORM matching POLLWRBAND, EPOLLWRBAND matching POLLMSG (and never triggered), EPOLLMSG matching POLLREMOVE (and also never triggered) and EPOLLRDHUP not matching anything. I don't believe that anything tries to use EPOLLMSG; EPOLLWRBAND and EPOLLWRNORM might be used (even though our manpage doesn't document either). EPOLLRDHUP _is_ documented and flat-out does not work on sparc; the only way to catch POLLRDHUP via epoll there is to give it a value that is not any of EPOLL... constants. Hell knows if anything tries to do it there... Comments?