[PATCH 2/2] dmapi: use ->put_listen callback directly
Christoph Hellwig
hch at infradead.org
Fri Nov 21 11:58:46 CST 2008
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 at lst.de>
>
> 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---
More information about the xfs
mailing list