Re: [PATCH] About the bug that makes pfinet crash

[Samuel Thibault]

Thomas Schwinge, le Sat 22 Nov 2008 22:54:21 +0100, a écrit :
> > believing
> > that the IP stack wouldn't overcome the typical 1500B MTU anyway.
> 
> It will, because you told it can do so, allowing for a PAGE_SIZE MTU.
> But: The MTU is defined to be without the ethernet headers, thus pfinet
> can create ethernet frames that are bigger than a page.

Ah, ok.

> Here is a fix.  OK to be committed?
> 
> Index: xen/net.c
> ===================================================================
> RCS file: /cvsroot/hurd/gnumach/xen/Attic/net.c,v
> retrieving revision 1.1.2.36
> diff -u -p -r1.1.2.36 net.c
> --- xen/net.c 22 Sep 2008 18:49:56 -0000      1.1.2.36
> +++ xen/net.c 22 Nov 2008 21:48:22 -0000
> @@ -383,9 +383,10 @@ void hyp_net_init(void) {
>               ifp = &nd->ifnet;
>               ifp->if_unit = n;
>               ifp->if_flags = IFF_UP | IFF_RUNNING;
> -             ifp->if_mtu = PAGE_SIZE;
>               ifp->if_header_size = 14;
>               ifp->if_header_format = HDR_ETHERNET;
> +             /* Set to the maximum that we can handle in device_write.  */
> +             ifp->if_mtu = PAGE_SIZE - ifp->if_header_size;

That's fine yes.

> Additionally, I'd suggest this change to better (?) explain what we're
> doing (and align to the other network device_write implementations).
> 
> Index: xen/net.c
> ===================================================================
> RCS file: /cvsroot/hurd/gnumach/xen/Attic/net.c,v
> retrieving revision 1.1.2.36
> diff -u -p -r1.1.2.36 net.c
> --- xen/net.c 22 Sep 2008 18:49:56 -0000      1.1.2.36
> +++ xen/net.c 22 Nov 2008 21:48:22 -0000
> @@ -483,13 +484,15 @@ device_write(void *d, ipc_port_t reply_p
>       vm_map_copy_t copy = (vm_map_copy_t) data;
>       grant_ref_t gref;
>       struct net_data *nd = d;
> +     struct ifnet *ifp = &nd->ifnet;
>       netif_tx_request_t *req;
>       unsigned reqn;
>       vm_offset_t offset;
>       vm_page_t m;
>       vm_size_t size;
>  
> -     if (count == 0 || count > PAGE_SIZE)
> +     if (count < ifp->if_header_size ||
> +         count > ifp->if_header_size + ifp->if_mtu)
>               return D_INVALID_SIZE;
>  
>       assert(copy->type == VM_MAP_COPY_PAGE_LIST);

Mmm, the < should be fine, but I'd rather see the > still use PAGE_SIZE,
or perhaps replace these PAGE_SIZE occurrences with a macro defed to
PAGE_SIZE.

Samuel