Re: [GIT PULL] Orangefs (text only resend)

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

On Sun, Sep 06, 2015 at 10:52:16AM -0400, Mike Marshall wrote:
> int pvfs_bufmap_copy_to_user_iovec2(struct pvfs2_bufmap *bufmap,
>                                                         struct iov_iter *iter,
>                                                         size_t total_size,
>                                                         int buffer_index)
> {
>         struct pvfs_bufmap_desc *from;
>         int ret = 0; void *from_kaddr = NULL;
>         from = &bufmap->desc_array[buffer_index];
> 
>         from_kaddr = pvfs2_kmap(from->page_array[0]);
>         ret = copy_to_iter(from_kaddr, total_size, iter);
>         return ret;
> }

Wrappers Are Evil: pvfs2_kmap().  It obfuscates the things for no reason
whatsoever, is actively wrong for a lot of reasons (kmap() slots are
system-wide resoure, and not too plentiful one, at that) _and_ obfuscates
the open-coding of existing primitive, badly: copy_page_to_iter()
does the same thing with less headache for caller and without a leak
(you forgot kunmap the damn thing afterwards).

> "cat filename" is a simple test that drives execution
> down the "Are we copying to User Virtual Addresses?"
> code path, and big and small files still copy
> out.
> 
> I haven't found a test case that drives execution down
> the "Are we copying to Kern Virtual Addresses?" code
> path yet.

Ummm...  GrepTFS?  pvfs_bufmap_copy_to_kernel_iovec() called from
postcopy_buffers() when the last argument is 0, which is called from
wait_for_direct_io() in the same conditions plus the first argument being
PVFS_IO_READ.  Said function has only two callers, one in do_readv_writev()
(the last argument is 1, out of consideration), another in pvfs2_inode_read().
Which does
+       ret = wait_for_direct_io(PVFS_IO_READ, inode, offset, &vec, 1,
+                       count, readahead_size, 0);
which should hit pvfs_bufmap_copy_to_kernel_iovec() just fine...

Incidentally, it's misannotated -
+ssize_t pvfs2_inode_read(struct inode *inode,
+                        char __user *buf,
+                        size_t count,
+                        loff_t *offset,
+                        loff_t readahead_size)
+{
claims buf to be a userland pointer, while its only caller is passing
kmap(some page) there.  This
                bytes_read = pvfs2_inode_read(inode,
-                                             page_data,
+                                             (char __user *) page_data,
                                              blocksize,
                                              &blockptr_offset,
in later commit forces sparce to STFU, but the point of __user annotations is
to be able to keep track which pointers are which, after all...

Oh, and to continue GTFS, the only caller of pvfs2_inode_read() appears
to be read_one_page(), called from pvfs2_readpage() and pvfs2_readpages(),
which are ->readpage() and ->readpages() methods in pvfs2_address_operations.
So I'd suggest trying to mmap and dereference.  Or execve() of a binary
there...

BTW, this
+       /* map the pages */
+       down_write(&current->mm->mmap_sem);
+       ret = get_user_pages(current,
+                            current->mm,
+                            (unsigned long)user_desc->ptr,
+                            bufmap->page_count,
+                            1,
+                            0,
+                            bufmap->page_array,
+                            NULL);
+       up_write(&current->mm->mmap_sem);
should almost certainly be replaced with
	ret = get_user_pages_fast((unsigned long)user_desc->ptr,
				  bufmap->page_count, 1,bufmap->page_array);
and let it deal with ->mmap_sem.