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

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

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)