cvs commit: src/sys/net if_ethersubr.c

[Luigi Rizzo]

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...

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 ?

	cheers
	luigi