Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1263689 > unrolled thread

Re: fs: out of bounds on stack in iov_iter_advance

Started byAl Viro <viro@ZenIV.linux.org.uk>
First post2015-11-06 02:40 +0100
Last post2015-11-11 22:50 +0100
Articles 5 on this page of 25 — 6 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-06 02:40 +0100
    Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-06 03:20 +0100
      Re: fs: out of bounds on stack in iov_iter_advance Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-06 04:40 +0100
        Re: fs: out of bounds on stack in iov_iter_advance Jens Axboe <axboe@kernel.dk> - 2015-11-06 17:10 +0100
        Re: fs: out of bounds on stack in iov_iter_advance Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-11 03:30 +0100
          Re: fs: out of bounds on stack in iov_iter_advance Jens Axboe <axboe@kernel.dk> - 2015-11-11 03:30 +0100
            Re: fs: out of bounds on stack in iov_iter_advance Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-11 03:40 +0100
              Re: fs: out of bounds on stack in iov_iter_advance Jens Axboe <axboe@kernel.dk> - 2015-11-11 03:50 +0100
                Re: fs: out of bounds on stack in iov_iter_advance Jens Axboe <axboe@kernel.dk> - 2015-11-11 03:50 +0100
                  Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 04:10 +0100
                    Re: fs: out of bounds on stack in iov_iter_advance Jens Axboe <axboe@kernel.dk> - 2015-11-11 04:10 +0100
                Re: fs: out of bounds on stack in iov_iter_advance Jens Axboe <axboe@kernel.dk> - 2015-11-11 03:50 +0100
              Re: fs: out of bounds on stack in iov_iter_advance Sasha Levin <sasha.levin@oracle.com> - 2015-11-11 04:30 +0100
          Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 04:00 +0100
            Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 04:40 +0100
              Re: fs: out of bounds on stack in iov_iter_advance Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-11 05:40 +0100
                Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 08:50 +0100
                  Re: fs: out of bounds on stack in iov_iter_advance Stephen Rothwell <sfr@canb.auug.org.au> - 2015-11-11 09:20 +0100
                    Re: fs: out of bounds on stack in iov_iter_advance Stephen Rothwell <sfr@canb.auug.org.au> - 2015-11-11 11:30 +0100
                      Re: fs: out of bounds on stack in iov_iter_advance Mike Marshall <hubcap@omnibond.com> - 2015-11-11 17:30 +0100
                        Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 17:40 +0100
                          Re: fs: out of bounds on stack in iov_iter_advance Mike Marshall <hubcap@omnibond.com> - 2015-11-11 18:00 +0100
                    Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 11:30 +0100
                      Re: fs: out of bounds on stack in iov_iter_advance Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-11 17:40 +0100
                        Re: fs: out of bounds on stack in iov_iter_advance Stephen Rothwell <sfr@canb.auug.org.au> - 2015-11-11 22:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1267299

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2015-11-11 17:40 +0100
Message-ID<qtGBb-5cy-1@gated-at.bofh.it>
In reply to#1267296
On Wed, Nov 11, 2015 at 11:25:17AM -0500, Mike Marshall wrote:
> I'm the Orangefs guy...
> 
> If the orangefs warnings that people see because of what's in
> linux-next is annoying, I could focus on quieting them down...

See the fixup just posted in this thread.

> We've been focusing on code review and documentation ever
> since our last big exchange with Al and Linus...

BTW, could you put the current state of the docs someplace public?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267319

FromMike Marshall <hubcap@omnibond.com>
Date2015-11-11 18:00 +0100
Message-ID<qtGUy-5jq-19@gated-at.bofh.it>
In reply to#1267299
 > BTW, could you put the current state of the docs someplace public?

The documentation will eventually end up in
Documentation/filesystems/orangefs.txt.

This part about the creation of the shared memory between userspace and
the kernel module seems complete and accurate to me so far. This "bufmap"
data structure is central to the protocol between userspace and the kernel
module. This describes the creation of the bufmap, details on how it is used
in exchanges is what I am working on now...

-----------------------------------------------------------------------------------------------------------

Orangefs is a user space filesystem and an associated kernel module.
We'll just refer to the user space part of Orangefs as "userspace"
from here on out...

The kernel module implements a pseudo device that userspace
can read from and write to. Userspace can also manipulate the
kernel module through the pseudo device with ioctl.

