| Message ID | 20160309202557.137597.51678.stgit@bwallan-cwp1.jf.intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of Bruce Allan > Sent: Wednesday, March 09, 2016 12:26 PM > To: intel-wired-lan@lists.osuosl.org > Subject: [Intel-wired-lan] [next-queue PATCH v3] 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. > > Unfortunately, BUILD_BUG_ON() cannot be used to replace the second > instance > of BUG_ON() as it will generate a compiletime assert when the compiler flag > -fsanitize=signed-integer-overflow is used (e.g. when UBSAN is enabled). > Instead, put in additional checks to prevent buffer overflow from occuring > and WARN if the code is ever mistakenly changed to get additional register > values without also updating the value of FM10K_REGS_LEN_VSI. > > Also, replace associated magic numbers with existing defined constants and > correct an applicable comment. > > Signed-off-by: Bruce Allan <bruce.w.allan@intel.com> > --- > v3: changes described in 2nd and 3rd paragraphs in updated description > drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 22 +++++++++++++- > -------- > 1 file changed, 13 insertions(+), 9 deletions(-) This version supersedes Jake's patch with the same title (which is really a v2 of the original one I sent) that is currently applied to next-queue/dev-queue.
No idea why this is the case, but with this patch applied I can't pass traffic over i40e devices. Revert the patch and I can pass traffic just fine. I don't know why it'd affect it though, I don't even have any fm10k devices in my system (I do have an ixgbe card) > -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of Allan, Bruce W > Sent: Wednesday, March 09, 2016 12:29 PM > To: Allan, Bruce W <bruce.w.allan@intel.com>; intel-wired- > lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [next-queue PATCH v3] fm10k: Avoid crashing > the kernel > > > -----Original Message----- > > From: Intel-wired-lan > > [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Bruce > > Allan > > Sent: Wednesday, March 09, 2016 12:26 PM > > To: intel-wired-lan@lists.osuosl.org > > Subject: [Intel-wired-lan] [next-queue PATCH v3] 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. > > > > Unfortunately, BUILD_BUG_ON() cannot be used to replace the second > > instance of BUG_ON() as it will generate a compiletime assert when the > > compiler flag -fsanitize=signed-integer-overflow is used (e.g. when > > UBSAN is enabled). > > Instead, put in additional checks to prevent buffer overflow from > > occuring and WARN if the code is ever mistakenly changed to get > > additional register values without also updating the value of > FM10K_REGS_LEN_VSI. > > > > Also, replace associated magic numbers with existing defined constants > > and correct an applicable comment. > > > > Signed-off-by: Bruce Allan <bruce.w.allan@intel.com> > > --- > > v3: changes described in 2nd and 3rd paragraphs in updated description > > drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 22 > +++++++++++++- > > -------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > This version supersedes Jake's patch with the same title (which is really a v2 > of the original one I sent) that is currently applied to next-queue/dev-queue. > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@lists.osuosl.org > http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> -----Original Message----- > From: Bowers, AndrewX > Sent: Thursday, March 10, 2016 10:35 AM > To: Allan, Bruce W <bruce.w.allan@intel.com>; intel-wired- > lan@lists.osuosl.org > Subject: RE: [Intel-wired-lan] [next-queue PATCH v3] fm10k: Avoid crashing > the kernel > > > No idea why this is the case, but with this patch applied I can't pass traffic > over i40e devices. Revert the patch and I can pass traffic just fine. > > I don't know why it'd affect it though, I don't even have any fm10k devices > in my system (I do have an ixgbe card) Issue with i40e cannot be related to this patch...there must be another explanation.
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c index c8ee3b5..6952ce2 100644 --- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c +++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c @@ -352,23 +352,27 @@ 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 */ -#define FM10K_REGS_LEN_VSI 43 +/* If function below adds more registers this define needs to be updated */ +#define FM10K_REGS_LEN_VSI (1 + FM10K_RSSRK_SIZE + FM10K_RETA_SIZE) static void fm10k_get_reg_vsi(struct fm10k_hw *hw, u32 *buff, int i) { int idx = 0, j; buff[idx++] = fm10k_read_reg(hw, FM10K_MRQC(i)); - for (j = 0; j < 10; j++) - buff[idx++] = fm10k_read_reg(hw, FM10K_RSSRK(i, j)); - for (j = 0; j < 32; j++) - buff[idx++] = fm10k_read_reg(hw, FM10K_RETA(i, j)); - - BUG_ON(idx != FM10K_REGS_LEN_VSI); + for (j = 0; j < FM10K_RSSRK_SIZE; j++, idx++) + if (idx < FM10K_REGS_LEN_VSI) + buff[idx] = fm10k_read_reg(hw, FM10K_RSSRK(i, j)); + for (j = 0; j < FM10K_RETA_SIZE; j++, idx++) + if (idx < FM10K_REGS_LEN_VSI) + buff[idx] = fm10k_read_reg(hw, FM10K_RETA(i, j)); + + WARN_ONCE(idx != FM10K_REGS_LEN_VSI, + "Incorrect value for FM10K_REGS_LEN_VSI (expected %d, got %d)\n", + idx, FM10K_REGS_LEN_VSI); } static void fm10k_get_regs(struct net_device *netdev,
Use BUILD_BUG_ON() instead of BUG_ON() where appropriate to get a compile error rather than crash the kernel. Unfortunately, BUILD_BUG_ON() cannot be used to replace the second instance of BUG_ON() as it will generate a compiletime assert when the compiler flag -fsanitize=signed-integer-overflow is used (e.g. when UBSAN is enabled). Instead, put in additional checks to prevent buffer overflow from occuring and WARN if the code is ever mistakenly changed to get additional register values without also updating the value of FM10K_REGS_LEN_VSI. Also, replace associated magic numbers with existing defined constants and correct an applicable comment. Signed-off-by: Bruce Allan <bruce.w.allan@intel.com> --- v3: changes described in 2nd and 3rd paragraphs in updated description drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)