Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Tim Hansen]

Mistakenly sent the patch previously with a missing semicolon.
Apologies.

Fix BUG() calls to use BUG_ON(conditional) macros.

This was found using make coccicheck M=net/core on linux next
tag next-20170929

Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
---
 net/core/skbuff.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d98c2e3ce2bf..34ce4c1a0f3c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
 	/* Set the tail pointer and length */
 	skb_put(n, skb->len);
 
-	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
-		BUG();
+	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
 
 	copy_skb_header(n, skb);
 	return n;
@@ -1449,8 +1448,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 
 	BUG_ON(nhead < 0);
 
-	if (skb_shared(skb))
-		BUG();
+	BUG_ON(skb_shared(skb));
 
 	size = SKB_DATA_ALIGN(size);
 
@@ -1595,9 +1593,8 @@ struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
 		head_copy_off = newheadroom - head_copy_len;
 
 	/* Copy the linear header and data. */
-	if (skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
-			  skb->len + head_copy_len))
-		BUG();
+	BUG_ON(skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
+			     skb->len + head_copy_len));
 
 	copy_skb_header(n, skb);
 
@@ -1878,8 +1875,8 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta)
 			return NULL;
 	}
 
-	if (skb_copy_bits(skb, skb_headlen(skb), skb_tail_pointer(skb), delta))
-		BUG();
+	BUG_ON(skb_copy_bits(skb, skb_headlen(skb),
+			     skb_tail_pointer(skb), delta));
 
 	/* Optimization: no fragments, no reasons to preestimate
 	 * size of pulled pages. Superb.
-- 
2.14.2

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[David Miller]

From: Tim Hansen <devtimhansen@gmail.com>
Date: Sun, 8 Oct 2017 16:03:38 -0400

> Mistakenly sent the patch previously with a missing semicolon.
> Apologies.
> 
> Fix BUG() calls to use BUG_ON(conditional) macros.
> 
> This was found using make coccicheck M=net/core on linux next
> tag next-20170929
> 
> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>

You're going to have to submit this new version again, the way you
replied to the original patch caused the fixed version to not get
queued up in patchwork properly.

[PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Tim Hansen]

Fix BUG() calls to use BUG_ON(conditional) macros.

This was found using make coccicheck M=net/core on linux next
tag next-2017092

Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
---
 net/core/skbuff.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d98c2e3ce2bf..34ce4c1a0f3c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
 	/* Set the tail pointer and length */
 	skb_put(n, skb->len);
 
-	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
-		BUG();
+	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
 
 	copy_skb_header(n, skb);
 	return n;
@@ -1449,8 +1448,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 
 	BUG_ON(nhead < 0);
 
-	if (skb_shared(skb))
-		BUG();
+	BUG_ON(skb_shared(skb));
 
 	size = SKB_DATA_ALIGN(size);
 
@@ -1595,9 +1593,8 @@ struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
 		head_copy_off = newheadroom - head_copy_len;
 
 	/* Copy the linear header and data. */
-	if (skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
-			  skb->len + head_copy_len))
-		BUG();
+	BUG_ON(skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
+			     skb->len + head_copy_len));
 
 	copy_skb_header(n, skb);
 
@@ -1878,8 +1875,8 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta)
 			return NULL;
 	}
 
-	if (skb_copy_bits(skb, skb_headlen(skb), skb_tail_pointer(skb), delta))
-		BUG();
+	BUG_ON(skb_copy_bits(skb, skb_headlen(skb),
+			     skb_tail_pointer(skb), delta));
 
 	/* Optimization: no fragments, no reasons to preestimate
 	 * size of pulled pages. Superb.
-- 
2.14.2

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Alexei Starovoitov]

On Mon, Oct 09, 2017 at 11:37:59AM -0400, Tim Hansen wrote:
> Fix BUG() calls to use BUG_ON(conditional) macros.
> 
> This was found using make coccicheck M=net/core on linux next
> tag next-2017092
> 
> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
> ---
>  net/core/skbuff.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d98c2e3ce2bf..34ce4c1a0f3c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
>  	/* Set the tail pointer and length */
>  	skb_put(n, skb->len);
>  
> -	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
> -		BUG();
> +	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));

