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

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

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

From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
To: Masanobu SAITOH <msaitoh%execsw.org@localhost>
Cc: "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: Mon, 3 Sep 2018 18:18:41 +0900

 On Mon, Sep 3, 2018 at 5:53 PM Masanobu SAITOH <msaitoh%execsw.org@localhost> wrote:
 >
 > On 2018/08/30 18:25, rokuyama%rk.phys.keio.ac.jp@localhost wrote:
 > >> Number:         53562
 > >> Category:       kern
 > >> Synopsis:       bridge(4) breaks segmentation / TX checksum offloading
 > >> Confidential:   no
 > >> Severity:       serious
 > >> Priority:       medium
 > >> Responsible:    kern-bug-people
 > >> State:          open
 > >> Class:          sw-bug
 > >> Submitter-Id:   net
 > >> Arrival-Date:   Thu Aug 30 09:25:00 +0000 2018
 > >> Originator:     Rin Okuyama
 > >> Release:        8.99.24
 > >> Organization:
 > > School of Science and Technology, Meiji University
 > >> Environment:
 > > NetBSD rpi3b 8.99.24 NetBSD 8.99.24 (RPI3-64) #24: Thu Aug 30 17:39:24 JST 2018  rin@latipes:/var/build/src/sys/arch/evbarm/compile/RPI3-64 evbarm aarch64
 > >> Description:
 > > If a network interface is added to bridge(4), segmentation or TX
 > > checksum offloading do not work. This is because csum_flags are
 > > cleared in bridge_enqueue():
 > >
 > > https://nxr.netbsd.org/xref/src/sys/net/if_bridge.c#1401
 > >
 > >    1391  void
 > >    1392  bridge_enqueue(struct bridge_softc *sc, struct ifnet *dst_ifp, struct mbuf *m,
 > >    1393      int runfilt)
 > >    1394  {
 > >    ....
 > >    1398          /*
 > >    1399           * Clear any in-bound checksum flags for this packet.
 > >    1400           */
 > >    1401          m->m_pkthdr.csum_flags = 0;
 > >> How-To-Repeat:
 > > Enable segmentation or TX checksum offloading for a network interface,
 > > and add it to bridge(4). Then, data transmission starts to fail.
 > >> Fix:
 > > Not known.
 > >
 >
 > Is the following change correct?
 >
 > 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 3 Sep 2018 08:51:23 -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) {
 > @@ -1644,6 +1639,11 @@ bridge_forward(struct bridge_softc *sc,
 >         if ((sc->sc_if.if_flags & IFF_RUNNING) == 0)
 >                 return;
 >
 > +       /*
 > +        * Clear any in-bound checksum flags for this packet.
 > +        */
 > +       m->m_pkthdr.csum_flags = 0;
 > +
 >         src_if = m_get_rcvif_psref(m, &psref_src);
 >         if (src_if == NULL) {
 >                 /* Interface is being destroyed? */
 >
 
 ether_input can be called via ether_broadcast that is called from
 bridge_forward.
 So I guess clearing csum_flags should be done just before calling bridge_enqueue
 like the following patch?
 
   ozaki-r
 
 
 diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
 index e8c73f8a199..f92675e3c81 100644
 --- a/sys/net/if_bridge.c
 +++ b/sys/net/if_bridge.c
 @@ -1395,11 +1395,6 @@ bridge_enqueue(struct bridge_softc *sc, struct
 ifnet *dst_ifp, struct mbuf *m,
         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,11 @@ bridge_forward(struct bridge_softc *sc, struct mbuf *m)
 
         bridge_release_member(sc, bif, &psref);
 
 +       /*
 +        * Clear any in-bound checksum flags for this packet.
 +        */
 +       m->m_pkthdr.csum_flags = 0;
 +
         ACQUIRE_GLOBAL_LOCKS();
         bridge_enqueue(sc, dst_if, m, 1);
         RELEASE_GLOBAL_LOCKS();
 @@ -1978,6 +1978,12 @@ bridge_broadcast(struct bridge_softc *sc,
 struct ifnet *src_if,
                                 sc->sc_if.if_oerrors++;
                                 goto next;
                         }
 +
 +                       /*
 +                        * Clear any in-bound checksum flags for this packet.
 +                        */
 +                       mc->m_pkthdr.csum_flags = 0;
 +
                         ACQUIRE_GLOBAL_LOCKS();
                         bridge_enqueue(sc, dst_if, mc, 1);