* [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel
@ 2015-12-22 22:10 Bruce Allan
2015-12-23 16:22 ` Alexander Duyck
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bruce Allan @ 2015-12-22 22:10 UTC (permalink / raw)
To: intel-wired-lan
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 <bruce.w.allan@intel.com>
---
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
index 28837ae..6a9f988 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
@@ -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,
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel
2015-12-22 22:10 [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel Bruce Allan
@ 2015-12-23 16:22 ` Alexander Duyck
2015-12-23 20:19 ` Allan, Bruce W
2015-12-30 0:14 ` Singh, Krishneil K
2016-02-08 12:08 ` Jeff Kirsher
2 siblings, 1 reply; 6+ messages in thread
From: Alexander Duyck @ 2015-12-23 16:22 UTC (permalink / raw)
To: intel-wired-lan
On Tue, Dec 22, 2015 at 2:10 PM, Bruce Allan <bruce.w.allan@intel.com> wrote:
> 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 <bruce.w.allan@intel.com>
> ---
> drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> index 28837ae..6a9f988 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> @@ -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,
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel
2015-12-23 16:22 ` Alexander Duyck
@ 2015-12-23 20:19 ` Allan, Bruce W
2015-12-23 21:11 ` Alexander Duyck
0 siblings, 1 reply; 6+ messages in thread
From: Allan, Bruce W @ 2015-12-23 20:19 UTC (permalink / raw)
To: intel-wired-lan
> -----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
>
> On Tue, Dec 22, 2015 at 2:10 PM, Bruce Allan <bruce.w.allan@intel.com>
> wrote:
> > 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 <bruce.w.allan@intel.com>
> > ---
> > drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> > index 28837ae..6a9f988 100644
> > --- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> > @@ -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,
>
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel
2015-12-23 20:19 ` Allan, Bruce W
@ 2015-12-23 21:11 ` Alexander Duyck
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Duyck @ 2015-12-23 21:11 UTC (permalink / raw)
To: intel-wired-lan
On Wed, Dec 23, 2015 at 12:19 PM, Allan, Bruce W
<bruce.w.allan@intel.com> wrote:
> 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 <aduyck@mirantis.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel
2015-12-22 22:10 [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel Bruce Allan
2015-12-23 16:22 ` Alexander Duyck
@ 2015-12-30 0:14 ` Singh, Krishneil K
2016-02-08 12:08 ` Jeff Kirsher
2 siblings, 0 replies; 6+ messages in thread
From: Singh, Krishneil K @ 2015-12-30 0:14 UTC (permalink / raw)
To: intel-wired-lan
-----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: intel-wired-lan@lists.osuosl.org
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 <bruce.w.allan@intel.com>
---
Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel
2015-12-22 22:10 [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel Bruce Allan
2015-12-23 16:22 ` Alexander Duyck
2015-12-30 0:14 ` Singh, Krishneil K
@ 2016-02-08 12:08 ` Jeff Kirsher
2 siblings, 0 replies; 6+ messages in thread
From: Jeff Kirsher @ 2016-02-08 12:08 UTC (permalink / raw)
To: intel-wired-lan
On Tue, 2015-12-22 at 14:10 -0800, Bruce Allan wrote:
> 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 <bruce.w.allan@intel.com>
> ---
> ?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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-08 12:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-22 22:10 [Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel Bruce Allan
2015-12-23 16:22 ` Alexander Duyck
2015-12-23 20:19 ` Allan, Bruce W
2015-12-23 21:11 ` Alexander Duyck
2015-12-30 0:14 ` Singh, Krishneil K
2016-02-08 12:08 ` Jeff Kirsher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox