[Cluster-devel] [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions

[Steven Whitehouse <swhiteho@redhat.com>]

On 29/07/14 23:25, Abhijith Das wrote:
>
> ----- Original Message -----
>> From: "Jonathan Corbet" <corbet@lwn.net>
>> To: "Abhi Das" <adas@redhat.com>
>> 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.