At startup userspace allocates two page-size-aligned (posix_memalign)
mlocked memory blocks, one is used for IO and one is used for readdir
operations. The IO block is 41943040 bytes and the readdir block is
4194304 bytes. Each block contains logical chunks, and a pointer to each
block is added to its own PVFS_dev_map_desc structure which also describes
its total size, as well as the size and number of the logical chunks.

A pointer to the IO block's PVFS_dev_map_desc structure is sent to a
mapping routine in the kernel module with an ioctl. The structure is
copied from user space to kernel space with copy_from_user and is used
to initialize the kernel module's "bufmap" (struct pvfs2_bufmap), which
then contains:

  * refcnt - a reference counter
  * desc_size - PVFS2_BUFMAP_DEFAULT_DESC_SIZE (4194304) the IO block's
    logical chunk size, which represents the filesystem's block size and
    is used for s_blocksize in super blocks.
  * desc_count - PVFS2_BUFMAP_DEFAULT_DESC_COUNT (10) the number of
    logical chunks in the IO block.
  * desc_shift - log2(desc_size), used for s_blocksize_bits in super blocks.
  * total_size - the total size of the IO block.
  * page_count - the number of 4096 byte pages in the IO block.
  * page_array - a pointer to page_count * (sizeof(struct page*)) bytes
    of kcalloced memory. This memory is used as an array of pointers
    to each of the pages in the IO block through a call to get_user_pages.
  * desc_array - a pointer to desc_count * (sizeof(struct pvfs_bufmap_desc))
    bytes of kcalloced memory. This memory is further intialized:

      user_desc is the kernel's copy of the IO block's PVFS_dev_map_desc
      structure. user_desc->ptr points to the IO block.

      pages_per_desc = bufmap->desc_size / PAGE_SIZE
      offset = 0

        bufmap->desc_array[0].page_array = &bufmap->page_array[offset]
        bufmap->desc_array[0].array_count = pages_per_desc = 1024
        bufmap->desc_array[0].uaddr = (user_desc->ptr) + (0 * 1024 * 4096)
        offset += 1024
                           .
                           .
                           .
        bufmap->desc_array[9].page_array = &bufmap->page_array[offset]
        bufmap->desc_array[9].array_count = pages_per_desc = 1024
        bufmap->desc_array[9].uaddr = (user_desc->ptr) +
                                               (9 * 1024 * 4096)
        offset += 1024

  * buffer_index_array - a desc_count sized array of ints, used to
    indicate which of the IO block's chunks are available to use.
  * buffer_index_lock - a spinlock to protect buffer_index_array during update.
  * readdir_index_array - a five (PVFS2_READDIR_DEFAULT_DESC_COUNT) element
    int array used to indicate which of the readdir block's chunks are
    available to use.
  * readdir_index_lock - a spinlock to protect readdir_index_array during
    update.

On Wed, Nov 11, 2015 at 11:36 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Nov 11, 2015 at 11:25:17AM -0500, Mike Marshall wrote:
>> I'm the Orangefs guy...
>>
>> If the orangefs warnings that people see because of what's in
>> linux-next is annoying, I could focus on quieting them down...
>
> See the fixup just posted in this thread.
>
>> We've been focusing on code review and documentation ever
>> since our last big exchange with Al and Linus...
>
> BTW, could you put the current state of the docs someplace public?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267093

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2015-11-11 11:30 +0100
Message-ID<qtAP8-1we-27@gated-at.bofh.it>
In reply to#1267032
On Wed, Nov 11, 2015 at 07:16:36PM +1100, Stephen Rothwell wrote:
> Hi Al,
> 
> On Wed, 11 Nov 2015 07:43:30 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >
> > dax_io fix isn't there, neither is overlayfs magic.h patch - both are
> > already in other trees.  I would like to get xattr series in as well,
> > but that's a separate pull request, if you'd accept them in this window in
> > the first place.  richacl stuff isn't there as well, and I think that one
> > is clear "leave it for 4.5" fodder.
> 
> So could you please remove the 4.5 stuff from your for-next branch
> until after the merge window closes.

Done.

