* [PATCH] net/core: Fix BUG to BUG_ON conditionals.
@ 2017-10-08 19:17 Tim Hansen
2017-10-08 20:03 ` [PATCH v2] " Tim Hansen
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Tim Hansen @ 2017-10-08 19:17 UTC (permalink / raw)
To: davem
Cc: willemb, edumazet, soheil, elena.reshetova, pabeni, tom, Jason,
fw, netdev, linux-kernel, alexander.levin, devtimhansen
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..461516f45b33 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-08 19:17 [PATCH] net/core: Fix BUG to BUG_ON conditionals Tim Hansen
@ 2017-10-08 20:03 ` Tim Hansen
2017-10-09 4:20 ` David Miller
2017-10-08 23:52 ` [PATCH] " kbuild test robot
2017-10-09 6:45 ` kbuild test robot
2 siblings, 1 reply; 14+ messages in thread
From: Tim Hansen @ 2017-10-08 20:03 UTC (permalink / raw)
To: davem
Cc: willemb, edumazet, soheil, elena.reshetova, pabeni, tom, Jason,
fw, netdev, linux-kernel, alexander.levin
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] net/core: Fix BUG to BUG_ON conditionals.
2017-10-08 19:17 [PATCH] net/core: Fix BUG to BUG_ON conditionals Tim Hansen
2017-10-08 20:03 ` [PATCH v2] " Tim Hansen
@ 2017-10-08 23:52 ` kbuild test robot
2017-10-09 6:45 ` kbuild test robot
2 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2017-10-08 23:52 UTC (permalink / raw)
To: Tim Hansen
Cc: kbuild-all, davem, willemb, edumazet, soheil, elena.reshetova,
pabeni, tom, Jason, fw, netdev, linux-kernel, alexander.levin,
devtimhansen
[-- Attachment #1: Type: text/plain, Size: 13088 bytes --]
Hi Tim,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.14-rc3 next-20170929]
[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/Tim-Hansen/net-core-Fix-BUG-to-BUG_ON-conditionals/20171009-070451
config: blackfin-allyesconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.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
make.cross ARCH=blackfin
All errors (new ones prefixed by >>):
net/core/skbuff.c: In function '__pskb_pull_tail':
>> net/core/skbuff.c:1884:2: error: expected ';' before 'if'
if (!skb_has_frag_list(skb))
^~
vim +1884 net/core/skbuff.c
^1da177e4c Linus Torvalds 2005-04-16 1838
^1da177e4c Linus Torvalds 2005-04-16 1839 /**
^1da177e4c Linus Torvalds 2005-04-16 1840 * __pskb_pull_tail - advance tail of skb header
^1da177e4c Linus Torvalds 2005-04-16 1841 * @skb: buffer to reallocate
^1da177e4c Linus Torvalds 2005-04-16 1842 * @delta: number of bytes to advance tail
^1da177e4c Linus Torvalds 2005-04-16 1843 *
^1da177e4c Linus Torvalds 2005-04-16 1844 * The function makes a sense only on a fragmented &sk_buff,
^1da177e4c Linus Torvalds 2005-04-16 1845 * it expands header moving its tail forward and copying necessary
^1da177e4c Linus Torvalds 2005-04-16 1846 * data from fragmented part.
^1da177e4c Linus Torvalds 2005-04-16 1847 *
^1da177e4c Linus Torvalds 2005-04-16 1848 * &sk_buff MUST have reference count of 1.
^1da177e4c Linus Torvalds 2005-04-16 1849 *
^1da177e4c Linus Torvalds 2005-04-16 1850 * Returns %NULL (and &sk_buff does not change) if pull failed
^1da177e4c Linus Torvalds 2005-04-16 1851 * or value of new tail of skb in the case of success.
^1da177e4c Linus Torvalds 2005-04-16 1852 *
^1da177e4c Linus Torvalds 2005-04-16 1853 * All the pointers pointing into skb header may change and must be
^1da177e4c Linus Torvalds 2005-04-16 1854 * reloaded after call to this function.
^1da177e4c Linus Torvalds 2005-04-16 1855 */
^1da177e4c Linus Torvalds 2005-04-16 1856
^1da177e4c Linus Torvalds 2005-04-16 1857 /* Moves tail of skb head forward, copying data from fragmented part,
^1da177e4c Linus Torvalds 2005-04-16 1858 * when it is necessary.
^1da177e4c Linus Torvalds 2005-04-16 1859 * 1. It may fail due to malloc failure.
^1da177e4c Linus Torvalds 2005-04-16 1860 * 2. It may change skb pointers.
^1da177e4c Linus Torvalds 2005-04-16 1861 *
^1da177e4c Linus Torvalds 2005-04-16 1862 * It is pretty complicated. Luckily, it is called only in exceptional cases.
^1da177e4c Linus Torvalds 2005-04-16 1863 */
af72868b90 Johannes Berg 2017-06-16 1864 void *__pskb_pull_tail(struct sk_buff *skb, int delta)
^1da177e4c Linus Torvalds 2005-04-16 1865 {
^1da177e4c Linus Torvalds 2005-04-16 1866 /* If skb has not enough free space at tail, get new one
^1da177e4c Linus Torvalds 2005-04-16 1867 * plus 128 bytes for future expansions. If we have enough
^1da177e4c Linus Torvalds 2005-04-16 1868 * room at tail, reallocate without expansion only if skb is cloned.
^1da177e4c Linus Torvalds 2005-04-16 1869 */
4305b54135 Arnaldo Carvalho de Melo 2007-04-19 1870 int i, k, eat = (skb->tail + delta) - skb->end;
^1da177e4c Linus Torvalds 2005-04-16 1871
^1da177e4c Linus Torvalds 2005-04-16 1872 if (eat > 0 || skb_cloned(skb)) {
^1da177e4c Linus Torvalds 2005-04-16 1873 if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
^1da177e4c Linus Torvalds 2005-04-16 1874 GFP_ATOMIC))
^1da177e4c Linus Torvalds 2005-04-16 1875 return NULL;
^1da177e4c Linus Torvalds 2005-04-16 1876 }
^1da177e4c Linus Torvalds 2005-04-16 1877
b4ef80dbcb Tim Hansen 2017-10-08 1878 BUG_ON(skb_copy_bits(skb, skb_headlen(skb),
b4ef80dbcb Tim Hansen 2017-10-08 1879 skb_tail_pointer(skb), delta))
^1da177e4c Linus Torvalds 2005-04-16 1880
^1da177e4c Linus Torvalds 2005-04-16 1881 /* Optimization: no fragments, no reasons to preestimate
^1da177e4c Linus Torvalds 2005-04-16 1882 * size of pulled pages. Superb.
^1da177e4c Linus Torvalds 2005-04-16 1883 */
21dc330157 David S. Miller 2010-08-23 @1884 if (!skb_has_frag_list(skb))
^1da177e4c Linus Torvalds 2005-04-16 1885 goto pull_pages;
^1da177e4c Linus Torvalds 2005-04-16 1886
^1da177e4c Linus Torvalds 2005-04-16 1887 /* Estimate size of pulled pages. */
^1da177e4c Linus Torvalds 2005-04-16 1888 eat = delta;
^1da177e4c Linus Torvalds 2005-04-16 1889 for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
9e903e0852 Eric Dumazet 2011-10-18 1890 int size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
9e903e0852 Eric Dumazet 2011-10-18 1891
9e903e0852 Eric Dumazet 2011-10-18 1892 if (size >= eat)
^1da177e4c Linus Torvalds 2005-04-16 1893 goto pull_pages;
9e903e0852 Eric Dumazet 2011-10-18 1894 eat -= size;
^1da177e4c Linus Torvalds 2005-04-16 1895 }
^1da177e4c Linus Torvalds 2005-04-16 1896
^1da177e4c Linus Torvalds 2005-04-16 1897 /* If we need update frag list, we are in troubles.
^1da177e4c Linus Torvalds 2005-04-16 1898 * Certainly, it possible to add an offset to skb data,
^1da177e4c Linus Torvalds 2005-04-16 1899 * but taking into account that pulling is expected to
^1da177e4c Linus Torvalds 2005-04-16 1900 * be very rare operation, it is worth to fight against
^1da177e4c Linus Torvalds 2005-04-16 1901 * further bloating skb head and crucify ourselves here instead.
^1da177e4c Linus Torvalds 2005-04-16 1902 * Pure masohism, indeed. 8)8)
^1da177e4c Linus Torvalds 2005-04-16 1903 */
^1da177e4c Linus Torvalds 2005-04-16 1904 if (eat) {
^1da177e4c Linus Torvalds 2005-04-16 1905 struct sk_buff *list = skb_shinfo(skb)->frag_list;
^1da177e4c Linus Torvalds 2005-04-16 1906 struct sk_buff *clone = NULL;
^1da177e4c Linus Torvalds 2005-04-16 1907 struct sk_buff *insp = NULL;
^1da177e4c Linus Torvalds 2005-04-16 1908
^1da177e4c Linus Torvalds 2005-04-16 1909 do {
09a626600b Kris Katterjohn 2006-01-08 1910 BUG_ON(!list);
^1da177e4c Linus Torvalds 2005-04-16 1911
^1da177e4c Linus Torvalds 2005-04-16 1912 if (list->len <= eat) {
^1da177e4c Linus Torvalds 2005-04-16 1913 /* Eaten as whole. */
^1da177e4c Linus Torvalds 2005-04-16 1914 eat -= list->len;
^1da177e4c Linus Torvalds 2005-04-16 1915 list = list->next;
^1da177e4c Linus Torvalds 2005-04-16 1916 insp = list;
^1da177e4c Linus Torvalds 2005-04-16 1917 } else {
^1da177e4c Linus Torvalds 2005-04-16 1918 /* Eaten partially. */
^1da177e4c Linus Torvalds 2005-04-16 1919
^1da177e4c Linus Torvalds 2005-04-16 1920 if (skb_shared(list)) {
^1da177e4c Linus Torvalds 2005-04-16 1921 /* Sucks! We need to fork list. :-( */
^1da177e4c Linus Torvalds 2005-04-16 1922 clone = skb_clone(list, GFP_ATOMIC);
^1da177e4c Linus Torvalds 2005-04-16 1923 if (!clone)
^1da177e4c Linus Torvalds 2005-04-16 1924 return NULL;
^1da177e4c Linus Torvalds 2005-04-16 1925 insp = list->next;
^1da177e4c Linus Torvalds 2005-04-16 1926 list = clone;
^1da177e4c Linus Torvalds 2005-04-16 1927 } else {
^1da177e4c Linus Torvalds 2005-04-16 1928 /* This may be pulled without
^1da177e4c Linus Torvalds 2005-04-16 1929 * problems. */
^1da177e4c Linus Torvalds 2005-04-16 1930 insp = list;
^1da177e4c Linus Torvalds 2005-04-16 1931 }
^1da177e4c Linus Torvalds 2005-04-16 1932 if (!pskb_pull(list, eat)) {
^1da177e4c Linus Torvalds 2005-04-16 1933 kfree_skb(clone);
^1da177e4c Linus Torvalds 2005-04-16 1934 return NULL;
^1da177e4c Linus Torvalds 2005-04-16 1935 }
^1da177e4c Linus Torvalds 2005-04-16 1936 break;
^1da177e4c Linus Torvalds 2005-04-16 1937 }
^1da177e4c Linus Torvalds 2005-04-16 1938 } while (eat);
^1da177e4c Linus Torvalds 2005-04-16 1939
^1da177e4c Linus Torvalds 2005-04-16 1940 /* Free pulled out fragments. */
^1da177e4c Linus Torvalds 2005-04-16 1941 while ((list = skb_shinfo(skb)->frag_list) != insp) {
^1da177e4c Linus Torvalds 2005-04-16 1942 skb_shinfo(skb)->frag_list = list->next;
^1da177e4c Linus Torvalds 2005-04-16 1943 kfree_skb(list);
^1da177e4c Linus Torvalds 2005-04-16 1944 }
^1da177e4c Linus Torvalds 2005-04-16 1945 /* And insert new clone at head. */
^1da177e4c Linus Torvalds 2005-04-16 1946 if (clone) {
^1da177e4c Linus Torvalds 2005-04-16 1947 clone->next = list;
^1da177e4c Linus Torvalds 2005-04-16 1948 skb_shinfo(skb)->frag_list = clone;
^1da177e4c Linus Torvalds 2005-04-16 1949 }
^1da177e4c Linus Torvalds 2005-04-16 1950 }
^1da177e4c Linus Torvalds 2005-04-16 1951 /* Success! Now we may commit changes to skb data. */
^1da177e4c Linus Torvalds 2005-04-16 1952
^1da177e4c Linus Torvalds 2005-04-16 1953 pull_pages:
^1da177e4c Linus Torvalds 2005-04-16 1954 eat = delta;
^1da177e4c Linus Torvalds 2005-04-16 1955 k = 0;
^1da177e4c Linus Torvalds 2005-04-16 1956 for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
9e903e0852 Eric Dumazet 2011-10-18 1957 int size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
9e903e0852 Eric Dumazet 2011-10-18 1958
9e903e0852 Eric Dumazet 2011-10-18 1959 if (size <= eat) {
ea2ab69379 Ian Campbell 2011-08-22 1960 skb_frag_unref(skb, i);
9e903e0852 Eric Dumazet 2011-10-18 1961 eat -= size;
^1da177e4c Linus Torvalds 2005-04-16 1962 } else {
^1da177e4c Linus Torvalds 2005-04-16 1963 skb_shinfo(skb)->frags[k] = skb_shinfo(skb)->frags[i];
^1da177e4c Linus Torvalds 2005-04-16 1964 if (eat) {
^1da177e4c Linus Torvalds 2005-04-16 1965 skb_shinfo(skb)->frags[k].page_offset += eat;
9e903e0852 Eric Dumazet 2011-10-18 1966 skb_frag_size_sub(&skb_shinfo(skb)->frags[k], eat);
3ccc6c6faa linzhang 2017-07-17 1967 if (!i)
3ccc6c6faa linzhang 2017-07-17 1968 goto end;
^1da177e4c Linus Torvalds 2005-04-16 1969 eat = 0;
^1da177e4c Linus Torvalds 2005-04-16 1970 }
^1da177e4c Linus Torvalds 2005-04-16 1971 k++;
^1da177e4c Linus Torvalds 2005-04-16 1972 }
^1da177e4c Linus Torvalds 2005-04-16 1973 }
^1da177e4c Linus Torvalds 2005-04-16 1974 skb_shinfo(skb)->nr_frags = k;
^1da177e4c Linus Torvalds 2005-04-16 1975
3ccc6c6faa linzhang 2017-07-17 1976 end:
^1da177e4c Linus Torvalds 2005-04-16 1977 skb->tail += delta;
^1da177e4c Linus Torvalds 2005-04-16 1978 skb->data_len -= delta;
^1da177e4c Linus Torvalds 2005-04-16 1979
1f8b977ab3 Willem de Bruijn 2017-08-03 1980 if (!skb->data_len)
1f8b977ab3 Willem de Bruijn 2017-08-03 1981 skb_zcopy_clear(skb, false);
1f8b977ab3 Willem de Bruijn 2017-08-03 1982
27a884dc3c Arnaldo Carvalho de Melo 2007-04-19 1983 return skb_tail_pointer(skb);
^1da177e4c Linus Torvalds 2005-04-16 1984 }
b4ac530fc3 David S. Miller 2009-02-10 1985 EXPORT_SYMBOL(__pskb_pull_tail);
^1da177e4c Linus Torvalds 2005-04-16 1986
:::::: The code at line 1884 was first introduced by commit
:::::: 21dc330157454046dd7c494961277d76e1c957fe net: Rename skb_has_frags to skb_has_frag_list
:::::: TO: David S. Miller <davem@davemloft.net>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46057 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-08 20:03 ` [PATCH v2] " Tim Hansen
@ 2017-10-09 4:20 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2017-10-09 4:20 UTC (permalink / raw)
To: devtimhansen
Cc: willemb, edumazet, soheil, elena.reshetova, pabeni, tom, Jason,
fw, netdev, linux-kernel, alexander.levin
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net/core: Fix BUG to BUG_ON conditionals.
2017-10-08 19:17 [PATCH] net/core: Fix BUG to BUG_ON conditionals Tim Hansen
2017-10-08 20:03 ` [PATCH v2] " Tim Hansen
2017-10-08 23:52 ` [PATCH] " kbuild test robot
@ 2017-10-09 6:45 ` kbuild test robot
2 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2017-10-09 6:45 UTC (permalink / raw)
To: Tim Hansen
Cc: kbuild-all, davem, willemb, edumazet, soheil, elena.reshetova,
pabeni, tom, Jason, fw, netdev, linux-kernel, alexander.levin,
devtimhansen
[-- Attachment #1: Type: text/plain, Size: 5990 bytes --]
Hi Tim,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.14-rc4 next-20170929]
[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/Tim-Hansen/net-core-Fix-BUG-to-BUG_ON-conditionals/20171009-070451
config: x86_64-randconfig-s1-10091351 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/module.h:9,
from net//core/skbuff.c:41:
net//core/skbuff.c: In function '__pskb_pull_tail':
>> include/linux/compiler.h:156:2: error: expected ';' before 'if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> net//core/skbuff.c:1884:2: note: in expansion of macro 'if'
if (!skb_has_frag_list(skb))
^~
>> include/linux/compiler.h:170:3: error: expected statement before ')' token
}))
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> net//core/skbuff.c:1884:2: note: in expansion of macro 'if'
if (!skb_has_frag_list(skb))
^~
include/linux/compiler.h:170:4: error: expected statement before ')' token
}))
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> net//core/skbuff.c:1884:2: note: in expansion of macro 'if'
if (!skb_has_frag_list(skb))
^~
--
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/module.h:9,
from net/core/skbuff.c:41:
net/core/skbuff.c: In function '__pskb_pull_tail':
>> include/linux/compiler.h:156:2: error: expected ';' before 'if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net/core/skbuff.c:1884:2: note: in expansion of macro 'if'
if (!skb_has_frag_list(skb))
^~
>> include/linux/compiler.h:170:3: error: expected statement before ')' token
}))
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net/core/skbuff.c:1884:2: note: in expansion of macro 'if'
if (!skb_has_frag_list(skb))
^~
include/linux/compiler.h:170:4: error: expected statement before ')' token
}))
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net/core/skbuff.c:1884:2: note: in expansion of macro 'if'
if (!skb_has_frag_list(skb))
^~
vim +156 include/linux/compiler.h
2bcd521a Steven Rostedt 2008-11-21 148
2bcd521a Steven Rostedt 2008-11-21 149 #ifdef CONFIG_PROFILE_ALL_BRANCHES
2bcd521a Steven Rostedt 2008-11-21 150 /*
2bcd521a Steven Rostedt 2008-11-21 151 * "Define 'is'", Bill Clinton
2bcd521a Steven Rostedt 2008-11-21 152 * "Define 'if'", Steven Rostedt
2bcd521a Steven Rostedt 2008-11-21 153 */
ab3c9c68 Linus Torvalds 2009-04-07 154 #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
ab3c9c68 Linus Torvalds 2009-04-07 155 #define __trace_if(cond) \
b33c8ff4 Arnd Bergmann 2016-02-12 @156 if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
2bcd521a Steven Rostedt 2008-11-21 157 ({ \
2bcd521a Steven Rostedt 2008-11-21 158 int ______r; \
2bcd521a Steven Rostedt 2008-11-21 159 static struct ftrace_branch_data \
2bcd521a Steven Rostedt 2008-11-21 160 __attribute__((__aligned__(4))) \
2bcd521a Steven Rostedt 2008-11-21 161 __attribute__((section("_ftrace_branch"))) \
2bcd521a Steven Rostedt 2008-11-21 162 ______f = { \
2bcd521a Steven Rostedt 2008-11-21 163 .func = __func__, \
2bcd521a Steven Rostedt 2008-11-21 164 .file = __FILE__, \
2bcd521a Steven Rostedt 2008-11-21 165 .line = __LINE__, \
2bcd521a Steven Rostedt 2008-11-21 166 }; \
2bcd521a Steven Rostedt 2008-11-21 167 ______r = !!(cond); \
97e7e4f3 Witold Baryluk 2009-03-17 168 ______f.miss_hit[______r]++; \
2bcd521a Steven Rostedt 2008-11-21 169 ______r; \
2bcd521a Steven Rostedt 2008-11-21 @170 }))
2bcd521a Steven Rostedt 2008-11-21 171 #endif /* CONFIG_PROFILE_ALL_BRANCHES */
2bcd521a Steven Rostedt 2008-11-21 172
:::::: The code at line 156 was first introduced by commit
:::::: b33c8ff4431a343561e2319f17c14286f2aa52e2 tracing: Fix freak link error caused by branch tracer
:::::: TO: Arnd Bergmann <arnd@arndb.de>
:::::: CC: Steven Rostedt <rostedt@goodmis.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29136 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 15:37 [PATCH v2] " Tim Hansen
2017-10-09 17:15 ` Alexei Starovoitov
@ 2017-10-10 19:32 ` David Miller
1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2017-10-10 19:32 UTC (permalink / raw)
To: devtimhansen
Cc: willemb, edumazet, soheil, pabeni, elena.reshetova, tom, Jason,
fw, netdev, linux-kernel, alexander.levin
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!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 23:23 ` Alexei Starovoitov
@ 2017-10-10 1:14 ` Levin, Alexander (Sasha Levin)
0 siblings, 0 replies; 14+ messages in thread
From: Levin, Alexander (Sasha Levin) @ 2017-10-10 1:14 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Tim Hansen, davem, willemb, edumazet, soheil, pabeni,
elena.reshetova, tom, Jason, fw, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 23:15 ` Levin, Alexander (Sasha Levin)
@ 2017-10-09 23:23 ` Alexei Starovoitov
2017-10-10 1:14 ` Levin, Alexander (Sasha Levin)
0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2017-10-09 23:23 UTC (permalink / raw)
To: Levin, Alexander (Sasha Levin)
Cc: Tim Hansen, davem, willemb, edumazet, soheil, pabeni,
elena.reshetova, tom, Jason, fw, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 23:06 ` Alexei Starovoitov
2017-10-09 23:15 ` Levin, Alexander (Sasha Levin)
@ 2017-10-09 23:17 ` David Miller
1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2017-10-09 23:17 UTC (permalink / raw)
To: alexei.starovoitov
Cc: alexander.levin, devtimhansen, willemb, edumazet, soheil, pabeni,
elena.reshetova, tom, Jason, fw, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 23:06 ` Alexei Starovoitov
@ 2017-10-09 23:15 ` Levin, Alexander (Sasha Levin)
2017-10-09 23:23 ` Alexei Starovoitov
2017-10-09 23:17 ` David Miller
1 sibling, 1 reply; 14+ messages in thread
From: Levin, Alexander (Sasha Levin) @ 2017-10-09 23:15 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Tim Hansen, davem, willemb, edumazet, soheil, pabeni,
elena.reshetova, tom, Jason, fw, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 20:26 ` Levin, Alexander (Sasha Levin)
@ 2017-10-09 23:06 ` Alexei Starovoitov
2017-10-09 23:15 ` Levin, Alexander (Sasha Levin)
2017-10-09 23:17 ` David Miller
0 siblings, 2 replies; 14+ messages in thread
From: Alexei Starovoitov @ 2017-10-09 23:06 UTC (permalink / raw)
To: Levin, Alexander (Sasha Levin)
Cc: Tim Hansen, davem, willemb, edumazet, soheil, pabeni,
elena.reshetova, tom, Jason, fw, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 17:15 ` Alexei Starovoitov
@ 2017-10-09 20:26 ` Levin, Alexander (Sasha Levin)
2017-10-09 23:06 ` Alexei Starovoitov
0 siblings, 1 reply; 14+ messages in thread
From: Levin, Alexander (Sasha Levin) @ 2017-10-09 20:26 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Tim Hansen, davem, willemb, edumazet, soheil, pabeni,
elena.reshetova, tom, Jason, fw, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
2017-10-09 15:37 [PATCH v2] " Tim Hansen
@ 2017-10-09 17:15 ` Alexei Starovoitov
2017-10-09 20:26 ` Levin, Alexander (Sasha Levin)
2017-10-10 19:32 ` David Miller
1 sibling, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2017-10-09 17:15 UTC (permalink / raw)
To: Tim Hansen
Cc: davem, willemb, edumazet, soheil, pabeni, elena.reshetova, tom,
Jason, fw, netdev, linux-kernel, alexander.levin
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
@ 2017-10-09 15:37 Tim Hansen
2017-10-09 17:15 ` Alexei Starovoitov
2017-10-10 19:32 ` David Miller
0 siblings, 2 replies; 14+ messages in thread
From: Tim Hansen @ 2017-10-09 15:37 UTC (permalink / raw)
To: davem
Cc: willemb, edumazet, soheil, pabeni, elena.reshetova, tom, Jason,
fw, netdev, linux-kernel, devtimhansen, alexander.levin
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-10-10 19:32 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-08 19:17 [PATCH] net/core: Fix BUG to BUG_ON conditionals Tim Hansen
2017-10-08 20:03 ` [PATCH v2] " Tim Hansen
2017-10-09 4:20 ` David Miller
2017-10-08 23:52 ` [PATCH] " kbuild test robot
2017-10-09 6:45 ` kbuild test robot
2017-10-09 15:37 [PATCH v2] " Tim Hansen
2017-10-09 17:15 ` Alexei Starovoitov
2017-10-09 20:26 ` Levin, Alexander (Sasha Levin)
2017-10-09 23:06 ` Alexei Starovoitov
2017-10-09 23:15 ` Levin, Alexander (Sasha Levin)
2017-10-09 23:23 ` Alexei Starovoitov
2017-10-10 1:14 ` Levin, Alexander (Sasha Levin)
2017-10-09 23:17 ` David Miller
2017-10-10 19:32 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.