From owner-freebsd-net@FreeBSD.ORG  Thu Aug 21 20:51:59 2008
Return-Path: <owner-freebsd-net@FreeBSD.ORG>
Delivered-To: net@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id 84248106566B;
	Thu, 21 Aug 2008 20:51:59 +0000 (UTC)
	(envelope-from luigi@onelab2.iet.unipi.it)
Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.9.129])
	by mx1.freebsd.org (Postfix) with ESMTP id 43FA48FC1E;
	Thu, 21 Aug 2008 20:51:59 +0000 (UTC)
	(envelope-from luigi@onelab2.iet.unipi.it)
Received: by onelab2.iet.unipi.it (Postfix, from userid 275)
	id 1AB7A7308B; Thu, 21 Aug 2008 22:35:19 +0200 (CEST)
Date: Thu, 21 Aug 2008 22:35:19 +0200
From: Luigi Rizzo <rizzo@iet.unipi.it>
To: gnn@freebsd.org
Message-ID: <20080821203519.GA51534@onelab2.iet.unipi.it>
References: <m27iaa6v43.wl%gnn@neville-neil.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <m27iaa6v43.wl%gnn@neville-neil.com>
User-Agent: Mutt/1.4.2.3i
Cc: net@freebsd.org
Subject: Re: Small patch to multicast code...
X-BeenThere: freebsd-net@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: Networking and TCP/IP with FreeBSD <freebsd-net.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-net>,
	<mailto:freebsd-net-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-net>
List-Post: <mailto:freebsd-net@freebsd.org>
List-Help: <mailto:freebsd-net-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-net>,
	<mailto:freebsd-net-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 21 Aug 2008 20:51:59 -0000

On Thu, Aug 21, 2008 at 03:11:56PM -0400, gnn@freebsd.org wrote:
> Hi,
> 
> Turns out there is a bug in the code that loops back multicast
> packets.  If the underlying device driver supports checksum offloading
> then the packet that is looped back, when it is transmitted on the
> wire, is incorrect, due to the fact that the packet is not fully
> copied.
> 
> Here is a patch.  Comments welcome.
> 
> Best,
> George
> 
> Index: ip_output.c
> ===================================================================
> --- ip_output.c	(revision 181731)
> +++ ip_output.c	(working copy)
> @@ -1135,7 +1135,7 @@
>  	register struct ip *ip;
>  	struct mbuf *copym;
>  
> -	copym = m_copy(m, 0, M_COPYALL);
> +	copym = m_dup(m, M_DONTWAIT);
>  	if (copym != NULL && (copym->m_flags & M_EXT || copym->m_len < hlen))
>  		copym = m_pullup(copym, hlen);
>  	if (copym != NULL) {

I am slightly puzzled -- what is exactly the problem, i.e. what part
of the packet on the wire is incorrect ? The IP header is within hlen so
the m_pullup() should be enough to leave the original content intact.

The only thing i can think of is that it's the UDP checksum,
residing beyond hlen, which is overwritten somewhere in the
call to if_simloop -- in which case perhaps a better fix is
to m_pullup() the udp header as well ?

(in any case, it is worthwhile to add a comment to explain
what should be done -- the code paths using m_*() have become
quite fragile with these hw support enhancements that now
require selective modifications on previously shared, readonly buffers).

	cheers
	luigi