From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Wed, 30 Jul 2014 10:06:13 +0100 Subject: [Cluster-devel] [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions In-Reply-To: <584806381.15003446.1406672757956.JavaMail.zimbra@redhat.com> References: <1406309888-10749-1-git-send-email-adas@redhat.com> <1406309888-10749-6-git-send-email-adas@redhat.com> <20140729145808.79bec165@lwn.net> <584806381.15003446.1406672757956.JavaMail.zimbra@redhat.com> Message-ID: <53D8B585.7000104@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On 29/07/14 23:25, Abhijith Das wrote: > > ----- Original Message ----- >> From: "Jonathan Corbet" >> To: "Abhi Das" >> Cc: linux-kernel at vger.kernel.org, linux-fsdevel at vger.kernel.org, cluster-devel at redhat.com >> Sent: Tuesday, July 29, 2014 1:58:08 PM >> Subject: Re: [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions [snip] >>> + if ((xc->xc_xattr_mask & XSTAT_XATTR_ALL) && >>> + lxd->xd_blob.xb_xattr_count) { >> How can that be right? lxd is __user, it doesn't seem right to be >> dereferencing it directly...? > Wouldn't the call to access_ok() at the start of the syscall take care of this? All the > __user pointers point to areas within the user supplied buffer buf and overflow past the > end of the buffer for the last lxd is checked for. > > The 2/5 patch in this series adds the following in fs/readdir.c: > > +SYSCALL_DEFINE5(xgetdents, unsigned int, fd, unsigned, flags, unsigned int, mask, > + void __user *, buf, unsigned int, count) > ... > ... > ... > + if (!access_ok(VERIFY_WRITE, buf, count)) > + return -EFAULT; > The access_ok() call only checks the source memory at the time of the call, it is possible for the page to become invalid for reading or writing to at any subsequent time. So that the kernel's accessor functions are required here. I think sparse should warn about this too. However, the intention was to show that the xreaddir approach is not ideal due to its complexity, and also due to it being very much single purpose compared with the readahead approach, and from that point of view it shouldn't affect the performance measurements in this case, Steve.