Skip to contentSkip to conversation

fm10k: Avoid crashing the kernel

PATCH Superseded Bruce Allan 4 participants 6 messages Dec 22, 2015 – Feb 8, 2016

Subsystems (2)
Network drivers Intel Ethernet

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Bruce Allan

author
#0 lore raw

PATCH 1/1 fm10k: Avoid crashing the kernel

Use BUILD_BUG_ON() instead of BUG_ON() where appropriate to get a compile error rather than crash the kernel.

Signed-off-by Bruce Allan <[email protected]>
---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +2 -2
index 28837ae..6a9f988 100644
@@ -398,7 +398,7 @@ static void fm10k_get_reg_q(struct fm10k_hw *hw, u32 *buff, int i)
 	buff[idx++] = fm10k_read_reg(hw, FM10K_TX_SGLORT(i));
 	buff[idx++] = fm10k_read_reg(hw, FM10K_PFVTCTL(i));
 
-	BUG_ON(idx != FM10K_REGS_LEN_Q);
+	BUILD_BUG_ON(idx != FM10K_REGS_LEN_Q);
 }
 
 /* If function above adds more registers this define needs to be updated */

@@ -414,7 +414,7 @@ static void fm10k_get_reg_vsi(struct fm10k_hw *hw, u32 *buff, int i)
 	for (j = 0; j < 32; j++)
 		buff[idx++] = fm10k_read_reg(hw, FM10K_RETA(i, j));
 
-	BUG_ON(idx != FM10K_REGS_LEN_VSI);
+	BUILD_BUG_ON(idx != FM10K_REGS_LEN_VSI);
 }
 
 static void fm10k_get_regs(struct net_device *netdev,
#1 lore raw

PATCH 1/1 fm10k: Avoid crashing the kernel

Quoted 22 lines · Bruce Allan

Use BUILD_BUG_ON() instead of BUG_ON() where appropriate to get a compile error rather than crash the kernel.

Signed-off-by Bruce Allan <[email protected]>
---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +1 -1
index 28837ae..6a9f988 100644
@@ -398,7 +398,7 @@ static void fm10k_get_reg_q(struct fm10k_hw *hw, u32 *buff, int i)
        buff[idx++] = fm10k_read_reg(hw, FM10K_TX_SGLORT(i));
        buff[idx++] = fm10k_read_reg(hw, FM10K_PFVTCTL(i));
 
-       BUG_ON(idx != FM10K_REGS_LEN_Q);
+       BUILD_BUG_ON(idx != FM10K_REGS_LEN_Q);
 }
 
 /* If function above adds more registers this define needs to be updated */

drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c static void fm10k_get_reg_vsi(struct fm10k_hw *hw, u32 *buff, int i) · line 420

Quoted 8 lines
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +1 -1
@@ -414,7 +414,7 @@ static void fm10k_get_reg_vsi(struct fm10k_hw *hw, u32 *buff, int i)
        for (j = 0; j < 32; j++)
                buff[idx++] = fm10k_read_reg(hw, FM10K_RETA(i, j));
 
