netdev - RE: [PATCH] net: openvswitch: fix race on port output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Apr 2023 11:18:46 +0000
From:   Felix Hüttner <felix.huettner@...l.schwarz>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Luca Czesla <Luca.Czesla@...l.schwarz>
Subject: RE: [PATCH] net: openvswitch: fix race on port output

Thanks for the review

On Sat, 1 Apr 2023 6:25:00 +0000 Jakub Kicinski wrote:
> On Fri, 31 Mar 2023 06:25:13 +0000 Felix Hüttner wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 253584777101..6628323b7bea 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3199,6 +3199,7 @@ static u16 skb_tx_hash(const struct net_device *dev,
> >         }
> >
> >         if (skb_rx_queue_recorded(skb)) {
> > +               BUG_ON(unlikely(qcount == 0));
>
> DEBUG_NET_WARN_ON()
>

However if this condition triggers we will be permanently stuck in the loop below.
>From my understading this also means that future calls to `synchronize_net` will never finish (as the packet never finishes processing).
So the user will quite probably need to restart his system.
I find DEBUG_NET_WARN_ON_ONCE to offer too little visiblity as CONFIG_DEBUG_NET is not necessarily enabled per default.
I as the user would see it as helpful to have this information available without additional config flags.
I would propose to use WARN_ON_ONCE

> >                 hash = skb_get_rx_queue(skb);
> >                 if (hash >= qoffset)
> >                         hash -= qoffset;
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index ca3ebfdb3023..33b317e5f9a5 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -913,7 +913,7 @@ static void do_output(struct datapath *dp, struct sk_buff *skb, int
> out_port,
> >  {
> >         struct vport *vport = ovs_vport_rcu(dp, out_port);
> >
> > -       if (likely(vport)) {
> > +       if (likely(vport && vport->dev->reg_state == NETREG_REGISTERED)) {
>
> Without looking too closely netif_carrier_ok() seems like a more
> appropriate check for liveness on the datapath?

Yes, will use that in v2

> >                 u16 mru = OVS_CB(skb)->mru;
> >                 u32 cutlen = OVS_CB(skb)->cutlen;
> >
> > --
> > 2.40.0
> >
> > Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung
> durch den vorgesehenen Empfänger bestimmt. Sollten Sie nicht der vorgesehene Empfänger
> sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail.
> Hinweise zum Datenschutz finden Sie
> hier<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.datenschutz.sch
> warz%2F&data=05%7C01%7C%7Cbc601e5604854cc671e208db32691a22%7Cd04f47175a6e4b98b3f96918e0385
> f4c%7C0%7C0%7C638159199209626766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OiRwLDMENMut92J%2Fl0Hs6n8sTWFQO1kc
> Dy7mN%2B4AX8Q%3D&reserved=0>.
>
> You gotta get rid of this to work upstream.

working on it
Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung durch den vorgesehenen Empfänger bestimmt. Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail. Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.