Skip to contentSkip to conversation

openvswitch: remove a couple of BUG_ON()

PATCH Superseded Paolo Abeni 3 participants 6 messages Nov 26, 2019 – Nov 29, 2019

Subsystems (3)
Networking Network drivers Openvswitch

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

cover openvswitch: remove a couple of BUG_ON()

The openvswitch kernel datapath includes some BUG_ON() statements to check for exceptional/unexpected failures. These patches drop a couple of them, where we can do that without introducing other side effects.

Paolo Abeni (2):
openvswitch: drop unneeded BUG_ON() in ovs_flow_cmd_build_info()
openvswitch: remove another BUG_ON()

 net/openvswitch/datapath.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Signature · 1 line
2.21.0
author maintainer
#1 lore raw

PATCH 1/2 openvswitch: drop unneeded BUG_ON() in ovs_flow_cmd_build_info()

All callers already deal with errors correctly, dump a warn instead.

Fixes ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Signed-off-by Paolo Abeni <[email protected]>
---
 net/openvswitch/datapath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/openvswitch/datapath.c +1 -1
index d8c364d637b1..e94f675794f1 100644
@@ -882,7 +882,7 @@ static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
 	retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
 					info->snd_portid, info->snd_seq, 0,
 					cmd, ufid_flags);
-	BUG_ON(retval < 0);
+	WARN_ON_ONCE(retval < 0);
 	return skb;
 }
Signature · 1 line
2.21.0
#2 lore raw

Re: [PATCH net 1/2] openvswitch: drop unneeded BUG_ON() in ovs_flow_cmd_build_info()

Hello!

Quoted 1 line · Paolo Abeni

All callers already deal with errors correctly, dump a warn instead.

Warning, maybe?

Quoted 11 lines
Fixes ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Signed-off-by Paolo Abeni <[email protected]>
---
  net/openvswitch/datapath.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
net/openvswitch/datapath.c +0 -0
index d8c364d637b1..e94f675794f1 100644

net/openvswitch/datapath.c static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow, · line 888

Quoted 8 lines
net/openvswitch/datapath.c +1 -1
@@ -882,7 +882,7 @@ static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
  	retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
  					info->snd_portid, info->snd_seq, 0,
  					cmd, ufid_flags);
-	BUG_ON(retval < 0);
+	WARN_ON_ONCE(retval < 0);
  	return skb;
  }

MBR, Sergei

maintainer
#3 lore raw

Re: [PATCH net 1/2] openvswitch: drop unneeded BUG_ON() in ovs_flow_cmd_build_info()

In reply to Paolo Abeni

From: Paolo Abeni <[email protected]>
Date: Tue, 26 Nov 2019 13:10:29 +0100

Quoted 13 lines

All callers already deal with errors correctly, dump a warn instead.

Fixes ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Signed-off-by Paolo Abeni <[email protected]>
---
 net/openvswitch/datapath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/openvswitch/datapath.c +0 -0
index d8c364d637b1..e94f675794f1 100644

net/openvswitch/datapath.c static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow, · line 887

Quoted 7 lines
net/openvswitch/datapath.c +1 -1
@@ -882,7 +882,7 @@ static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
 	retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
 					info->snd_portid, info->snd_seq, 0,
 					cmd, ufid_flags);
-	BUG_ON(retval < 0);
+	WARN_ON_ONCE(retval < 0);
 	return skb;
 }

I don't think this is right. We should propagate the error by freeing the skb and returning a proper error pointer based upon retval.

author maintainer
#4 lore raw

Re: [PATCH net 1/2] openvswitch: drop unneeded BUG_ON() in ovs_flow_cmd_build_info()

Quoted 11 lines · David Miller

From: Paolo Abeni <[email protected]>
Date: Tue, 26 Nov 2019 13:10:29 +0100

Quoted 8 lines

All callers already deal with errors correctly, dump a warn instead.

Fixes ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Signed-off-by Paolo Abeni <[email protected]>
---
 net/openvswitch/datapath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

net/openvswitch/datapath.c static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow, · line 887

Quoted 12 lines
net/openvswitch/datapath.c +1 -1
index d8c364d637b1..e94f675794f1 100644
@@ -882,7 +882,7 @@ static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
      retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
                                      info->snd_portid, info->snd_seq, 0,
                                      cmd, ufid_flags);
-     BUG_ON(retval < 0);
+     WARN_ON_ONCE(retval < 0);
      return skb;
 }

I don't think this is right. We should propagate the error by freeing the skb and returning a proper error pointer based upon retval.

Indeed you are right. Thank you for catching this.

Never cook patches when coffee is too low :/

Will send a v2

Thank you!

Paolo

author maintainer
#5 lore raw

PATCH 2/2 openvswitch: remove another BUG_ON()

In reply to Paolo Abeni

If we can't build the flow del notification, we can simply delete the flow, no need to crash the kernel. Still keep a WARN_ON to preserve debuggability.

Note: the BUG_ON() predates the Fixes tag, but this change can be applied only after the mentioned commit.

Fixes aed067783e50 ("openvswitch: Minimize ovs_flow_cmd_del critical section.")
Signed-off-by Paolo Abeni <[email protected]>
---
 net/openvswitch/datapath.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/openvswitch/datapath.c +3 -1
index e94f675794f1..50656e807c8c 100644
@@ -1346,7 +1346,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
 						     OVS_FLOW_CMD_DEL,
 						     ufid_flags);
 			rcu_read_unlock();
-			BUG_ON(err < 0);
+			if (WARN_ON_ONCE(err < 0))
+				goto out_free;
 
 			ovs_notify(&dp_flow_genl_family, reply, info);
 		} else {

@@ -1354,6 +1355,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
 		}
 	}
 
+out_free:
 	ovs_flow_free(flow, true);
 	return 0;
 unlock:
Signature · 1 line
2.21.0