-       BUG_ON(idx != FM10K_REGS_LEN_VSI);
+       BUILD_BUG_ON(idx != FM10K_REGS_LEN_VSI);
 }
 
 static void fm10k_get_regs(struct net_device *netdev,

I'm not sure that is entirely valid. It seems like BUILD_BUG_ON should be given an expression that can be tested at build time, but the way this code is written I would suspect idx is likely not going to be a fixed value unless you are compiling with -03 which would likely unfold most of the loops, so I don't see how BUILD_BUG_ON will do anything other than be compiled out.

- Alex

#2 lore raw

PATCH 1/1 fm10k: Avoid crashing the kernel

Quoted 46 lines

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com]
Sent: Wednesday, December 23, 2015 8:23 AM
To: Allan, Bruce W
Cc: intel-wired-lan
Subject: Re: [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel

Quoted 1 line · Bruce Allan

Use BUILD_BUG_ON() instead of BUG_ON() where appropriate to get a

compile

Review

Quoted 8 lines

error rather than crash the kernel.

Signed-off-by Bruce Allan <[email protected]>
---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c

drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c static void fm10k_get_reg_q(struct fm10k_hw

Quoted 4 lines

index 28837ae..6a9f988 100644

drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +0 -0
@@ -398,7 +398,7 @@ static void fm10k_get_reg_q(struct fm10k_hw

*hw, u32 *buff, int i)

drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c static void fm10k_get_reg_q(struct fm10k_hw *hw, u32 *buff, int i)

Quoted 8 lines
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +1 -1
        buff[idx++] = fm10k_read_reg(hw, FM10K_TX_SGLORT(i));
        buff[idx++] = fm10k_read_reg(hw, FM10K_PFVTCTL(i));
 
-       BUG_ON(idx != FM10K_REGS_LEN_Q);
+       BUILD_BUG_ON(idx != FM10K_REGS_LEN_Q);
 }
 
 /* If function above adds more registers this define needs to be updated

*/

drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c static void fm10k_get_reg_vsi(struct fm10k_hw

Quoted 1 line
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +0 -0
@@ -414,7 +414,7 @@ static void fm10k_get_reg_vsi(struct fm10k_hw

*hw, u32 *buff, int i)

drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c static void fm10k_get_reg_vsi(struct fm10k_hw *hw, u32 *buff, int i) · line 420

Quoted 8 lines
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +1 -1
        for (j = 0; j < 32; j++)
                buff[idx++] = fm10k_read_reg(hw, FM10K_RETA(i, j));
 
-       BUG_ON(idx != FM10K_REGS_LEN_VSI);
+       BUILD_BUG_ON(idx != FM10K_REGS_LEN_VSI);
 }
 
 static void fm10k_get_regs(struct net_device *netdev,
Quoted 8 lines

I'm not sure that is entirely valid. It seems like BUILD_BUG_ON should be given an expression that can be tested at build time, but the way this code is written I would suspect idx is likely not going to be a fixed value unless you are compiling with -03 which would likely unfold most of the loops, so I don't see how BUILD_BUG_ON will do anything other than be compiled out.

- Alex

Compilers are very smart today. This patch works as described (i.e. causes build errors) when idx would not equal the defined constant where it is checked against for all available levels of optimization in gcc.

It does generate an undesirable error with -O0 and -Og even if idx would equal the defined constant, but with those optimization levels errors would be generated for other similar instances of BUILD_BUG_ON() in the rest of the kernel source (not to mention a significant number of other "errors").

Bruce.

#3 lore raw

PATCH 1/1 fm10k: Avoid crashing the kernel

Quoted 3 lines · Bruce W

Compilers are very smart today. This patch works as described (i.e. causes build errors) when idx would not equal the defined constant where it is checked against for all available levels of optimization in gcc.

It does generate an undesirable error with -O0 and -Og even if idx would equal the defined constant, but with those optimization levels errors would be generated for other similar instances of BUILD_BUG_ON() in the rest of the kernel source (not to mention a significant number of other "errors").

I had a hard time believing it was optimizing to this level, but it looks like you are right. On my system I do see the BUILD_BUG_ON triggered if I tweak FM10K_REGS_LEN_VSI which was my concern since I wasn't sure it would unroll all the loop logic, but it looks like it did it enough with either -Os or -O2 to work correctly. I'll go ahead and throw my ack in there since it seems like this works, and I suspect worse case scenario would be that the some compilers may just ignore it if they don't take care of the loop unrolling.

Acked-by Alexander Duyck <[email protected]>
#4 lore raw

PATCH 1/1 fm10k: Avoid crashing the kernel

In reply to Bruce Allan

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Bruce Allan
Sent: Tuesday, December 22, 2015 2:10 PM
To: [email protected]
Subject: [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel

Use BUILD_BUG_ON() instead of BUG_ON() where appropriate to get a compile error rather than crash the kernel.

Signed-off-by Bruce Allan <[email protected]>

---

Tested-by Krishneil Singh <[email protected]>
#5 lore raw

PATCH 1/1 fm10k: Avoid crashing the kernel

In reply to Bruce Allan
Quoted 8 lines · Bruce Allan

Use BUILD_BUG_ON() instead of BUG_ON() where appropriate to get a compile
error rather than crash the kernel.

Signed-off-by Bruce Allan <[email protected]>

---
?drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c |??? 4 ++-- ?1 file changed, 2 insertions(+), 2 deletions(-)

Dropping this patch because it now causes a compile issue with my next- queue tree (dev-queue branch), here is the compile errors recieved:

[04:04:54 @jtkirshe-linux:next-queue]$ make -j 77 -s
In file included from include/linux/linkage.h:4:0,
?????????????????from include/linux/preempt.h:9,
?????????????????from include/linux/spinlock.h:50,
?????????????????from include/linux/vmalloc.h:4,
?????????????????from
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c:21:
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c: In function
?fm10k_get_reg_vsi?:
include/linux/compiler.h:501:38: error: call to
?__compiletime_assert_487? declared with attribute error: BUILD_BUG_ON
failed: idx != FM10K_REGS_LEN_VSI
? _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) ??????????????????????????????????????^
include/linux/compiler.h:484:4: note: in definition of macro
?__compiletime_assert?
????prefix ## suffix();????\
????^
include/linux/compiler.h:501:2: note: in expansion of macro
?_compiletime_assert?
? _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
? ^
include/linux/bug.h:50:37: note: in expansion of macro
?compiletime_assert?
?#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) ?????????????????????????????????????^
include/linux/bug.h:74:2: note: in expansion of macro
?BUILD_BUG_ON_MSG?
? BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
? ^
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c:487:2: note: in
expansion of macro ?BUILD_BUG_ON?
? BUILD_BUG_ON(idx != FM10K_REGS_LEN_VSI);
? ^
scripts/Makefile.build:258: recipe for target 'drivers/net/ethernet/intel/fm10k/fm10k_ethtool.o' failed
make[5]: *** [drivers/net/ethernet/intel/fm10k/fm10k_ethtool.o] Error 1
make[5]: *** Waiting for unfinished jobs....
scripts/Makefile.build:407: recipe for target
'drivers/net/ethernet/intel/fm10k' failed
make[4]: *** [drivers/net/ethernet/intel/fm10k] Error 2
make[4]: *** Waiting for unfinished jobs....
scripts/Makefile.build:407: recipe for target
'drivers/net/ethernet/intel' failed
make[3]: *** [drivers/net/ethernet/intel] Error 2
scripts/Makefile.build:407: recipe for target 'drivers/net/ethernet'
failed
make[2]: *** [drivers/net/ethernet] Error 2
scripts/Makefile.build:407: recipe for target 'drivers/net' failed
make[1]: *** [drivers/net] Error 2
Makefile:950: recipe for target 'drivers' failed
make: *** [drivers] Error 2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160208/76dbb2ac/attachment.asc>