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

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

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);