I'm concerned with this change.
1. Calling non-trivial bit of code inside the macro is a poor coding style (imo)
2. BUG_ON != BUG. Some archs like mips and ppc have HAVE_ARCH_BUG_ON and implementation
of BUG and BUG_ON look quite different.

I'd prefer to keep the code as-is.

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Levin, Alexander (Sasha Levin)]

On Mon, Oct 09, 2017 at 10:15:42AM -0700, Alexei Starovoitov wrote:
>On Mon, Oct 09, 2017 at 11:37:59AM -0400, Tim Hansen wrote:
>> Fix BUG() calls to use BUG_ON(conditional) macros.
>>
>> This was found using make coccicheck M=net/core on linux next
>> tag next-2017092
>>
>> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
>> ---
>>  net/core/skbuff.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index d98c2e3ce2bf..34ce4c1a0f3c 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
>>  	/* Set the tail pointer and length */
>>  	skb_put(n, skb->len);
>>
>> -	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
>> -		BUG();
>> +	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
>
>I'm concerned with this change.
>1. Calling non-trivial bit of code inside the macro is a poor coding style (imo)
>2. BUG_ON != BUG. Some archs like mips and ppc have HAVE_ARCH_BUG_ON and implementation
>of BUG and BUG_ON look quite different.

For these archs, wouldn't it then be more efficient to use BUG_ON rather than BUG()?

-- 

Thanks,
Sasha

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Alexei Starovoitov]

On Mon, Oct 09, 2017 at 08:26:34PM +0000, Levin, Alexander (Sasha Levin) wrote:
> On Mon, Oct 09, 2017 at 10:15:42AM -0700, Alexei Starovoitov wrote:
> >On Mon, Oct 09, 2017 at 11:37:59AM -0400, Tim Hansen wrote:
> >> Fix BUG() calls to use BUG_ON(conditional) macros.
> >>
> >> This was found using make coccicheck M=net/core on linux next
> >> tag next-2017092
> >>
> >> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
> >> ---
> >>  net/core/skbuff.c | 15 ++++++---------
> >>  1 file changed, 6 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index d98c2e3ce2bf..34ce4c1a0f3c 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
> >>  	/* Set the tail pointer and length */
> >>  	skb_put(n, skb->len);
> >>
> >> -	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
> >> -		BUG();
> >> +	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
> >
> >I'm concerned with this change.
> >1. Calling non-trivial bit of code inside the macro is a poor coding style (imo)
> >2. BUG_ON != BUG. Some archs like mips and ppc have HAVE_ARCH_BUG_ON and implementation
> >of BUG and BUG_ON look quite different.
> 
> For these archs, wouldn't it then be more efficient to use BUG_ON rather than BUG()?

why more efficient? any data to prove that?
I'm pointing that the change is not equivalent and
this code has been around forever (pre-git days), so I see
no reason to risk changing it.

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Levin, Alexander (Sasha Levin)]

On Mon, Oct 09, 2017 at 04:06:20PM -0700, Alexei Starovoitov wrote:
>On Mon, Oct 09, 2017 at 08:26:34PM +0000, Levin, Alexander (Sasha Levin) wrote:
>> On Mon, Oct 09, 2017 at 10:15:42AM -0700, Alexei Starovoitov wrote:
>> >On Mon, Oct 09, 2017 at 11:37:59AM -0400, Tim Hansen wrote:
>> >> Fix BUG() calls to use BUG_ON(conditional) macros.
>> >>
>> >> This was found using make coccicheck M=net/core on linux next
>> >> tag next-2017092
>> >>
>> >> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
>> >> ---
>> >>  net/core/skbuff.c | 15 ++++++---------
>> >>  1 file changed, 6 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> >> index d98c2e3ce2bf..34ce4c1a0f3c 100644
>> >> --- a/net/core/skbuff.c
>> >> +++ b/net/core/skbuff.c
>> >> @@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
>> >>  	/* Set the tail pointer and length */
>> >>  	skb_put(n, skb->len);
>> >>
>> >> -	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
>> >> -		BUG();
>> >> +	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
>> >
>> >I'm concerned with this change.
>> >1. Calling non-trivial bit of code inside the macro is a poor coding style (imo)
>> >2. BUG_ON != BUG. Some archs like mips and ppc have HAVE_ARCH_BUG_ON and implementation
>> >of BUG and BUG_ON look quite different.
>>
>> For these archs, wouldn't it then be more efficient to use BUG_ON rather than BUG()?
>
>why more efficient? any data to prove that?

