Bug in use of grant tables in blkback.c error path?

Bug in use of grant tables in blkback.c error path?

[Harry Butterworth]

In dispatch_rw_block_io after a call to HYPERVISOR_grant_table_op, there
is the following code which calls fast_flush_area and breaks out of the
loop early if one of the handles returned from HYPERVISOR_grant_table_op
is negative:

	for (i = 0; i < nseg; i++) {
		if (unlikely(map[i].handle < 0)) {
			DPRINTK("invalid buffer -- could not remap it\n");
			fast_flush_area(pending_idx, nseg);
			goto bad_descriptor;
		}

		phys_to_machine_mapping[__pa(MMAP_VADDR(
			pending_idx, i)) >> PAGE_SHIFT] =
			FOREIGN_FRAME(map[i].dev_bus_addr >> PAGE_SHIFT);

		pending_handle(pending_idx, i) = map[i].handle;
	}

The implementation of fast_flush_area uses pending_handle and is called
to flush the whole range nseg but the loop above is setting up
pending_handle as it goes along so the handles for the pages after the
erroneous one are not set up when fast_flush area is called.

Here's the bit of fast_flush_area that uses pending_handle:

	for (i = 0; i < nr_pages; i++) {
		handle = pending_handle(idx, i); <<<<<<<<<<<<<<<<<
		if (handle == BLKBACK_INVALID_HANDLE)
			continue;
		unmap[i].host_addr      = MMAP_VADDR(idx, i);
		unmap[i].dev_bus_addr   = 0;
		unmap[i].handle         = handle;
		pending_handle(idx, i)  = BLKBACK_INVALID_HANDLE;
		invcount++;
	}

I also checked the implementation of gnttab_map_grant_ref:

static long
gnttab_map_grant_ref(
    gnttab_map_grant_ref_t *uop, unsigned int count)
{
    int i;

    for ( i = 0; i < count; i++ )
        (void)__gnttab_map_grant_ref(&uop[i]);

    return 0;
}

gnttab_map_grant_ref seems to keep going and attempt to map the
remaining pages after an error is encountered.

All in all, this looks like a bug to me where failure to map a grant
reference in the middle of a set would result in pages kept mapped in
the backend when fast_flush_area fails to clean them up.

Am I right?

Harry.

-- 
Harry Butterworth <harry@hebutterworth.freeserve.co.uk>

Re: Bug in use of grant tables in blkback.c error path?

[Keir Fraser]

On 6 Nov 2005, at 12:24, Harry Butterworth wrote:

> All in all, this looks like a bug to me where failure to map a grant
> reference in the middle of a set would result in pages kept mapped in
> the backend when fast_flush_area fails to clean them up.
>
> Am I right?

Yes, it's a bug. We'll sort out a patch.

  -- Keir

Re: Bug in use of grant tables in blkback.c error path?

[Harry Butterworth]

On Sun, 2005-11-06 at 15:32 +0000, Keir Fraser wrote:
> On 6 Nov 2005, at 12:24, Harry Butterworth wrote:
> 
> > All in all, this looks like a bug to me where failure to map a grant
> > reference in the middle of a set would result in pages kept mapped in
> > the backend when fast_flush_area fails to clean them up.
> >
> > Am I right?
> 
> Yes, it's a bug. We'll sort out a patch.
> 

OK.  I ended up doing the following in my code.  I'm not particularly
confident about it but hopefully it's correct.  As in blkback, the
phys_to_machine_mapping gets left lying around after unmap.  I assume
this is not a problem.

typedef struct xenidc_grant_table_mapping_context_struct
  xenidc_grant_table_mapping_context;

struct xenidc_grant_table_mapping_context_struct
{
    xenidc_buffer_mappable_class    * class;
    xenidc_buffer_resource_provider * provider;
    unsigned long                     page_count;
    unsigned long                     mmap_vaddress;
    int16_t                           handle
      [ XENIDC_REMOTE_BUFFER_REFERENCE_GRANT_TABLE_REFERENCE_COUNT ];
};

static void xenidc_grant_table_unmap_rbr
  ( xenidc_buffer_mappable_class ** context_in );

