| Message ID | 20171023181857.GA25379@embeddedor.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: xfrm_user: use BUG_ON instead of if condition followed by BUG | expand |
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,
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
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,
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
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
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,
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.
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.
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,
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.
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!
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
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
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,
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
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); }
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(-)