> Also, I noticed these new warnings today:
> 
> fs/orangefs/xattr.c:509:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
>   .get = pvfs2_xattr_get_trusted,
>          ^
> fs/orangefs/xattr.c:509:9: note: (near initialization for 'pvfs2_xattr_trusted_handler.get')
> fs/orangefs/xattr.c:510:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
>   .set = pvfs2_xattr_set_trusted,
>          ^
> fs/orangefs/xattr.c:510:9: note: (near initialization for 'pvfs2_xattr_trusted_handler.set')
> fs/orangefs/xattr.c:520:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
>   .get = pvfs2_xattr_get_default,
>          ^
> fs/orangefs/xattr.c:520:9: note: (near initialization for 'pvfs2_xattr_default_handler.get')
> fs/orangefs/xattr.c:521:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
>   .set = pvfs2_xattr_set_default,
>          ^
> fs/orangefs/xattr.c:521:9: note: (near initialization for 'pvfs2_xattr_default_handler.set')

That's "xattr handlers: Pass handler to operations instead of flags" fallout,
trivially adjusted (typical change is
-ext2_xattr_security_list(struct dentry *dentry, char *list, size_t list_size,
-                        const char *name, size_t name_len, int type)
+ext2_xattr_security_list(const struct xattr_handler *handler,
+                        struct dentry *dentry, char *list, size_t list_size,
+                        const char *name, size_t name_len)
with type replaced with handler->flags if it's used anywhere in the body;
AFAICS, none of orangefs instances use it at all, so it's just a matter of
changing the argument lists in pvfs2_xattr_[gs]et_{default,trusted},
adding const struct xattr_handler *handler in the beginning and removing
the last argument; callers in pvfs2_ioctl() should simply use
pvfs2_inode_[gs]etxattr()).

Note, however, that orangefs in linux-next lacks a lot of fixes (see
vfs.git#orangefs-untested for some; AFAICS, those are missing from all
branches in orangefs git tree) and there are problems I don't know
how to fix, mostly due to the lack of documentation.  The last I've
heard from them was that they were putting such docs together; hopefully
once that get done we'll be able to sort the rest of that thing out.
It'll be after -rc1, though.

So xattr conflicts are the least of the problems there; those are easy
to adjust for, there are more serious issues in the entire thing ;-/
BTW, while we are at it - pvfs2_listxattr() doesn't even validate
resp.listxattr.returned_count, so a bogus response from buggered
server will do really interesting things to the kernel.

I'll cook the minimal fixup for API change after I get some sleep and
send it your way, unless somebody gets there first...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267310

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2015-11-11 17:40 +0100
Message-ID<qtGBd-5cy-37@gated-at.bofh.it>
In reply to#1267093
On Wed, Nov 11, 2015 at 10:19:48AM +0000, Al Viro wrote:

> I'll cook the minimal fixup for API change after I get some sleep and
> send it your way, unless somebody gets there first...

This should do it - switches ->ioctl() to pvfs2_inode_[gs]etxattr() and
converts xattr_handler ->[gs]et() to new API.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index feb1764..3d6ffe0 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -793,11 +793,10 @@ static long pvfs2_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	 */
 	if (cmd == FS_IOC_GETFLAGS) {
 		val = 0;
-		ret = pvfs2_xattr_get_default(file->f_path.dentry,
-					      "user.pvfs2.meta_hint",
-					      &val,
-					      sizeof(val),
-					      0);
+		ret = pvfs2_inode_getxattr(file_inode(file),
+					   PVFS2_XATTR_NAME_DEFAULT_PREFIX,
+					   "user.pvfs2.meta_hint",
+					   &val, sizeof(val));
 		if (ret < 0 && ret != -ENODATA)
 			return ret;
 		else if (ret == -ENODATA)
@@ -827,12 +826,10 @@ static long pvfs2_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		gossip_debug(GOSSIP_FILE_DEBUG,
 			     "pvfs2_ioctl: FS_IOC_SETFLAGS: %llu\n",
 			     (unsigned long long)val);
-		ret = pvfs2_xattr_set_default(file->f_path.dentry,
-					      "user.pvfs2.meta_hint",
-					      &val,
-					      sizeof(val),
-					      0,
-					      0);
+		ret = pvfs2_inode_setxattr(file_inode(file),
+					   PVFS2_XATTR_NAME_DEFAULT_PREFIX,
+					   "user.pvfs2.meta_hint",
+					   &val, sizeof(val), 0);
 	}
 
 	return ret;
