Date: Tue, 12 Dec 2006 15:17:59 +0100 From: Bernd Walter <ticso@cicely12.cicely.de> To: Luigi Rizzo <rizzo@icir.org> Cc: Luigi Rizzo <luigi@FreeBSD.org>, src-committers@FreeBSD.org, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org, imp@FreeBSD.org, ticso@cicely.de Subject: Re: cvs commit: src/sys/net if_ethersubr.c Message-ID: <20061212141759.GZ54209@cicely12.cicely.de> In-Reply-To: <20061212055756.A61182@xorpc.icir.org> References: <200612081036.kB8AakMD029277@repoman.freebsd.org> <20061212131333.GU54209@cicely12.cicely.de> <20061212055756.A61182@xorpc.icir.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Dec 12, 2006 at 05:57:56AM -0800, Luigi Rizzo wrote:
> On Tue, Dec 12, 2006 at 02:13:34PM +0100, Bernd Walter wrote:
> > On Fri, Dec 08, 2006 at 10:36:46AM +0000, Luigi Rizzo wrote:
> > > luigi 2006-12-08 10:36:45 UTC
> > >
> > > FreeBSD src repository
> > >
> > > Modified files:
> > > sys/net if_ethersubr.c
> > > Log:
> > > Fix an oscure bug triggered by a recent change in kern_socket.c.
> > > The symptoms were that outgoing DHCP requests for diskless kernels
> > > had the IP header corrupt. After long investigations, the source of
> > > the problem was found in ether_output() - for SIMPLEX interfaces
> > > and broadcast traffic, a copy of the packet is passed back to the kernel
> > > through if_simloop(). However if_simloop() modifies the mbuf, while
> > > the copy obtained through m_copym() is a readonly one.
> > >
> > > The bug has been there forever, but it has been triggered only recently
> > > by a change in sosend_dgram() which passed down mbufs with sufficient
> > > space to prepend the header.
> > >
> > > This fix is trivial - use m_dup() instead of m_copy() to create
> > > the copy. As an alternative, we could try and modify if_simloop()
> > > to play safely with readonly mbufs, but i don't think it is worthwhile
> > > because 1) this is a relatively infrequent code path so we do not need
> > > to worry too much about performance, and 2) the cost of doing an
> > > extra m_pullup in if_simloop() is probably the same as doing the
> > > copy of the cluster, anyways.
> >
> > This change produces an alignment panic on arm.
> > Reverting it gets my system back to live.
>
> then i suppose the proper fix is to revert to m_copy() and
> work on if_simloop() so that 1. it handles a readonly chain, and
> 2. when doing so, it passes up a properly aligned packet...
Can't comment on this, as I don't have enough knowledge about network
code.
According to the xscale report it was likely never properly aligned,
the alignment obviously just moved with your change.
> however note that there is already some code in net/if_loop.c::if_simloop(),
> just that it uses this:
>
> #if defined(__ia64__) || defined(__sparc64__)
> /*
> * Some archs do not like unaligned data, so
> * we move data down in the first mbuf.
> */
> if (mtod(m, vm_offset_t) & 3) {
> KASSERT(hlen >= 3, ("if_simloop: hlen too small"));
> bcopy(m->m_data,
> (char *)(mtod(m, vm_offset_t)
> - (mtod(m, vm_offset_t) & 3)),
> m->m_len);
> m->m_data -= (mtod(m,vm_offset_t) & 3);
> }
> #endif
> to detect whether the architecture is alignment-sensitive.
> Is there any other identifier that we can use to check ?
I wonder how many of these are missing __arm__?
--
B.Walter http://www.bwct.de http://www.fizon.de
bernd@bwct.de info@bwct.de support@fizon.de
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061212141759.GZ54209>