static xenidc_buffer_mappable_class ** xenidc_grant_table_map_rbr
(
    xenidc_buffer_mappable_class     * class,
    xenidc_remote_buffer_reference   * rbr,
    xenidc_address                   * address,
    xenidc_buffer_resource_provider  * provider,
    void                            ** mapping
)
{
    trace();

    {
        xenidc_grant_table_mapping_context * context =
          xenidc_buffer_resource_provider_allocate_page( provider );

        struct gnttab_map_grant_ref map
         [ XENIDC_REMOTE_BUFFER_REFERENCE_GRANT_TABLE_REFERENCE_COUNT ];

        unsigned long first_page = rbr->byte_offset / PAGE_SIZE;
        unsigned long final_page =
          ( rbr->byte_offset + rbr->byte_count - 1 ) / PAGE_SIZE;

        context->class = class;

        context->provider = provider;

        context->page_count = final_page - first_page + 1;

        context->mmap_vaddress =
          xenidc_buffer_resource_provider_allocate_empty_page_range
            ( provider, context->page_count );

        {
            int i;

            for( i = 0; i < context->page_count; i++ )
            {
                map[ i ].host_addr =
                  context->mmap_vaddress + ( i * PAGE_SIZE );
                map[ i ].dom       =
                  xenidc_address_query_remote_domain_id( address );
                map[ i ].ref       =
                  rbr->base.grant_table.reference[ first_page + i ];
                map[ i ].flags     = GNTMAP_host_map;
            }
        }

        {
            int error = HYPERVISOR_grant_table_op
              ( GNTTABOP_map_grant_ref, map, context->page_count );

            BUG_ON( error );
        }

        {
            int error = 0;

            {
                int i;

                for( i = 0; i < context->page_count; i++ )
                {
                    context->handle[ i ] = map[ i ].handle;

                    if( unlikely( map[ i ].handle < 0 ) )
                    {
                        error = 1;
                    }
                }
            }

            if( !error )
            {
                int i;

                for( i = 0; i < context->page_count; i++ )
                {
                    phys_to_machine_mapping
                    [
                        __pa( context->mmap_vaddress + ( i *
PAGE_SIZE ) )
                        >>
                        PAGE_SHIFT
                    ]
                    =
                    FOREIGN_FRAME( map[ i ].dev_bus_addr >>
PAGE_SHIFT );
                }
            }
            else
            {
                xenidc_grant_table_unmap_rbr( &context->class );

                context = NULL;
            }
        }

        if( context != NULL )
        {
            *mapping =
              (void *)( context->mmap_vaddress + ( rbr->byte_offset %
PAGE_SIZE ) );

            return &context->class;
        }
        else
        {
            return NULL;
        }
    }
}

static void xenidc_grant_table_unmap_rbr
  ( xenidc_buffer_mappable_class ** context_in )
{
    trace();

    {
        xenidc_grant_table_mapping_context * context = container_of
          ( context_in, xenidc_grant_table_mapping_context, class );

        {
            struct gnttab_unmap_grant_ref unmap

[ XENIDC_REMOTE_BUFFER_REFERENCE_GRANT_TABLE_REFERENCE_COUNT ];

            int i;
            int j;

            for( i = 0, j = 0; i < context->page_count; i++ )
            {
                if( context->handle[ i ] >= 0 )
                {
                    unmap[ j ].host_addr    =
                      context->mmap_vaddress + ( i * PAGE_SIZE );
                    unmap[ j ].dev_bus_addr = 0;
                    unmap[ j ].handle       = context->handle[ i ];

                    j++;
                }
            }

            if( j != 0 )
            {
                int error = HYPERVISOR_grant_table_op
                  ( GNTTABOP_unmap_grant_ref, unmap, j );

                BUG_ON( error );
            }
        }

        xenidc_buffer_resource_provider_free_empty_page_range
          ( context->provider, context->mmap_vaddress );

        xenidc_buffer_resource_provider_free_page
          ( context->provider, context );
    }
}



>   -- Keir
> 
> 
-- 
Harry Butterworth <harry@hebutterworth.freeserve.co.uk>