[PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Use BUG_ON instead of if condition followed by BUG.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 net/xfrm/xfrm_user.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 465f3ec..9e8851f 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1152,8 +1152,7 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (r_skb == NULL)
 		return -ENOMEM;
 
-	if (build_spdinfo(r_skb, net, sportid, seq, *flags) < 0)
-		BUG();
+	BUG_ON(build_spdinfo(r_skb, net, sportid, seq, *flags) < 0);
 
 	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
 }
@@ -1210,8 +1209,7 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (r_skb == NULL)
 		return -ENOMEM;
 
-	if (build_sadinfo(r_skb, net, sportid, seq, *flags) < 0)
-		BUG();
+	BUG_ON(build_sadinfo(r_skb, net, sportid, seq, *flags) < 0);
 
 	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
 }
@@ -1958,8 +1956,8 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	c.seq = nlh->nlmsg_seq;
 	c.portid = nlh->nlmsg_pid;
 
-	if (build_aevent(r_skb, x, &c) < 0)
-		BUG();
+	BUG_ON(build_aevent(r_skb, x, &c) < 0);
+
 	err = nlmsg_unicast(net->xfrm.nlsk, r_skb, NETLINK_CB(skb).portid);
 	spin_unlock_bh(&x->lock);
 	xfrm_state_put(x);
@@ -2393,8 +2391,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		return -ENOMEM;
 
 	/* build migrate */
-	if (build_migrate(skb, m, num_migrate, k, sel, encap, dir, type) < 0)
-		BUG();
+	BUG_ON(build_migrate(skb, m, num_migrate, k, sel, encap, dir, type) < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
 }
@@ -2623,8 +2620,7 @@ static int xfrm_aevent_state_notify(struct xfrm_state *x, const struct km_event
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_aevent(skb, x, c) < 0)
-		BUG();
+	BUG_ON(build_aevent(skb, x, c) < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_AEVENTS);
 }
@@ -2836,8 +2832,7 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_acquire(skb, x, xt, xp) < 0)
-		BUG();
+	BUG_ON(build_acquire(skb, x, xt, xp) < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_ACQUIRE);
 }
@@ -2951,8 +2946,7 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, const struct
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_polexpire(skb, xp, dir, c) < 0)
-		BUG();
+	BUG_ON(build_polexpire(skb, xp, dir, c) < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_EXPIRE);
 }
@@ -3112,8 +3106,7 @@ static int xfrm_send_report(struct net *net, u8 proto,
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_report(skb, proto, sel, addr) < 0)
-		BUG();
+	BUG_ON(build_report(skb, proto, sel, addr) < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_REPORT);
 }
@@ -3165,8 +3158,7 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_mapping(skb, x, ipaddr, sport) < 0)
-		BUG();
+	BUG_ON(build_mapping(skb, x, ipaddr, sport) < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MAPPING);
 }
-- 
2.7.4

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Herbert Xu]

On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
> Use BUG_ON instead of if condition followed by BUG.
> 
> This issue was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>

I think this patch is terrible.  Why on earth is Coccinelle even
warning about this?

If anything we should be converting these constructs to not use
BUG.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Quoting Herbert Xu <herbert@gondor.apana.org.au>:

> On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
>> Use BUG_ON instead of if condition followed by BUG.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> I think this patch is terrible.  Why on earth is Coccinelle even
> warning about this?
>
> If anything we should be converting these constructs to not use
> BUG.
>

Yeah, and under this assumption the original code is even worse.

Thanks
--
Gustavo A. R. Silva

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Herbert Xu]

On Mon, Oct 23, 2017 at 10:50:43PM -0500, Gustavo A. R. Silva wrote:
> 
> Quoting Herbert Xu <herbert@gondor.apana.org.au>:
> 
> >On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
> >>Use BUG_ON instead of if condition followed by BUG.
> >>
> >>This issue was detected with the help of Coccinelle.
> >>
> >>Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> >
> >I think this patch is terrible.  Why on earth is Coccinelle even
> >warning about this?
> >
> >If anything we should be converting these constructs to not use
> >BUG.
> >
> 
> Yeah, and under this assumption the original code is even worse.

No your patch makes it worse.  The original construct makes it
clear that the conditional is real code and not just some debugging
aid.

With your patch, real code is now inside BUG_ON.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Quoting Herbert Xu <herbert@gondor.apana.org.au>:

> On Mon, Oct 23, 2017 at 10:50:43PM -0500, Gustavo A. R. Silva wrote:
>>
>> Quoting Herbert Xu <herbert@gondor.apana.org.au>:
>>
>> >On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
>> >>Use BUG_ON instead of if condition followed by BUG.
>> >>
>> >>This issue was detected with the help of Coccinelle.
>> >>
>> >>Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> >
>> >I think this patch is terrible.  Why on earth is Coccinelle even
>> >warning about this?
>> >
>> >If anything we should be converting these constructs to not use
>> >BUG.
>> >
>>
>> Yeah, and under this assumption the original code is even worse.
>
> No your patch makes it worse.  The original construct makes it
> clear that the conditional is real code and not just some debugging
> aid.
>
> With your patch, real code is now inside BUG_ON.
>

What is the reason for BUG_ON to exist then if it makes the code worse  
than an _if_ condition followed by BUG?

Thanks
--
Gustavo A. R. Silva

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Alexei Starovoitov]

On Tue, Oct 24, 2017 at 11:53:20AM +0800, Herbert Xu wrote:
> On Mon, Oct 23, 2017 at 10:50:43PM -0500, Gustavo A. R. Silva wrote:
> > 
> > Quoting Herbert Xu <herbert@gondor.apana.org.au>:
> > 
> > >On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
> > >>Use BUG_ON instead of if condition followed by BUG.
> > >>
> > >>This issue was detected with the help of Coccinelle.
> > >>
> > >>Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> > >
> > >I think this patch is terrible.  Why on earth is Coccinelle even
> > >warning about this?
> > >
> > >If anything we should be converting these constructs to not use
> > >BUG.
> > >
> > 
> > Yeah, and under this assumption the original code is even worse.
> 
> No your patch makes it worse.  The original construct makes it
> clear that the conditional is real code and not just some debugging
> aid.
> 
> With your patch, real code is now inside BUG_ON.

fwiw I had the same argument earlier:
https://lkml.org/lkml/2017/10/9/1139

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Herbert Xu]

On Mon, Oct 23, 2017 at 09:01:46PM -0700, Alexei Starovoitov wrote:
>
> fwiw I had the same argument earlier:
> https://lkml.org/lkml/2017/10/9/1139

Fair point on eliminating a branch.  But I'd prefer something like

	bool cond;

	cond = code_that_does_something();
	BUG_ON(cond);

rather than just

	BUG_ON(code_that_does_something());

But maybe it's just me.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 24 Oct 2017 11:53:20 +0800

> No your patch makes it worse.  The original construct makes it
> clear that the conditional is real code and not just some debugging
> aid.
> 
> With your patch, real code is now inside BUG_ON.

This discussion has happened before.

But I'll explain the conclusion here for your benefit.

BUG_ON() is a statement and everything inside of it will
always execute.

BUG_ON() is always preferred because it allows arch
specific code to pass the conditional result properly
into inline asm and builtins for optimal code generation.

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Herbert Xu]

On Tue, Oct 24, 2017 at 05:48:42PM +0900, David Miller wrote:
> 
> This discussion has happened before.
> 
> But I'll explain the conclusion here for your benefit.
> 
> BUG_ON() is a statement and everything inside of it will
> always execute.
> 
> BUG_ON() is always preferred because it allows arch
> specific code to pass the conditional result properly
> into inline asm and builtins for optimal code generation.

This is a good point.  However, while a little bit more verbose you
can still achieve the same assembly-level result by something like

	int err;

	err = <insert real code here>;
	BUG_ON(err);

Having real code in BUG_ON may pose problems to people reading the
code because some of us tend to ignore code in BUG_ON and similar
macros such as BUILD_BUG_ON.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 25 Oct 2017 12:05:41 +0800

> On Tue, Oct 24, 2017 at 05:48:42PM +0900, David Miller wrote:
>> 
>> This discussion has happened before.
>> 
>> But I'll explain the conclusion here for your benefit.
>> 
>> BUG_ON() is a statement and everything inside of it will
>> always execute.
>> 
>> BUG_ON() is always preferred because it allows arch
>> specific code to pass the conditional result properly
>> into inline asm and builtins for optimal code generation.
> 
> This is a good point.  However, while a little bit more verbose you
> can still achieve the same assembly-level result by something like
> 
> 	int err;
> 
> 	err = <insert real code here>;
> 	BUG_ON(err);
> 
> Having real code in BUG_ON may pose problems to people reading the
> code because some of us tend to ignore code in BUG_ON and similar
> macros such as BUILD_BUG_ON.

I agree that this makes the code easier to read and audit.

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Steffen Klassert]

On Wed, Oct 25, 2017 at 01:22:22PM +0900, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 25 Oct 2017 12:05:41 +0800
> 
> > On Tue, Oct 24, 2017 at 05:48:42PM +0900, David Miller wrote:
> >> 
> >> This discussion has happened before.
> >> 
> >> But I'll explain the conclusion here for your benefit.
> >> 
> >> BUG_ON() is a statement and everything inside of it will
> >> always execute.
> >> 
> >> BUG_ON() is always preferred because it allows arch
> >> specific code to pass the conditional result properly
> >> into inline asm and builtins for optimal code generation.
> > 
> > This is a good point.  However, while a little bit more verbose you
> > can still achieve the same assembly-level result by something like
> > 
> > 	int err;
> > 
> > 	err = <insert real code here>;
> > 	BUG_ON(err);
> > 
> > Having real code in BUG_ON may pose problems to people reading the
> > code because some of us tend to ignore code in BUG_ON and similar
> > macros such as BUILD_BUG_ON.
> 
> I agree that this makes the code easier to read and audit.

It seems that we have an agreement on the above version,
Gustavo can you please update your patches to this?

Thanks!

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Hi all,

Quoting Steffen Klassert <steffen.klassert@secunet.com>:

> On Wed, Oct 25, 2017 at 01:22:22PM +0900, David Miller wrote:
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>> Date: Wed, 25 Oct 2017 12:05:41 +0800
>>
>> > On Tue, Oct 24, 2017 at 05:48:42PM +0900, David Miller wrote:
>> >>
>> >> This discussion has happened before.
>> >>
>> >> But I'll explain the conclusion here for your benefit.
>> >>
>> >> BUG_ON() is a statement and everything inside of it will
>> >> always execute.
>> >>
>> >> BUG_ON() is always preferred because it allows arch
>> >> specific code to pass the conditional result properly
>> >> into inline asm and builtins for optimal code generation.
>> >
>> > This is a good point.  However, while a little bit more verbose you
>> > can still achieve the same assembly-level result by something like
>> >
>> > 	int err;
>> >
>> > 	err = <insert real code here>;
>> > 	BUG_ON(err);
>> >
>> > Having real code in BUG_ON may pose problems to people reading the
>> > code because some of us tend to ignore code in BUG_ON and similar
>> > macros such as BUILD_BUG_ON.
>>
>> I agree that this makes the code easier to read and audit.
>
> It seems that we have an agreement on the above version,
> Gustavo can you please update your patches to this?
>

I can do that. I'll work on a patchset for this.

Thanks
--
Gustavo A. R. Silva

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Quoting "Gustavo A. R. Silva" <garsilva@embeddedor.com>:

> Hi all,
>
> Quoting Steffen Klassert <steffen.klassert@secunet.com>:
>
>> On Wed, Oct 25, 2017 at 01:22:22PM +0900, David Miller wrote:
>>> From: Herbert Xu <herbert@gondor.apana.org.au>
>>> Date: Wed, 25 Oct 2017 12:05:41 +0800
>>>
>>>> On Tue, Oct 24, 2017 at 05:48:42PM +0900, David Miller wrote:
>>>>>
>>>>> This discussion has happened before.
>>>>>
>>>>> But I'll explain the conclusion here for your benefit.
>>>>>
>>>>> BUG_ON() is a statement and everything inside of it will
>>>>> always execute.
>>>>>
>>>>> BUG_ON() is always preferred because it allows arch
>>>>> specific code to pass the conditional result properly
>>>>> into inline asm and builtins for optimal code generation.
>>>>
>>>> This is a good point.  However, while a little bit more verbose you
>>>> can still achieve the same assembly-level result by something like
>>>>
>>>> 	int err;
>>>>
>>>> 	err = <insert real code here>;
>>>> 	BUG_ON(err);
>>>>
>>>> Having real code in BUG_ON may pose problems to people reading the
>>>> code because some of us tend to ignore code in BUG_ON and similar
>>>> macros such as BUILD_BUG_ON.
>>>
>>> I agree that this makes the code easier to read and audit.
>>
>> It seems that we have an agreement on the above version,
>> Gustavo can you please update your patches to this?
>>
>

By the way... this solution applies to the following sort of code:

if (xdr_buf_subsegment(buf, &integ_buf, 0, integ_len))
	BUG();

But what about the original code in this patch:

if (build_spdinfo(r_skb, net, sportid, seq, *flags) < 0)
	BUG();

I don't think we want something like:

bool err;

err = build_spdinfo(r_skb, net, sportid, seq, *flags) < 0 ? true : false;
BUG_ON(err);

Are you willing to accept the original patch in this case?

Thanks
--
Gustavo A. R. Silva

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Herbert Xu]

On Wed, Oct 25, 2017 at 07:38:35PM -0500, Gustavo A. R. Silva wrote:
>
> I don't think we want something like:
> 
> bool err;
> 
> err = build_spdinfo(r_skb, net, sportid, seq, *flags) < 0 ? true : false;
> BUG_ON(err);

How about

	int err;

	err = build_spdinfo(r_skb, net, sportid, seq, *flags);
	BUG_ON(err < 0);

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Hi Herbert,

Quoting Herbert Xu <herbert@gondor.apana.org.au>:

> On Wed, Oct 25, 2017 at 07:38:35PM -0500, Gustavo A. R. Silva wrote:
>>
>> I don't think we want something like:
>>
>> bool err;
>>
>> err = build_spdinfo(r_skb, net, sportid, seq, *flags) < 0 ? true : false;
>> BUG_ON(err);
>
> How about
>
> 	int err;
>
> 	err = build_spdinfo(r_skb, net, sportid, seq, *flags);
> 	BUG_ON(err < 0);
>

I like it.

Thanks
--
Gustavo A. R. Silva

[PATCH v2] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Use BUG_ON instead of if condition followed by BUG.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
 Update the code as suggested by Herbert Xu:

 int err;

 err = foo();
 BUG_ON(err < 0);

 net/xfrm/xfrm_user.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b997f13..fb8c79e 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1147,13 +1147,14 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	u32 *flags = nlmsg_data(nlh);
 	u32 sportid = NETLINK_CB(skb).portid;
 	u32 seq = nlh->nlmsg_seq;
+	int err;
 
 	r_skb = nlmsg_new(xfrm_spdinfo_msgsize(), GFP_ATOMIC);
 	if (r_skb == NULL)
 		return -ENOMEM;
 
-	if (build_spdinfo(r_skb, net, sportid, seq, *flags) < 0)
-		BUG();
+	err = build_spdinfo(r_skb, net, sportid, seq, *flags);
+	BUG_ON(err < 0);
 
 	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
 }
@@ -1205,13 +1206,14 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	u32 *flags = nlmsg_data(nlh);
 	u32 sportid = NETLINK_CB(skb).portid;
 	u32 seq = nlh->nlmsg_seq;
+	int err;
 
 	r_skb = nlmsg_new(xfrm_sadinfo_msgsize(), GFP_ATOMIC);
 	if (r_skb == NULL)
 		return -ENOMEM;
 
-	if (build_sadinfo(r_skb, net, sportid, seq, *flags) < 0)
-		BUG();
+	err = build_sadinfo(r_skb, net, sportid, seq, *flags);
+	BUG_ON(err < 0);
 
 	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
 }
@@ -1958,8 +1960,9 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	c.seq = nlh->nlmsg_seq;
 	c.portid = nlh->nlmsg_pid;
 
-	if (build_aevent(r_skb, x, &c) < 0)
-		BUG();
+	err = build_aevent(r_skb, x, &c);
+	BUG_ON(err < 0);
+
 	err = nlmsg_unicast(net->xfrm.nlsk, r_skb, NETLINK_CB(skb).portid);
 	spin_unlock_bh(&x->lock);
 	xfrm_state_put(x);
@@ -2386,6 +2389,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 {
 	struct net *net = &init_net;
 	struct sk_buff *skb;
+	int err;
 
 	skb = nlmsg_new(xfrm_migrate_msgsize(num_migrate, !!k, !!encap),
 			GFP_ATOMIC);
@@ -2393,8 +2397,8 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		return -ENOMEM;
 
 	/* build migrate */
-	if (build_migrate(skb, m, num_migrate, k, sel, encap, dir, type) < 0)
-		BUG();
+	err = build_migrate(skb, m, num_migrate, k, sel, encap, dir, type);
+	BUG_ON(err < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
 }
@@ -2618,13 +2622,14 @@ static int xfrm_aevent_state_notify(struct xfrm_state *x, const struct km_event
 {
 	struct net *net = xs_net(x);
 	struct sk_buff *skb;
+	int err;
 
 	skb = nlmsg_new(xfrm_aevent_msgsize(x), GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_aevent(skb, x, c) < 0)
-		BUG();
+	err = build_aevent(skb, x, c);
+	BUG_ON(err < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_AEVENTS);
 }
@@ -2830,13 +2835,14 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
 {
 	struct net *net = xs_net(x);
 	struct sk_buff *skb;
+	int err;
 
 	skb = nlmsg_new(xfrm_acquire_msgsize(x, xp), GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_acquire(skb, x, xt, xp) < 0)
-		BUG();
+	err = build_acquire(skb, x, xt, xp);
+	BUG_ON(err < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_ACQUIRE);
 }
@@ -2945,13 +2951,14 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, const struct
 {
 	struct net *net = xp_net(xp);
 	struct sk_buff *skb;
+	int err;
 
 	skb = nlmsg_new(xfrm_polexpire_msgsize(xp), GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_polexpire(skb, xp, dir, c) < 0)
-		BUG();
+	err = build_polexpire(skb, xp, dir, c);
+	BUG_ON(err < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_EXPIRE);
 }
@@ -3105,13 +3112,14 @@ static int xfrm_send_report(struct net *net, u8 proto,
 			    struct xfrm_selector *sel, xfrm_address_t *addr)
 {
 	struct sk_buff *skb;
+	int err;
 
 	skb = nlmsg_new(xfrm_report_msgsize(), GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_report(skb, proto, sel, addr) < 0)
-		BUG();
+	err = build_report(skb, proto, sel, addr);
+	BUG_ON(err < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_REPORT);
 }
@@ -3152,6 +3160,7 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
 {
 	struct net *net = xs_net(x);
 	struct sk_buff *skb;
+	int err;
 
 	if (x->id.proto != IPPROTO_ESP)
 		return -EINVAL;
@@ -3163,8 +3172,8 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_mapping(skb, x, ipaddr, sport) < 0)
-		BUG();
+	err = build_mapping(skb, x, ipaddr, sport);
+	BUG_ON(err < 0);
 
 	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MAPPING);
 }
-- 
2.7.4

Re: [PATCH v2] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Steffen Klassert]

On Thu, Oct 26, 2017 at 06:31:35AM -0500, Gustavo A. R. Silva wrote:
> Use BUG_ON instead of if condition followed by BUG.
> 
> This issue was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>

Applied to ipsec-next, thanks Gustavo!

Re: [PATCH v2] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Quoting Steffen Klassert <steffen.klassert@secunet.com>:

> On Thu, Oct 26, 2017 at 06:31:35AM -0500, Gustavo A. R. Silva wrote:
>> Use BUG_ON instead of if condition followed by BUG.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> Applied to ipsec-next, thanks Gustavo!

Glad to help. :)

Thank you
--
Gustavo A. R. Silva

Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 24 Oct 2017 11:25:08 +0800

> On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
>> Use BUG_ON instead of if condition followed by BUG.
>> 
>> This issue was detected with the help of Coccinelle.
>> 
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> 
> I think this patch is terrible.  Why on earth is Coccinelle even
> warning about this?

BUG_ON(x) generates significantly better code than
"if (cond) BUG();" because it's not possible to emit
the most optimal code sequences like on powerpc that has
conditional traps.

That's why.

I'm applying these transformations all over the networking as I
receive them and I advise that you do so for crypto as well.

Thank.