xfs
[Top] [All Lists]

Re: [PATCH 2/2] dmapi: use ->put_listen callback directly

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 2/2] dmapi: use ->put_listen callback directly
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 21 Nov 2008 12:58:46 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080603114848.GC2703@xxxxxx>
References: <20080603114848.GC2703@xxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
Any chance to get this in after almost six month?

On Tue, Jun 03, 2008 at 01:48:48PM +0200, Christoph Hellwig wrote:
> Rewrite xfs_dm_getall_dmattr to directly call xfs_list_attr_int in
> fill the userspace buffer directly from the listent. Doing the direct
> put means the need for the large kernel space buffer is gone, there
> is only one btree traversal needed and thus the list vs get race
> is fixed.  By doing the proper put_user/copy_to_user the direct
> userspace memory derference is fixed, too and the code is a lot
> simpler and easier to understand.
> 
> This patch passes xfsqa but I don't really trust it much vs dmapi,
> so please review carefully.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: linux-2.6-xfs/fs/xfs/dmapi/xfs_dm.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/dmapi/xfs_dm.c  2008-06-01 14:45:20.000000000 
> +0200
> +++ linux-2.6-xfs/fs/xfs/dmapi/xfs_dm.c       2008-06-03 12:00:22.000000000 
> +0200
> @@ -2092,149 +2092,136 @@ xfs_dm_get_region(
>  }
>  
>  
> +#define DM_GETALL_DMATTR_ALIGN       (sizeof(int) - 1)
> +
>  STATIC int
> -xfs_dm_getall_dmattr(
> -     struct inode    *inode,
> -     dm_right_t      right,
> -     size_t          buflen,
> -     void            __user *bufp,
> -     size_t          __user *rlenp)
> +xfs_dm_put_listent(
> +     xfs_attr_list_context_t *context,
> +     int                     flags,
> +     char                    *name,
> +     int                     namelen,
> +     int                     valuelen,
> +     char                    *value)
>  {
> -     attrlist_cursor_kern_t cursor;
> -     attrlist_t      *attrlist;
> -     dm_attrlist_t   __user *ulist;
> -     int             *last_link;
> -     int             alignment;
> -     int             total_size;
> -     int             list_size = 8192;       /* should be big enough */
> -     int             error;
> +     dm_attrlist_t __user    *ulist;
> +     int                     size_needed;
>  
> -     /* Returns negative errors to DMAPI */
> +     ASSERT(context->count >= 0);
>  
> -     if (right < DM_RIGHT_SHARED)
> -             return(-EACCES);
> +     /*
> +      * Skip over all non-DMAPI attributes.  If the attribute name is too
> +      * long, we assume it is non-DMAPI even if it starts with the correct
> +      * prefix.
> +      */
> +     if (!(flags & XFS_ATTR_ROOT))
> +             return 0;
> +     if (namelen > DM_ATTR_NAME_SIZE)
> +             return 0;
> +     if (strncmp(name, dmattr_prefix, DMATTR_PREFIXLEN))
> +             return 0;
>  
> -     /* Verify that the user gave us a buffer that is 4-byte aligned, lock
> -        it down, and work directly within that buffer.  As a side-effect,
> -        values of buflen < sizeof(int) return EINVAL.
> -     */
> +     size_needed = sizeof(dm_attrlist_t) + valuelen;
> +     size_needed = (size_needed + DM_GETALL_DMATTR_ALIGN) &
> +                     ~DM_GETALL_DMATTR_ALIGN;
>  
> -     alignment = sizeof(int) - 1;
> -     if ((((__psint_t)bufp & alignment) != 0) ||
> -               !access_ok(VERIFY_WRITE, bufp, buflen)) {
> -             return(-EFAULT);
> +     /*
> +      * We have a valid DMAPI attribute to return.  If it won't fit in the
> +      * user's buffer, we still need to keep track of the number of bytes
> +      * for the user's next call.
> +      */
> +     context->count += size_needed;
> +     if (context->count > context->bufsize) {
> +             context->seen_enough = 1;
> +             return 1;
> +     }
> +
> +     ulist = (dm_attrlist_t __user __force *)
> +              (context->alist + context->count);
> +
> +     if (copy_to_user(ulist->al_name.an_chars, name, namelen) ||
> +         clear_user(ulist->al_name.an_chars + namelen,
> +                     DM_ATTR_NAME_SIZE - namelen) ||
> +         put_user(sizeof(dm_attrlist_t), &ulist->al_data.vd_offset) ||
> +         put_user(valuelen, &ulist->al_data.vd_length) ||
> +         put_user(size_needed, &ulist->_link) ||
> +         copy_to_user(ulist + 1, value, valuelen)) {
> +             context->count = -1;
> +             return 1;
>       }
> -     buflen &= ~alignment;           /* round down the alignment */
>  
> -     /* Initialize all the structures and variables for the main loop. */
> +     context->last_link = &ulist->_link;
> +     return 0;
> +}
>  
> -     memset(&cursor, 0, sizeof(cursor));
> -     attrlist = (attrlist_t *)kmem_alloc(list_size, KM_SLEEP);
> -     total_size = 0;
> -     ulist = (dm_attrlist_t *)bufp;
> -     last_link = NULL;
> -
> -     /* Use vop_attr_list to get the names of DMAPI attributes, and use
> -        vop_attr_get to get their values.  There is a risk here that the
> -        DMAPI attributes could change between the vop_attr_list and
> -        vop_attr_get calls.  If we can detect it, we return EIO to notify
> -        the user.
> -     */
> +STATIC int
> +xfs_dm_getall_dmattr(
> +     struct inode            *inode,
> +     dm_right_t              right,
> +     size_t                  buflen,
> +     void                    __user *bufp,
> +     size_t                  __user *rlenp)
> +{
> +     xfs_attr_list_context_t context;
> +     attrlist_cursor_kern_t  cursor = { 0, };
> +     int                     error;
>  
> -     do {
> -             int     i;
> +     if (right < DM_RIGHT_SHARED)
> +             return -EACCES;
>  
> -             /* Get a buffer full of attribute names.  If there aren't any
> -                more or if we encounter an error, then finish up.
> -             */
> +     /* verify that the user gave us a buffer that is 4-byte aligned */
> +     if ((unsigned long)bufp & DM_GETALL_DMATTR_ALIGN)
> +             return -EFAULT;
>  
> -             error = xfs_attr_list(XFS_I(inode), (char *)attrlist, list_size,
> -                                             ATTR_ROOT, &cursor);
> -             DM_EA_XLATE_ERR(error);
> +     /* round down the alignment */
> +     buflen &= ~DM_GETALL_DMATTR_ALIGN;
>  
> -             if (error || attrlist->al_count == 0)
> -                     break;
> +     memset(&context, 0, sizeof(context));
> +     context.dp = XFS_I(inode);
> +     context.cursor = &cursor;
> +     context.resynch = 1;
> +     context.flags = ATTR_ROOT;
> +     /* alist is just a cookie */
> +     context.alist = (char * __force)bufp;
> +     context.bufsize = buflen;
> +     context.put_value = 1;
> +     context.put_listent = xfs_dm_put_listent;
>  
> -             for (i = 0; i < attrlist->al_count; i++) {
> -                     attrlist_ent_t  *entry;
> -                     char            *user_name;
> -                     int             size_needed;
> -                     int             value_len;
> -
> -                     /* Skip over all non-DMAPI attributes.  If the
> -                        attribute name is too long, we assume it is
> -                        non-DMAPI even if it starts with the correct
> -                        prefix.
> -                     */
> -
> -                     entry = ATTR_ENTRY(attrlist, i);
> -                     if (strncmp(entry->a_name, dmattr_prefix, 
> DMATTR_PREFIXLEN))
> -                             continue;
> -                     user_name = &entry->a_name[DMATTR_PREFIXLEN];
> -                     if (strlen(user_name) > DM_ATTR_NAME_SIZE)
> -                             continue;
> -
> -                     /* We have a valid DMAPI attribute to return.  If it
> -                        won't fit in the user's buffer, we still need to
> -                        keep track of the number of bytes for the user's
> -                        next call.
> -                     */
> -
> -
> -                     size_needed = sizeof(*ulist) + entry->a_valuelen;
> -                     size_needed = (size_needed + alignment) & ~alignment;
> -
> -                     total_size += size_needed;
> -                     if (total_size > buflen)
> -                             continue;
> -
> -                     /* Start by filling in all the fields in the
> -                        dm_attrlist_t structure.
> -                     */
> -
> -                     strncpy((char *)ulist->al_name.an_chars, user_name,
> -                             DM_ATTR_NAME_SIZE);
> -                     ulist->al_data.vd_offset = sizeof(*ulist);
> -                     ulist->al_data.vd_length = entry->a_valuelen;
> -                     ulist->_link =  size_needed;
> -                     last_link = &ulist->_link;
> -
> -                     /* Next read the attribute's value into its correct
> -                        location after the dm_attrlist structure.  Any sort
> -                        of error indicates that the data is moving under us,
> -                        so we return EIO to let the user know.
> -                     */
> -
> -                     value_len = entry->a_valuelen;
> -
> -                     error = xfs_attr_get(XFS_I(inode), entry->a_name,
> -                                             (void *)(ulist + 1), &value_len,
> -                                             ATTR_ROOT);
> -                     DM_EA_XLATE_ERR(error);
> -
> -                     if (error || value_len != entry->a_valuelen) {
> -                             error = EIO;
> -                             break;
> -                     }
> +     error = -xfs_attr_list_int(&context);
> +     ASSERT(error <= 0);
>  
> -                     ulist = (dm_attrlist_t *)((char *)ulist + ulist->_link);
> -             }
> -     } while (!error && attrlist->al_more);
> -     if (last_link)
> -             *last_link = 0;
> -
> -     if (!error && total_size > buflen)
> -             error = E2BIG;
> -     if (!error || error == E2BIG) {
> -             if (put_user(total_size, rlenp))
> -                     error = EFAULT;
> +     /*
> +      * Count smaller zero means the iterator got an EFAULT.
> +      *
> +      * XXX(hch): we badly need a better way to deal with this, including
> +      *           passing and actual errno value back from the iterator.
> +      */
> +     if (context.count < 0)
> +             return -EFAULT;
> +
> +     /*
> +      * Terminate the list if we were successfull.
> +      */
> +     if (context.last_link && put_user(0, context.last_link))
> +             return -EFAULT;
> +
> +     if (!error) {
> +             /*
> +              * We still need to copy the count member to userspace in the
> +              * case where there was not enough buffer space for the request.
> +              *
> +              * The ABI defines a count bigger as the passed in buffer length
> +              * as indicator for requiring to call again with a bigger 
> buffer.
> +              */
> +             if (context.count > buflen)
> +                     error = -E2BIG;
> +
> +             if (put_user(context.count, rlenp))
> +                     error = -EFAULT;
>       }
>  
> -     kmem_free(attrlist);
> -     return(-error); /* Return negative error to DMAPI */
> +     return error;
>  }
>  
> -
>  /* ARGSUSED */
>  STATIC int
>  xfs_dm_getall_inherit(
> Index: linux-2.6-xfs/fs/xfs/xfs_attr.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.h      2008-06-03 09:56:25.000000000 
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.h   2008-06-03 11:45:11.000000000 +0200
> @@ -119,6 +119,7 @@ typedef struct xfs_attr_list_context {
>       int                             put_value;      /* T/F: need value for 
> listent */
>       put_listent_func_t              put_listent;    /* list output fmt 
> function */
>       int                             index;          /* index into output 
> buffer */
> +     int __user                      *last_link;     /* list terminator */
>  } xfs_attr_list_context_t;
>  
>  
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ksyms.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_ksyms.c   2008-06-03 
> 12:01:08.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ksyms.c        2008-06-03 
> 12:01:13.000000000 +0200
> @@ -262,7 +262,7 @@ EXPORT_SYMBOL(xfs_attr_get);
>  EXPORT_SYMBOL(xfs_attr_set);
>  EXPORT_SYMBOL(xfs_fsync);
>  EXPORT_SYMBOL(xfs_attr_remove);
> -EXPORT_SYMBOL(xfs_attr_list);
> +EXPORT_SYMBOL(xfs_attr_list_int);
>  EXPORT_SYMBOL(xfs_readdir);
>  EXPORT_SYMBOL(xfs_setsize_buftarg);
>  EXPORT_SYMBOL(xfs_syncsub);
> 
> 
---end quoted text---

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH 2/2] dmapi: use ->put_listen callback directly, Christoph Hellwig <=