| Message ID | DU0PR10MB5244A38E7712028763169A93EA8F9@DU0PR10MB5244.EURPRD10.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: openvswitch: fix race on port output | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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() > 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? > 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://www.datenschutz.schwarz>. You gotta get rid of this to work upstream.
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>.
On Mon, 3 Apr 2023 11:18:46 +0000 Felix Hüttner wrote: > 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 skb_tx_hash() may get called a lot, we shouldn't slow it down on production systems just to catch buggy drivers, IMO.
On Mon, 3 Apr 2023 20:50:00 +0000 Jakub Kicinski wrote: > On Mon, 3 Apr 2023 11:18:46 +0000 Felix Hüttner wrote: > > 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 > > skb_tx_hash() may get called a lot, we shouldn't slow it down on > production systems just to catch buggy drivers, IMO. Thanks for the clarification. Will then use DEBUG_NET_WARN_ON_ONCE in v2 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>.
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)); 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)) { u16 mru = OVS_CB(skb)->mru; u32 cutlen = OVS_CB(skb)->cutlen;