diff --git a/fs/orangefs/pvfs2-kernel.h b/fs/orangefs/pvfs2-kernel.h
index 29b4a48..43339c6 100644
--- a/fs/orangefs/pvfs2-kernel.h
+++ b/fs/orangefs/pvfs2-kernel.h
@@ -237,19 +237,6 @@ extern const struct xattr_handler *pvfs2_xattr_handlers[];
 extern struct posix_acl *pvfs2_get_acl(struct inode *inode, int type);
 extern int pvfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type);
 
-int pvfs2_xattr_set_default(struct dentry *dentry,
-			    const char *name,
-			    const void *buffer,
-			    size_t size,
-			    int flags,
-			    int handler_flags);
-
-int pvfs2_xattr_get_default(struct dentry *dentry,
-			    const char *name,
-			    void *buffer,
-			    size_t size,
-			    int handler_flags);
-
 /*
  * Redefine xtvec structure so that we could move helper functions out of
  * the define
diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
index 227eaa4..b683daa 100644
--- a/fs/orangefs/xattr.c
+++ b/fs/orangefs/xattr.c
@@ -447,12 +447,12 @@ out_unlock:
 	return ret;
 }
 
-int pvfs2_xattr_set_default(struct dentry *dentry,
-			    const char *name,
-			    const void *buffer,
-			    size_t size,
-			    int flags,
-			    int handler_flags)
+static int pvfs2_xattr_set_default(const struct xattr_handler *handler,
+				   struct dentry *dentry,
+				   const char *name,
+				   const void *buffer,
+				   size_t size,
+				   int flags)
 {
 	return pvfs2_inode_setxattr(dentry->d_inode,
 				    PVFS2_XATTR_NAME_DEFAULT_PREFIX,
@@ -462,11 +462,11 @@ int pvfs2_xattr_set_default(struct dentry *dentry,
 				    flags);
 }
 
-int pvfs2_xattr_get_default(struct dentry *dentry,
-			    const char *name,
-			    void *buffer,
-			    size_t size,
-			    int handler_flags)
+static int pvfs2_xattr_get_default(const struct xattr_handler *handler,
+				   struct dentry *dentry,
+				   const char *name,
+				   void *buffer,
+				   size_t size)
 {
 	return pvfs2_inode_getxattr(dentry->d_inode,
 				    PVFS2_XATTR_NAME_DEFAULT_PREFIX,
@@ -476,12 +476,12 @@ int pvfs2_xattr_get_default(struct dentry *dentry,
 
 }
 
-static int pvfs2_xattr_set_trusted(struct dentry *dentry,
-			    const char *name,
-			    const void *buffer,
-			    size_t size,
-			    int flags,
-			    int handler_flags)
+static int pvfs2_xattr_set_trusted(const struct xattr_handler *handler,
+				   struct dentry *dentry,
+				   const char *name,
+				   const void *buffer,
+				   size_t size,
+				   int flags)
 {
 	return pvfs2_inode_setxattr(dentry->d_inode,
 				    PVFS2_XATTR_NAME_TRUSTED_PREFIX,
@@ -491,11 +491,11 @@ static int pvfs2_xattr_set_trusted(struct dentry *dentry,
 				    flags);
 }
 
-static int pvfs2_xattr_get_trusted(struct dentry *dentry,
-			    const char *name,
-			    void *buffer,
-			    size_t size,
-			    int handler_flags)
+static int pvfs2_xattr_get_trusted(const struct xattr_handler *handler,
+				   struct dentry *dentry,
+				   const char *name,
+				   void *buffer,
+				   size_t size)
 {
 	return pvfs2_inode_getxattr(dentry->d_inode,
 				    PVFS2_XATTR_NAME_TRUSTED_PREFIX,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267464

FromStephen Rothwell <sfr@canb.auug.org.au>
Date2015-11-11 22:50 +0100
Message-ID<qtLrc-8iQ-21@gated-at.bofh.it>
In reply to#1267310
Hi Al,

On Wed, 11 Nov 2015 16:33:39 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
> On Wed, Nov 11, 2015 at 10:19:48AM +0000, Al Viro wrote:
> 
> > I'll cook the minimal fixup for API change after I get some sleep and
> > send it your way, unless somebody gets there first...  
> 
> This should do it - switches ->ioctl() to pvfs2_inode_[gs]etxattr() and
> converts xattr_handler ->[gs]et() to new API.

Thanks, I will use that as a merge conflict fix patch from today.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web