The if condition can be removed if we use BUG_ON directly.
The issule is detected with the help of Coccinelle.
Signed-off-by: zhong jiang <[email protected]>
---
net/ipv4/tcp_input.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 62508a2..893bde3 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4934,8 +4934,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
BUG_ON(offset < 0);
if (size > 0) {
size = min(copy, size);
- if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
- BUG();
+ BUG(skb_copy_bits(skb, offset,
+ skb_put(nskb, size), size));
TCP_SKB_CB(nskb)->end_seq += size;
copy -= size;
start += size;
@@ -5327,8 +5327,8 @@ static void tcp_urg(struct sock *sk, struct sk_buff *skb, const struct tcphdr *t
/* Is the urgent pointer pointing into this packet? */
if (ptr < skb->len) {
u8 tmp;
- if (skb_copy_bits(skb, ptr, &tmp, 1))
- BUG();
+
+ BUG(skb_copy_bits(skb, ptr, &tmp, 1));
tp->urg_data = TCP_URG_VALID | tmp;
if (!sock_flag(sk, SOCK_DEAD))
sk->sk_data_ready(sk);
--
1.7.12.4
On Mon, 10 Sep 2018 22:38:02 +0800, zhong jiang wrote:
> + BUG(skb_copy_bits(skb, ptr, &tmp, 1));
I doubt this builds.
Jiri
Hi zhong,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/zhong-jiang/net-ipv4-Use-BUG_ON-directly-instead-of-a-if-condition-followed-by-BUG/20180911-034152
config: x86_64-randconfig-x009-201836 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
net//ipv4/tcp_input.c: In function 'tcp_collapse':
>> net//ipv4/tcp_input.c:4925:35: error: macro "BUG" passed 1 arguments, but takes just 0
skb_put(nskb, size), size));
^
>> net//ipv4/tcp_input.c:4924:5: error: 'BUG' undeclared (first use in this function)
BUG(skb_copy_bits(skb, offset,
^~~
net//ipv4/tcp_input.c:4924:5: note: each undeclared identifier is reported only once for each function it appears in
net//ipv4/tcp_input.c: In function 'tcp_urg':
net//ipv4/tcp_input.c:5318:40: error: macro "BUG" passed 1 arguments, but takes just 0
BUG(skb_copy_bits(skb, ptr, &tmp, 1));
^
net//ipv4/tcp_input.c:5318:4: error: 'BUG' undeclared (first use in this function)
BUG(skb_copy_bits(skb, ptr, &tmp, 1));
^~~
vim +/BUG +4925 net//ipv4/tcp_input.c
4838
4839 /* Collapse contiguous sequence of skbs head..tail with
4840 * sequence numbers start..end.
4841 *
4842 * If tail is NULL, this means until the end of the queue.
4843 *
4844 * Segments with FIN/SYN are not collapsed (only because this
4845 * simplifies code)
4846 */
4847 static void
4848 tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
4849 struct sk_buff *head, struct sk_buff *tail, u32 start, u32 end)
4850 {
4851 struct sk_buff *skb = head, *n;
4852 struct sk_buff_head tmp;
4853 bool end_of_skbs;
4854
4855 /* First, check that queue is collapsible and find
4856 * the point where collapsing can be useful.
4857 */
4858 restart:
4859 for (end_of_skbs = true; skb != NULL && skb != tail; skb = n) {
4860 n = tcp_skb_next(skb, list);
4861
4862 /* No new bits? It is possible on ofo queue. */
4863 if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
4864 skb = tcp_collapse_one(sk, skb, list, root);
4865 if (!skb)
4866 break;
4867 goto restart;
4868 }
4869
4870 /* The first skb to collapse is:
4871 * - not SYN/FIN and
4872 * - bloated or contains data before "start" or
4873 * overlaps to the next one.
4874 */
4875 if (!(TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) &&
4876 (tcp_win_from_space(sk, skb->truesize) > skb->len ||
4877 before(TCP_SKB_CB(skb)->seq, start))) {
4878 end_of_skbs = false;
4879 break;
4880 }
4881
4882 if (n && n != tail &&
4883 TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(n)->seq) {
4884 end_of_skbs = false;
4885 break;
4886 }
4887
4888 /* Decided to skip this, advance start seq. */
4889 start = TCP_SKB_CB(skb)->end_seq;
4890 }
4891 if (end_of_skbs ||
4892 (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)))
4893 return;
4894
4895 __skb_queue_head_init(&tmp);
4896
4897 while (before(start, end)) {
4898 int copy = min_t(int, SKB_MAX_ORDER(0, 0), end - start);
4899 struct sk_buff *nskb;
4900
4901 nskb = alloc_skb(copy, GFP_ATOMIC);
4902 if (!nskb)
4903 break;
4904
4905 memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
4906 #ifdef CONFIG_TLS_DEVICE
4907 nskb->decrypted = skb->decrypted;
4908 #endif
4909 TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;
4910 if (list)
4911 __skb_queue_before(list, skb, nskb);
4912 else
4913 __skb_queue_tail(&tmp, nskb); /* defer rbtree insertion */
4914 skb_set_owner_r(nskb, sk);
4915
4916 /* Copy data, releasing collapsed skbs. */
4917 while (copy > 0) {
4918 int offset = start - TCP_SKB_CB(skb)->seq;
4919 int size = TCP_SKB_CB(skb)->end_seq - start;
4920
4921 BUG_ON(offset < 0);
4922 if (size > 0) {
4923 size = min(copy, size);
> 4924 BUG(skb_copy_bits(skb, offset,
> 4925 skb_put(nskb, size), size));
4926 TCP_SKB_CB(nskb)->end_seq += size;
4927 copy -= size;
4928 start += size;
4929 }
4930 if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
4931 skb = tcp_collapse_one(sk, skb, list, root);
4932 if (!skb ||
4933 skb == tail ||
4934 (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)))
4935 goto end;
4936 #ifdef CONFIG_TLS_DEVICE
4937 if (skb->decrypted != nskb->decrypted)
4938 goto end;
4939 #endif
4940 }
4941 }
4942 }
4943 end:
4944 skb_queue_walk_safe(&tmp, skb, n)
4945 tcp_rbtree_insert(root, skb);
4946 }
4947
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi zhong,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/zhong-jiang/net-ipv4-Use-BUG_ON-directly-instead-of-a-if-condition-followed-by-BUG/20180911-034152
config: mips-rt305x_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All errors (new ones prefixed by >>):
net/ipv4/tcp_input.c: In function 'tcp_collapse':
>> net/ipv4/tcp_input.c:4924:5: error: too many arguments to function 'BUG'
BUG(skb_copy_bits(skb, offset,
^~~
In file included from include/linux/bug.h:5:0,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from net/ipv4/tcp_input.c:67:
arch/mips/include/asm/bug.h:12:31: note: declared here
static inline void __noreturn BUG(void)
^~~
net/ipv4/tcp_input.c: In function 'tcp_urg':
net/ipv4/tcp_input.c:5318:4: error: too many arguments to function 'BUG'
BUG(skb_copy_bits(skb, ptr, &tmp, 1));
^~~
In file included from include/linux/bug.h:5:0,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from net/ipv4/tcp_input.c:67:
arch/mips/include/asm/bug.h:12:31: note: declared here
static inline void __noreturn BUG(void)
^~~
vim +/BUG +4924 net/ipv4/tcp_input.c
4838
4839 /* Collapse contiguous sequence of skbs head..tail with
4840 * sequence numbers start..end.
4841 *
4842 * If tail is NULL, this means until the end of the queue.
4843 *
4844 * Segments with FIN/SYN are not collapsed (only because this
4845 * simplifies code)
4846 */
4847 static void
4848 tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
4849 struct sk_buff *head, struct sk_buff *tail, u32 start, u32 end)
4850 {
4851 struct sk_buff *skb = head, *n;
4852 struct sk_buff_head tmp;
4853 bool end_of_skbs;
4854
4855 /* First, check that queue is collapsible and find
4856 * the point where collapsing can be useful.
4857 */
4858 restart:
4859 for (end_of_skbs = true; skb != NULL && skb != tail; skb = n) {
4860 n = tcp_skb_next(skb, list);
4861
4862 /* No new bits? It is possible on ofo queue. */
4863 if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
4864 skb = tcp_collapse_one(sk, skb, list, root);
4865 if (!skb)
4866 break;
4867 goto restart;
4868 }
4869
4870 /* The first skb to collapse is:
4871 * - not SYN/FIN and
4872 * - bloated or contains data before "start" or
4873 * overlaps to the next one.
4874 */
4875 if (!(TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) &&
4876 (tcp_win_from_space(sk, skb->truesize) > skb->len ||
4877 before(TCP_SKB_CB(skb)->seq, start))) {
4878 end_of_skbs = false;
4879 break;
4880 }
4881
4882 if (n && n != tail &&
4883 TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(n)->seq) {
4884 end_of_skbs = false;
4885 break;
4886 }
4887
4888 /* Decided to skip this, advance start seq. */
4889 start = TCP_SKB_CB(skb)->end_seq;
4890 }
4891 if (end_of_skbs ||
4892 (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)))
4893 return;
4894
4895 __skb_queue_head_init(&tmp);
4896
4897 while (before(start, end)) {
4898 int copy = min_t(int, SKB_MAX_ORDER(0, 0), end - start);
4899 struct sk_buff *nskb;
4900
4901 nskb = alloc_skb(copy, GFP_ATOMIC);
4902 if (!nskb)
4903 break;
4904
4905 memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
4906 #ifdef CONFIG_TLS_DEVICE
4907 nskb->decrypted = skb->decrypted;
4908 #endif
4909 TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;
4910 if (list)
4911 __skb_queue_before(list, skb, nskb);
4912 else
4913 __skb_queue_tail(&tmp, nskb); /* defer rbtree insertion */
4914 skb_set_owner_r(nskb, sk);
4915
4916 /* Copy data, releasing collapsed skbs. */
4917 while (copy > 0) {
4918 int offset = start - TCP_SKB_CB(skb)->seq;
4919 int size = TCP_SKB_CB(skb)->end_seq - start;
4920
4921 BUG_ON(offset < 0);
4922 if (size > 0) {
4923 size = min(copy, size);
> 4924 BUG(skb_copy_bits(skb, offset,
4925 skb_put(nskb, size), size));
4926 TCP_SKB_CB(nskb)->end_seq += size;
4927 copy -= size;
4928 start += size;
4929 }
4930 if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
4931 skb = tcp_collapse_one(sk, skb, list, root);
4932 if (!skb ||
4933 skb == tail ||
4934 (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)))
4935 goto end;
4936 #ifdef CONFIG_TLS_DEVICE
4937 if (skb->decrypted != nskb->decrypted)
4938 goto end;
4939 #endif
4940 }
4941 }
4942 }
4943 end:
4944 skb_queue_walk_safe(&tmp, skb, n)
4945 tcp_rbtree_insert(root, skb);
4946 }
4947
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 2018/9/10 23:39, Jiri Benc wrote:
> On Mon, 10 Sep 2018 22:38:02 +0800, zhong jiang wrote:
>> + BUG(skb_copy_bits(skb, ptr, &tmp, 1));
> I doubt this builds.
This is my fault. should Be BUG_ON
Thanks,
zhong jiang
> Jiri
>
> .
>
On 9/10/2018 5:38 PM, zhong jiang wrote:
> The if condition can be removed if we use BUG_ON directly.
> The issule is detected with the help of Coccinelle.
Issue?
>
> Signed-off-by: zhong jiang <[email protected]>
> ---
> net/ipv4/tcp_input.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 62508a2..893bde3 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4934,8 +4934,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
> BUG_ON(offset < 0);
> if (size > 0) {
> size = min(copy, size);
> - if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
> - BUG();
> + BUG(skb_copy_bits(skb, offset,
You said BUG_ON()?
> + skb_put(nskb, size), size));
> TCP_SKB_CB(nskb)->end_seq += size;
> copy -= size;
> start += size;
> @@ -5327,8 +5327,8 @@ static void tcp_urg(struct sock *sk, struct sk_buff *skb, const struct tcphdr *t
> /* Is the urgent pointer pointing into this packet? */
> if (ptr < skb->len) {
> u8 tmp;
> - if (skb_copy_bits(skb, ptr, &tmp, 1))
> - BUG();
> +
> + BUG(skb_copy_bits(skb, ptr, &tmp, 1));
Likewise.
[...]
MBR, Sergei
On 2018/9/11 16:51, Sergei Shtylyov wrote:
> On 9/10/2018 5:38 PM, zhong jiang wrote:
>
>> The if condition can be removed if we use BUG_ON directly.
>> The issule is detected with the help of Coccinelle.
>
> Issue?
>
>>
>> Signed-off-by: zhong jiang <[email protected]>
>> ---
>> net/ipv4/tcp_input.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index 62508a2..893bde3 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -4934,8 +4934,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>> BUG_ON(offset < 0);
>> if (size > 0) {
>> size = min(copy, size);
>> - if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>> - BUG();
>> + BUG(skb_copy_bits(skb, offset,
>
> You said BUG_ON()?
Yep. Do you think that it is worthing to do
Thanks,
zhong jiang
>
>> + skb_put(nskb, size), size));
>> TCP_SKB_CB(nskb)->end_seq += size;
>> copy -= size;
>> start += size;
>> @@ -5327,8 +5327,8 @@ static void tcp_urg(struct sock *sk, struct sk_buff *skb, const struct tcphdr *t
>> /* Is the urgent pointer pointing into this packet? */
>> if (ptr < skb->len) {
>> u8 tmp;
>> - if (skb_copy_bits(skb, ptr, &tmp, 1))
>> - BUG();
>> +
>> + BUG(skb_copy_bits(skb, ptr, &tmp, 1));
>
> Likewise.
>
> [...]
>
> MBR, Sergei
>
> .
>
On 9/11/2018 11:59 AM, zhong jiang wrote:
>>> The if condition can be removed if we use BUG_ON directly.
>>> The issule is detected with the help of Coccinelle.
>>
>> Issue?
>>
>>>
>>> Signed-off-by: zhong jiang <[email protected]>
>>> ---
>>> net/ipv4/tcp_input.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>>> index 62508a2..893bde3 100644
>>> --- a/net/ipv4/tcp_input.c
>>> +++ b/net/ipv4/tcp_input.c
>>> @@ -4934,8 +4934,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>>> BUG_ON(offset < 0);
>>> if (size > 0) {
>>> size = min(copy, size);
>>> - if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>>> - BUG();
>>> + BUG(skb_copy_bits(skb, offset,
>>
>> You said BUG_ON()?
> Yep. Do you think that it is worthing to do
I think BUG() doesn't take parameters, BUG_ON() does. Have you tried to
build the kernel with your patch at all?
> Thanks,
> zhong jiang
[...]
MBR, Sergei
On 2018/9/11 17:11, Sergei Shtylyov wrote:
> On 9/11/2018 11:59 AM, zhong jiang wrote:
>
>>>> The if condition can be removed if we use BUG_ON directly.
>>>> The issule is detected with the help of Coccinelle.
>>>
>>> Issue?
>>>
>>>>
>>>> Signed-off-by: zhong jiang <[email protected]>
>>>> ---
>>>> net/ipv4/tcp_input.c | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>>>> index 62508a2..893bde3 100644
>>>> --- a/net/ipv4/tcp_input.c
>>>> +++ b/net/ipv4/tcp_input.c
>>>> @@ -4934,8 +4934,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>>>> BUG_ON(offset < 0);
>>>> if (size > 0) {
>>>> size = min(copy, size);
>>>> - if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>>>> - BUG();
>>>> + BUG(skb_copy_bits(skb, offset,
>>>
>>> You said BUG_ON()?
>> Yep. Do you think that it is worthing to do
>
> I think BUG() doesn't take parameters, BUG_ON() does. Have you tried to build the kernel with your patch at all?
>
I know that the patch should be BUG_ON instead of BUG. This is my mistake. I just want to know that it is worthing to do so.
Thanks,
zhong jiang
>> Thanks,
>> zhong jiang
> [...]
>
> MBR, Sergei
>
>
>
On 9/11/2018 12:19 PM, zhong jiang wrote:
>>>>> The if condition can be removed if we use BUG_ON directly.
>>>>> The issule is detected with the help of Coccinelle.
>>>>
>>>> Issue?
>>>>
>>>>>
>>>>> Signed-off-by: zhong jiang <[email protected]>
>>>>> ---
>>>>> net/ipv4/tcp_input.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>>>>> index 62508a2..893bde3 100644
>>>>> --- a/net/ipv4/tcp_input.c
>>>>> +++ b/net/ipv4/tcp_input.c
>>>>> @@ -4934,8 +4934,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>>>>> BUG_ON(offset < 0);
>>>>> if (size > 0) {
>>>>> size = min(copy, size);
>>>>> - if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>>>>> - BUG();
>>>>> + BUG(skb_copy_bits(skb, offset,
>>>>
>>>> You said BUG_ON()?
>>> Yep. Do you think that it is worthing to do
>>
>> I think BUG() doesn't take parameters, BUG_ON() does. Have you tried to build the kernel with your patch at all?
>>
> I know that the patch should be BUG_ON instead of BUG. This is my mistake. I just want to know that it is worthing to do so.
Yes, probably.
> Thanks,
> zhong jiang
[...]
MBR, Sergei
From: zhong jiang <[email protected]>
Date: Mon, 10 Sep 2018 22:38:02 +0800
> The if condition can be removed if we use BUG_ON directly.
> The issule is detected with the help of Coccinelle.
>
> Signed-off-by: zhong jiang <[email protected]>
You keep asking if this is worthy to do.
I think the most worthy thing to do is actually build test your
patches.
On 2018/9/12 14:54, David Miller wrote:
> From: zhong jiang <[email protected]>
> Date: Mon, 10 Sep 2018 22:38:02 +0800
>
>> The if condition can be removed if we use BUG_ON directly.
>> The issule is detected with the help of Coccinelle.
>>
>> Signed-off-by: zhong jiang <[email protected]>
> You keep asking if this is worthy to do.
>
> I think the most worthy thing to do is actually build test your
> patches.
I am sorry for that. :-[
Thanks,
zhong jiang