[PATCH net 0/2] openvswitch: remove a couple of BUG_ON()

[PATCH net 0/2] openvswitch: remove a couple of BUG_ON()

[Paolo Abeni]

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

-- 
2.21.0

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

[Paolo Abeni]

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

Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/openvswitch/datapath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index d8c364d637b1..e94f675794f1 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -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;
 }
 
-- 
2.21.0

[PATCH net 2/2] openvswitch: remove another BUG_ON()

[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 <pabeni@redhat.com>
---
 net/openvswitch/datapath.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index e94f675794f1..50656e807c8c 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -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:
-- 
2.21.0

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

[Sergei Shtylyov]

Hello!

On 26.11.2019 15:10, Paolo Abeni wrote:

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

    Warning, maybe?

> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>   net/openvswitch/datapath.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index d8c364d637b1..e94f675794f1 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -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

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

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 26 Nov 2019 13:10:29 +0100

> All callers already deal with errors correctly, dump a warn instead.
> 
> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/openvswitch/datapath.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index d8c364d637b1..e94f675794f1 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -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.

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

[Paolo Abeni]

On Thu, 2019-11-28 at 13:17 -0800, David Miller wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Tue, 26 Nov 2019 13:10:29 +0100
> 
> > All callers already deal with errors correctly, dump a warn instead.
> > 
> > Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  net/openvswitch/datapath.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> > index d8c364d637b1..e94f675794f1 100644
> > --- a/net/openvswitch/datapath.c
> > +++ b/net/openvswitch/datapath.c
> > @@ -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