[Intel-wired-lan] [next-queue PATCH] fm10k: Avoid crashing the kernel

Allan, Bruce W bruce.w.allan at intel.com
Wed Dec 23 20:19:57 UTC 2015


> -----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 at 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 at 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.


More information about the Intel-wired-lan mailing list