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

PATCH In review Gustavo A. R. Silva 5 participants 19 messages Oct 23, 2017 – Oct 27, 2017

Subsystems (3)

Networking Network drivers Networking IPsec

lore

Share & tools

lore

Gustavo A. R. Silva

author

2017-10-23 18:18 UTC #0 lore raw

PATCH 1/1 net: xfrm_user: use BUG_ON instead of if condition followed by BUG

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 <[email protected]>

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

net/xfrm/xfrm_user.c +10 ~~-18~~

index 465f3ec..9e8851f 100644
@@ -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);
 }

Signature · 1 line

2.7.4

Herbert Xu

maintainer

2017-10-24 03:25 UTC #1 lore raw

Quoted 5 lines · 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 <[email protected]>

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Gustavo A. R. Silva

author

2017-10-24 03:50 UTC #2 lore raw

Quoting Herbert Xu <[email protected]>:

Quoted 5 lines · Gustavo A. R. Silva

Quoted 4 lines
Use BUG_ON instead of if condition followed by BUG.
This issue was detected with the help of Coccinelle.

Quoted 8 lines

Quoted 1 line
Signed-off-by Gustavo A. R. Silva <[email protected]>
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

Herbert Xu

maintainer

2017-10-24 03:53 UTC #3 lore raw

Quoted 10 lines · Gustavo A. R. Silva

Quoting Herbert Xu <[email protected]>:
Quoted 7 lines · Gustavo A. R. Silva
Quoted 5 lines
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 <[email protected]>

Quoted 8 lines

Quoted 6 lines
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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Gustavo A. R. Silva

author

2017-10-24 03:57 UTC #4 lore raw

Quoting Herbert Xu <[email protected]>:

Quoted 18 lines · Gustavo A. R. Silva

Quoted 17 lines
Quoting Herbert Xu <[email protected]>:
Quoted 13 lines · Gustavo A. R. Silva
Quoted 5 lines
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 <[email protected]>
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.

Quoted 8 lines

Quoted 1 line
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

Alexei Starovoitov

2017-10-24 04:01 UTC #5 lore raw

In reply to Herbert Xu

Quoted 18 lines · Herbert Xu

Quoted 17 lines · Gustavo A. R. Silva
Quoting Herbert Xu <[email protected]>:
Quoted 13 lines · Gustavo A. R. Silva
Quoted 5 lines
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 <[email protected]>
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.

Quoted 7 lines

Quoted 1 line
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

Herbert Xu

maintainer

2017-10-24 04:31 UTC #6 lore raw

Quoted 3 lines · Alexei Starovoitov

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2017-10-24 08:48 UTC #7 lore raw

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Tue, 24 Oct 2017 11:53:20 +0800

Quoted 5 lines

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.

Herbert Xu

maintainer

2017-10-25 04:05 UTC #8 lore raw

Quoted 11 lines · David Miller

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2017-10-25 04:22 UTC #9 lore raw

From: Herbert Xu <[email protected]>
Date: Wed, 25 Oct 2017 12:05:41 +0800

Quoted 16 lines · David Miller

Quoted 11 lines
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

Quoted 8 lines

	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.

Steffen Klassert

maintainer

2017-10-25 07:28 UTC #10 lore raw

Quoted 21 lines · David Miller

From: Herbert Xu <[email protected]>
Date: Wed, 25 Oct 2017 12:05:41 +0800
Quoted 18 lines · David Miller
Quoted 11 lines
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;

Quoted 8 lines

Quoted 6 lines
	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!

Gustavo A. R. Silva

author

2017-10-25 16:43 UTC #11 lore raw

Hi all,

Quoting Steffen Klassert <[email protected]>:

Quoted 25 lines · David Miller

Quoted 24 lines
From: Herbert Xu <[email protected]>
Date: Wed, 25 Oct 2017 12:05:41 +0800
Quoted 21 lines · David Miller
Quoted 11 lines
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);

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
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

Gustavo A. R. Silva

author

2017-10-26 00:38 UTC #12 lore raw

Quoting "Gustavo A. R. Silva" <[email protected]>:

Quoted 39 lines

Hi all,
Quoting Steffen Klassert <[email protected]>:
Quoted 34 lines · David Miller
Quoted 21 lines
From: Herbert Xu <[email protected]>
Date: Wed, 25 Oct 2017 12:05:41 +0800
Quoted 18 lines · David Miller
Quoted 11 lines
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;
Quoted 8 lines
Quoted 6 lines
	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

Herbert Xu

maintainer

2017-10-26 06:36 UTC #13 lore raw

Quoted 7 lines · Gustavo A. R. Silva

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Gustavo A. R. Silva

author

2017-10-26 10:47 UTC #14 lore raw

Hi Herbert,

Quoting Herbert Xu <[email protected]>:

Quoted 9 lines · Gustavo A. R. Silva

Quoted 7 lines
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);

Quoted 7 lines

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

Gustavo A. R. Silva

author

2017-10-26 11:36 UTC #15 lore raw

PATCH v2 1/1 net: xfrm_user: use BUG_ON instead of if condition followed by BUG

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 <[email protected]>

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

net/xfrm/xfrm_user.c +27 ~~-18~~

index b997f13..fb8c79e 100644
@@ -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);
 }

Signature · 1 line

2.7.4

Steffen Klassert

maintainer Applied

2017-10-27 10:46 UTC #16 lore raw

Quoted 5 lines · 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 <[email protected]>

Applied to ipsec-next, thanks Gustavo!

Gustavo A. R. Silva

author

2017-10-27 20:56 UTC #17 lore raw

Quoting Steffen Klassert <[email protected]>:

Quoted 8 lines · Gustavo A. R. Silva

Quoted 5 lines
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 <[email protected]>
Applied to ipsec-next, thanks Gustavo!

Glad to help. :)

Thank you
--
Gustavo A. R. Silva

David Miller

maintainer

2017-10-24 08:47 UTC #18 lore raw

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Tue, 24 Oct 2017 11:25:08 +0800

Quoted 9 lines · Gustavo A. R. Silva

Quoted 5 lines
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 <[email protected]>
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.