Just guessing.

Either way, is there a particular reason for not using BUG_ON() here
besides that it's implementation is "quite different"?

>I'm pointing that the change is not equivalent and
>this code has been around forever (pre-git days), so I see
>no reason to risk changing it.

Do you know that BUG_ON() is broken on any archs?

If not, "this code has been around forever" is really not an excuse to
not touch code.

If BUG_ON() behavior is broken somewhere, then it needs to get fixed.

-- 

Thanks,
Sasha

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[David Miller]

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Mon, 9 Oct 2017 16:06:20 -0700

>> For these archs, wouldn't it then be more efficient to use BUG_ON
>> rather than BUG()?
> 
> why more efficient? any data to prove that?

It can completely eliminate a branch.

For example on powerpc if you use BUG() then the code generated is:

	test condition
	branch_not_true	1f
	unconditional_trap
1:

Whereas with BUG_ON() it's just:

	test condition
	trap_if_true

Which is a lot better even when the branches in the first case are
well predicted.

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Alexei Starovoitov]

On Mon, Oct 09, 2017 at 11:15:40PM +0000, Levin, Alexander (Sasha Levin) wrote:
> On Mon, Oct 09, 2017 at 04:06:20PM -0700, Alexei Starovoitov wrote:
> >On Mon, Oct 09, 2017 at 08:26:34PM +0000, Levin, Alexander (Sasha Levin) wrote:
> >> On Mon, Oct 09, 2017 at 10:15:42AM -0700, Alexei Starovoitov wrote:
> >> >On Mon, Oct 09, 2017 at 11:37:59AM -0400, Tim Hansen wrote:
> >> >> Fix BUG() calls to use BUG_ON(conditional) macros.
> >> >>
> >> >> This was found using make coccicheck M=net/core on linux next
> >> >> tag next-2017092
> >> >>
> >> >> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
> >> >> ---
> >> >>  net/core/skbuff.c | 15 ++++++---------
> >> >>  1 file changed, 6 insertions(+), 9 deletions(-)
> >> >>
> >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> >> index d98c2e3ce2bf..34ce4c1a0f3c 100644
> >> >> --- a/net/core/skbuff.c
> >> >> +++ b/net/core/skbuff.c
> >> >> @@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
> >> >>  	/* Set the tail pointer and length */
> >> >>  	skb_put(n, skb->len);
> >> >>
> >> >> -	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
> >> >> -		BUG();
> >> >> +	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
> >> >
> >> >I'm concerned with this change.
> >> >1. Calling non-trivial bit of code inside the macro is a poor coding style (imo)
> >> >2. BUG_ON != BUG. Some archs like mips and ppc have HAVE_ARCH_BUG_ON and implementation
> >> >of BUG and BUG_ON look quite different.
> >>
> >> For these archs, wouldn't it then be more efficient to use BUG_ON rather than BUG()?
> >
> >why more efficient? any data to prove that?
> 
> Just guessing.
> 
> Either way, is there a particular reason for not using BUG_ON() here
> besides that it's implementation is "quite different"?
> 
> >I'm pointing that the change is not equivalent and
> >this code has been around forever (pre-git days), so I see
> >no reason to risk changing it.
> 
> Do you know that BUG_ON() is broken on any archs?
> 
> If not, "this code has been around forever" is really not an excuse to
> not touch code.
> 
> If BUG_ON() behavior is broken somewhere, then it needs to get fixed.

no idea whether it's broken. My main objection is #1.
imo it's a very poor coding style to put functions with
side-effects into macros. Especially debug/bug/warn-like.
For example llvm has DEBUG() macro and everything inside
will disappear depending on compilation flags.
I wouldn't be surprised if somebody for the name
of security (to avoid crash on BUG_ON) will replace
BUG/BUG_ON with some other implementation or nop
and will have real bugs, since skb_copy_bits() is somehow
not called or called in different context.

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[Levin, Alexander (Sasha Levin)]

