Re: xen3pae on 5.0_stable/i386

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

On 01/23/10 02:01, Christoph Egger wrote:
> So we have address overflow in xennet and in xengnt.
> Some more variables storing physical addresses must be of type paddr_t.
[snip]
> Index: sys/arch/xen/xen/if_xennet_xenbus.c
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/xen/xen/if_xennet_xenbus.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 if_xennet_xenbus.c
> --- sys/arch/xen/xen/if_xennet_xenbus.c 19 Jan 2010 22:06:23 -0000      1.40
> +++ sys/arch/xen/xen/if_xennet_xenbus.c 23 Jan 2010 00:56:14 -0000
> @@ -208,7 +208,7 @@ struct xennet_xenbus_softc {
>
>  /* too big to be on stack */
>  static multicall_entry_t rx_mcl[NET_RX_RING_SIZE+1];
> -static u_long xennet_pages[NET_RX_RING_SIZE];
> +static paddr_t xennet_pages[NET_RX_RING_SIZE];
>
>  static int  xennet_xenbus_match(device_t, cfdata_t, void *);
>  static void xennet_xenbus_attach(device_t, device_t, void *);
> @@ -604,7 +604,7 @@ xennet_alloc_rx_buffer(struct xennet_xen
>         xpq_flush_queue();
>         splx(s2);
>         /* now decrease reservation */
> -       xenguest_handle(reservation.extent_start) = xennet_pages;
> +       xenguest_handle(reservation.extent_start) = (u_long *)xennet_pages; /* 
> XXX */
>         reservation.nr_extents = i;
>         reservation.extent_order = 0;
>         reservation.address_bits = 0;

Are you sure xennet_pages contains physical addresses and not frame 
numbers (as it is used elsewhere with xpmap_phys_to_machine_mapping)?

Having unsigned long is fine when using PFNs, even under PAE.

> Index: sys/arch/xen/xen/xengnt.c
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/xen/xen/xengnt.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 xengnt.c
> --- sys/arch/xen/xen/xengnt.c   7 Nov 2009 07:27:49 -0000       1.16
> +++ sys/arch/xen/xen/xengnt.c   23 Jan 2010 00:56:14 -0000
> @@ -127,20 +127,20 @@ static int
>  xengnt_more_entries(void)
>  {
>         gnttab_setup_table_t setup;
> -       unsigned long *pages;
> +       paddr_t *pages;
>         int nframes_new = gnt_nr_grant_frames + 1;
>         int i;
>
>         if (gnt_nr_grant_frames == gnt_max_grant_frames)
>                 return ENOMEM;
>
> -       pages = malloc(nframes_new * sizeof(long), M_DEVBUF, M_NOWAIT);
> +       pages = malloc(nframes_new * sizeof(paddr_t), M_DEVBUF, M_NOWAIT);
>         if (pages == NULL)
>                 return ENOMEM;
>
>         setup.dom = DOMID_SELF;
>         setup.nr_frames = nframes_new;
> -       xenguest_handle(setup.frame_list) = pages;
> +       xenguest_handle(setup.frame_list) = (unsigned long *)pages; /* XXX */

Same here

>         /*
>          * setup the grant table, made of nframes_new frames
> @@ -156,7 +156,7 @@ xengnt_more_entries(void)
>                 return ENOMEM;
>         }
>
> -       DPRINTF(("xengnt_more_entries: map 0x%lx -> %p\n",
> +       DPRINTF(("xengnt_more_entries: map 0x%"PRIxPADDR" -> %p\n",
>             pages[gnt_nr_grant_frames],
>             (char *)grant_table + gnt_nr_grant_frames * PAGE_SIZE));
>
> @@ -165,7 +165,8 @@ xengnt_more_entries(void)
>          * the grant table frames
>          */
>         pmap_kenter_ma(((vaddr_t)grant_table) + gnt_nr_grant_frames * PAGE_SIZE,
> -           pages[gnt_nr_grant_frames] << PAGE_SHIFT, VM_PROT_WRITE, 0);
> +           pages[gnt_nr_grant_frames] << PAGE_SHIFT,
> +           VM_PROT_WRITE, 0);

Now that one is bad, the shift is done on an unsigned long (in -current 
context), which will hide away the high order bits in case of PAE. 
'pages' should be casted to (paddr_t) (or its type promoted to paddr_t 
instead of unsigned long, but IMHO it is overkill as it seems to track 
PFNs).


<side_note>
On a more general perspective, MD code is messy with Xen and PAE, 
because there is an assumption that obtaining the page address is as 
easy as PAGE_SHIFTing the PFN, which is wrong. Xen handles PFNs as 
"unsigned long" and not "unsigned long long" in its API; if we are not 
careful enough, we may lose bits in the shift process. Bha.
</side_note>

> Manuel: I am not sure if attached patch is correct (only compile
> tested), but it points into the right direction at least.

Cheers!

--
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost