Re: kern/53562: bridge(4) breaks segmentation / TX checksum offloading

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

The following reply was made to PR kern/53562; it has been noted by GNATS.

From: Masanobu SAITOH <msaitoh%execsw.org@localhost>
To: Christoph Badura <bad%bsd.de@localhost>, Rin Okuyama <rokuyama%rk.phys.keio.ac.jp@localhost>
Cc: msaitoh%execsw.org@localhost, Ryota Ozaki <ozaki-r%netbsd.org@localhost>,
 "gnats-bugs%NetBSD.org@localhost" <gnats-bugs%netbsd.org@localhost>, kern-bug-people%netbsd.org@localhost,
 gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/53562: bridge(4) breaks segmentation / TX checksum
 offloading
Date: Fri, 7 Sep 2018 17:50:58 +0900

 On 2018/09/06 6:51, Christoph Badura wrote:
 > On Wed, Sep 05, 2018 at 09:04:16PM +0900, Rin Okuyama wrote:
 >> With both patches, TX works fine through an interface added to bridge,
 >> even if TSO is enabled. However, RX from an host which shares the
 >> interface via bridge does not work for larger packets (because of TSO?).
 >> RX of smaller packets and TX work for that host.
 > 
 > I would not expect any TSO flags to be set on a packet that is received
 > from another host.  The packet should have been segmented on other host
 > already.
 > 
 >> I'm not sure if my setup should work in principle...
 > 
 > Maybe you could describe your setup in more detail.  Looking at the PR I
 > don't understand what exactly your setup is.
 
 I think so, too. I read if_bridge.c and I think ozaki-r's change should be
 correct. There might be another bug. The detailed configuration is required
 to reproduce the problem.
 
 
 > It might be worth going over the original PR kern/27007 again.  Also note
 > the second part of PR kern/21831 by yamt@.
 > 
 > Note that in kern/27007 the damage was actually done only when ipf was
 > enabled on the bridge interface.  It might well be that my change was
 > not correct because it was to broad.  Maybe ipf has changed since, so that
 > packets do not get corrupted anymore in this case, too.
 > 
 > Looking back at the code I think the comment could have been worded
 > better.  If you move it out of bridge_enqueue() it would be nice if the
 > comment could be expanded.  Like, what exactly the circumstances are that
 > the flags have been set and why they need clearing.
 
 New patch:
 Index: if_bridge.c
 ===================================================================
 RCS file: /cvsroot/src/sys/net/if_bridge.c,v
 retrieving revision 1.156
 diff -u -p -r1.156 if_bridge.c
 --- if_bridge.c	25 May 2018 04:40:27 -0000	1.156
 +++ if_bridge.c	7 Sep 2018 08:47:47 -0000
 @@ -1395,11 +1395,6 @@ bridge_enqueue(struct bridge_softc *sc,
   	int len, error;
   	short mflags;
   
 -	/*
 -	 * Clear any in-bound checksum flags for this packet.
 -	 */
 -	m->m_pkthdr.csum_flags = 0;
 -
   	if (runfilt) {
   		if (pfil_run_hooks(sc->sc_if.if_pfil, &m,
   		    dst_ifp, PFIL_OUT) != 0) {
 @@ -1768,6 +1763,13 @@ bridge_forward(struct bridge_softc *sc,
   
   	bridge_release_member(sc, bif, &psref);
   
 +	/*
 +	 * Before enqueueing this packet to the destination interface,
 +	 * clear any in-bound checksum flags to prevent them from being
 +	 * misused as out-bound flags.
 +	 */
 +	m->m_pkthdr.csum_flags = 0;
 +
   	ACQUIRE_GLOBAL_LOCKS();
   	bridge_enqueue(sc, dst_if, m, 1);
   	RELEASE_GLOBAL_LOCKS();
 @@ -1978,6 +1980,13 @@ bridge_broadcast(struct bridge_softc *sc
   				sc->sc_if.if_oerrors++;
   				goto next;
   			}
 +			/*
 +			 * Before enqueueing this packet to the destination
 +			 * interface, clear any in-bound checksum flags to
 +			 * prevent them from being misused as out-bound flags.
 +			 */
 +			m->m_pkthdr.csum_flags = 0;
 +
   			ACQUIRE_GLOBAL_LOCKS();
   			bridge_enqueue(sc, dst_if, mc, 1);
   			RELEASE_GLOBAL_LOCKS();
 
 
 > Maybe we need ATF tests.  That certainly would be nice.  Also we now have
 > more test cases to consider: npf and pf active on the bridge.
 
 I think adding offload functions into shmif(4) is a good idea to test offload
 stuff.
 
 > 
 > --chris
 > 
 
 
 -- 
 -----------------------------------------------
                  SAITOH Masanobu (msaitoh%execsw.org@localhost
                                   msaitoh%netbsd.org@localhost)