On Mon, Oct 09, 2017 at 04:23:57PM -0700, Alexei Starovoitov wrote:
>On Mon, Oct 09, 2017 at 11:15:40PM +0000, Levin, Alexander (Sasha Levin) wrote:
>> On Mon, Oct 09, 2017 at 04:06:20PM -0700, Alexei Starovoitov wrote:
>> >On Mon, Oct 09, 2017 at 08:26:34PM +0000, Levin, Alexander (Sasha Levin) wrote:
>> >> On Mon, Oct 09, 2017 at 10:15:42AM -0700, Alexei Starovoitov wrote:
>> >> >On Mon, Oct 09, 2017 at 11:37:59AM -0400, Tim Hansen wrote:
>> >> >> Fix BUG() calls to use BUG_ON(conditional) macros.
>> >> >>
>> >> >> This was found using make coccicheck M=net/core on linux next
>> >> >> tag next-2017092
>> >> >>
>> >> >> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
>> >> >> ---
>> >> >>  net/core/skbuff.c | 15 ++++++---------
>> >> >>  1 file changed, 6 insertions(+), 9 deletions(-)
>> >> >>
>> >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> >> >> index d98c2e3ce2bf..34ce4c1a0f3c 100644
>> >> >> --- a/net/core/skbuff.c
>> >> >> +++ b/net/core/skbuff.c
>> >> >> @@ -1350,8 +1350,7 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
>> >> >>  	/* Set the tail pointer and length */
>> >> >>  	skb_put(n, skb->len);
>> >> >>
>> >> >> -	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
>> >> >> -		BUG();
>> >> >> +	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
>> >> >
>> >> >I'm concerned with this change.
>> >> >1. Calling non-trivial bit of code inside the macro is a poor coding style (imo)
>> >> >2. BUG_ON != BUG. Some archs like mips and ppc have HAVE_ARCH_BUG_ON and implementation
>> >> >of BUG and BUG_ON look quite different.
>> >>
>> >> For these archs, wouldn't it then be more efficient to use BUG_ON rather than BUG()?
>> >
>> >why more efficient? any data to prove that?
>>
>> Just guessing.
>>
>> Either way, is there a particular reason for not using BUG_ON() here
>> besides that it's implementation is "quite different"?
>>
>> >I'm pointing that the change is not equivalent and
>> >this code has been around forever (pre-git days), so I see
>> >no reason to risk changing it.
>>
>> Do you know that BUG_ON() is broken on any archs?
>>
>> If not, "this code has been around forever" is really not an excuse to
>> not touch code.
>>
>> If BUG_ON() behavior is broken somewhere, then it needs to get fixed.
>
>no idea whether it's broken. My main objection is #1.
>imo it's a very poor coding style to put functions with
>side-effects into macros. Especially debug/bug/warn-like.

This, however, seems to be an accepted coding style in the net/
subsys. Look a few lines lower in the very same file to find:

	BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));

Side effects ahoy ;)

>For example llvm has DEBUG() macro and everything inside
>will disappear depending on compilation flags.
>I wouldn't be surprised if somebody for the name
>of security (to avoid crash on BUG_ON) will replace
>BUG/BUG_ON with some other implementation or nop
>and will have real bugs, since skb_copy_bits() is somehow
>not called or called in different context.

This was already discussed, with the conclusion that BUG() can never
be disabled, since, as you described, it'll lead to very catastrophic
results. See i.e.:

	commit b06dd879f5db33c1d7f5ab516ea671627f99c0c9
	Author: Josh Triplett <josh@joshtriplett.org>
	Date:   Mon Apr 7 15:39:14 2014 -0700

    		x86: always define BUG() and HAVE_ARCH_BUG, even with !CONFIG_BUG


Anyway, as you said, this boils down to coding style nitpicking. I
guess that my only comment here would be that it shpid go one way or
the other and we document the decision somewhere (either per subsys,
or for the entire tree).

-- 

Thanks,
Sasha

Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.

[David Miller]

From: Tim Hansen <devtimhansen@gmail.com>
Date: Mon, 9 Oct 2017 11:37:59 -0400

> Fix BUG() calls to use BUG_ON(conditional) macros.
> 
> This was found using make coccicheck M=net/core on linux next
> tag next-2017092
> 
> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>

Althrough there were objections raised, none of them technically
stand up, and this does improve code generation for some
architectures, so I have applied this.

Thanks!