Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1639346 > unrolled thread

[PATCH] qed: fix uninitialized data in aRFS intrastructure

Back to article view | Back to linux.kernel

Contents

  [PATCH] qed: fix uninitialized data in aRFS intrastructure Arnd Bergmann <arnd@arndb.de> - 2017-05-11 14:20 +0200
    RE: [PATCH] qed: fix uninitialized data in aRFS intrastructure "Mintz, Yuval" <Yuval.Mintz@cavium.com> - 2017-05-11 16:10 +0200
      RE: [PATCH] qed: fix uninitialized data in aRFS intrastructure "Mintz, Yuval" <Yuval.Mintz@cavium.com> - 2017-05-11 16:40 +0200
        Re: [PATCH] qed: fix uninitialized data in aRFS intrastructure Arnd Bergmann <arnd@arndb.de> - 2017-05-11 17:10 +0200
      Re: [PATCH] qed: fix uninitialized data in aRFS intrastructure Arnd Bergmann <arnd@arndb.de> - 2017-05-11 16:40 +0200

#1639346 — [PATCH] qed: fix uninitialized data in aRFS intrastructure

The new code contains an incredibly elaborate way of setting a 64-bit
register, which went subtly wrong due to the wrong size in a memset():

ethernet/qlogic/qed/qed_init_fw_funcs.c: In function 'qed_set_rfs_mode_disable':
ethernet/qlogic/qed/qed_init_fw_funcs.c:993:3: error: '*((void *)&ramline+4)' is used uninitialized in this function [-Werror=uninitialized]

This removes the silly loop and memset, and instead directly writes
the correct value to the register.

Fixes: d51e4af5c209 ("qed: aRFS infrastructure support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 .../net/ethernet/qlogic/qed/qed_init_fw_funcs.c    | 48 +++++-----------------
 1 file changed, 11 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c b/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c
index 67200c5498ab..a7c2c147a738 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c
@@ -966,45 +966,29 @@ void qed_set_geneve_enable(struct qed_hwfn *p_hwfn,
 #define PARSER_ETH_CONN_CM_HDR (0x0)
 #define CAM_LINE_SIZE sizeof(u32)
 #define RAM_LINE_SIZE sizeof(u64)
-#define REG_SIZE sizeof(u32)
+#define CAM_REG(pf_id) (PRS_REG_GFT_CAM + CAM_LINE_SIZE * (pf_id))
+#define RAM_REG(pf_id) (PRS_REG_GFT_PROFILE_MASK_RAM + RAM_LINE_SIZE * (pf_id))
 
 void qed_set_rfs_mode_disable(struct qed_hwfn *p_hwfn,
 			      struct qed_ptt *p_ptt, u16 pf_id)
 {
-	union gft_cam_line_union camline;
-	struct gft_ram_line ramline;
-	u32 *p_ramline, i;
-
-	p_ramline = (u32 *)&ramline;
-
 	/*stop using gft logic */
 	qed_wr(p_hwfn, p_ptt, PRS_REG_SEARCH_GFT, 0);
 	qed_wr(p_hwfn, p_ptt, PRS_REG_CM_HDR_GFT, 0x0);
-	memset(&camline, 0, sizeof(union gft_cam_line_union));
-	qed_wr(p_hwfn, p_ptt, PRS_REG_GFT_CAM + CAM_LINE_SIZE * pf_id,
-	       camline.cam_line_mapped.camline);
-	memset(&ramline, 0, sizeof(union gft_cam_line_union));
-
-	for (i = 0; i < RAM_LINE_SIZE / REG_SIZE; i++) {
-		u32 hw_addr = PRS_REG_GFT_PROFILE_MASK_RAM;
-
-		hw_addr += (RAM_LINE_SIZE * pf_id + i * REG_SIZE);
-
-		qed_wr(p_hwfn, p_ptt, hw_addr, *(p_ramline + i));
-	}
+	qed_wr(p_hwfn, p_ptt, CAM_REG(pf_id), 0);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id), 0);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id) + 4, 0);
 }
 
 void qed_set_rfs_mode_enable(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 			     u16 pf_id, bool tcp, bool udp,
 			     bool ipv4, bool ipv6)
 {
-	u32 rfs_cm_hdr_event_id, *p_ramline;
+	u32 rfs_cm_hdr_event_id;
 	union gft_cam_line_union camline;
 	struct gft_ram_line ramline;
-	int i;
 
 	rfs_cm_hdr_event_id = qed_rd(p_hwfn, p_ptt, PRS_REG_CM_HDR_GFT);
-	p_ramline = (u32 *)&ramline;
 
 	if (!ipv6 && !ipv4)
 		DP_NOTICE(p_hwfn,
@@ -1060,8 +1044,7 @@ void qed_set_rfs_mode_enable(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 	}
 
 	/* write characteristics to cam */
-	qed_wr(p_hwfn, p_ptt, PRS_REG_GFT_CAM + CAM_LINE_SIZE * pf_id,
-	       camline.cam_line_mapped.camline);
+	qed_wr(p_hwfn, p_ptt, CAM_REG(pf_id), camline.cam_line_mapped.camline);
 	camline.cam_line_mapped.camline = qed_rd(p_hwfn, p_ptt,
 						 PRS_REG_GFT_CAM +
 						 CAM_LINE_SIZE * pf_id);
@@ -1074,19 +1057,10 @@ void qed_set_rfs_mode_enable(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 	SET_FIELD(ramline.low32bits, GFT_RAM_LINE_SRC_PORT, 1);
 	SET_FIELD(ramline.low32bits, GFT_RAM_LINE_DST_PORT, 1);
 
-	/* each iteration write to reg */
-	for (i = 0; i < RAM_LINE_SIZE / REG_SIZE; i++)
-		qed_wr(p_hwfn, p_ptt,
-		       PRS_REG_GFT_PROFILE_MASK_RAM + RAM_LINE_SIZE * pf_id +
-		       i * REG_SIZE, *(p_ramline + i));
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id),     ramline.low32bits);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id) + 4, ramline.high32bits);
 
 	/* set default profile so that no filter match will happen */
-	ramline.low32bits = 0xffff;
-	ramline.high32bits = 0xffff;
-
-	for (i = 0; i < RAM_LINE_SIZE / REG_SIZE; i++)
-		qed_wr(p_hwfn, p_ptt,
-		       PRS_REG_GFT_PROFILE_MASK_RAM + RAM_LINE_SIZE *
-		       PRS_GFT_CAM_LINES_NO_MATCH + i * REG_SIZE,
-		       *(p_ramline + i));
+	qed_wr(p_hwfn, p_ptt, RAM_REG(PRS_GFT_CAM_LINES_NO_MATCH),     0xffff);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(PRS_GFT_CAM_LINES_NO_MATCH) + 4, 0xffff);
 }
-- 
2.9.0

[toc] | [next] | [standalone]

#1639485

> register, which went subtly wrong due to the wrong size in a memset():
> 
> ethernet/qlogic/qed/qed_init_fw_funcs.c: In function
> 'qed_set_rfs_mode_disable':
> ethernet/qlogic/qed/qed_init_fw_funcs.c:993:3: error: '*((void
> *)&ramline+4)' is used uninitialized in this function [-Werror=uninitialized]
> 
> This removes the silly loop and memset, and instead directly writes the
> correct value to the register.

Hi Arnd,

For the most part - I'm almost all in favor of this change.
But just to make it clear - the actual fix could have been a one-liner, right?
The rest are style changes.

> +#define CAM_REG(pf_id) (PRS_REG_GFT_CAM + CAM_LINE_SIZE * (pf_id))
> +#define RAM_REG(pf_id) (PRS_REG_GFT_PROFILE_MASK_RAM +

Not sure I'm a huge fan of this specific style change;
Seems like we could easily manage without these macros.

[toc] | [prev] | [next] | [standalone]

#1639597

> > For the most part - I'm almost all in favor of this change.
> > But just to make it clear - the actual fix could have been a one-liner, right?
> > The rest are style changes.

> Correct. Having the correct length in the memset is a sufficient fix for the warning,
> but it felt wrong to send it since the root of the problem seems to be the
> complexity of the code that was hiding it.

...

> Generally speaking, feel free to treat any of my compile-time warning fix
> patches as simple bug reports and apply a different fix that seems more
> appropriate. I mainly send it in patch form since that seems to be the
> quickest way to address any issues.

Sure. 

Once net-next is re-opened I intend to push our next FW version which
is also going to change some of the aRFS related configurations.

So I think we should stick to the single-liner fix for now,
and I'll revise the style [if still needed; I'll have to check] on that submission.

[toc] | [prev] | [next] | [standalone]

#1639658

On Thu, May 11, 2017 at 4:37 PM, Mintz, Yuval <Yuval.Mintz@cavium.com> wrote:
>> > For the most part - I'm almost all in favor of this change.
>> > But just to make it clear - the actual fix could have been a one-liner, right?
>> > The rest are style changes.
>
>> Correct. Having the correct length in the memset is a sufficient fix for the warning,
>> but it felt wrong to send it since the root of the problem seems to be the
>> complexity of the code that was hiding it.
>
> ...
>
>> Generally speaking, feel free to treat any of my compile-time warning fix
>> patches as simple bug reports and apply a different fix that seems more
>> appropriate. I mainly send it in patch form since that seems to be the
>> quickest way to address any issues.
>
> Sure.
>
> Once net-next is re-opened I intend to push our next FW version which
> is also going to change some of the aRFS related configurations.
>
> So I think we should stick to the single-liner fix for now,
> and I'll revise the style [if still needed; I'll have to check] on that submission.

Sounds good, thanks!

     Arnd

[toc] | [prev] | [next] | [standalone]

#1639623

On Thu, May 11, 2017 at 4:03 PM, Mintz, Yuval <Yuval.Mintz@cavium.com> wrote:
>> register, which went subtly wrong due to the wrong size in a memset():
>>
>> ethernet/qlogic/qed/qed_init_fw_funcs.c: In function
>> 'qed_set_rfs_mode_disable':
>> ethernet/qlogic/qed/qed_init_fw_funcs.c:993:3: error: '*((void
>> *)&ramline+4)' is used uninitialized in this function [-Werror=uninitialized]
>>
>> This removes the silly loop and memset, and instead directly writes the
>> correct value to the register.
>
> Hi Arnd,
>
> For the most part - I'm almost all in favor of this change.
> But just to make it clear - the actual fix could have been a one-liner, right?
> The rest are style changes.

Correct. Having the correct length in the memset is a sufficient fix for
the warning, but it felt wrong to send it since the root of the problem
seems to be the complexity of the code that was hiding it.

>> +#define CAM_REG(pf_id) (PRS_REG_GFT_CAM + CAM_LINE_SIZE * (pf_id))
>> +#define RAM_REG(pf_id) (PRS_REG_GFT_PROFILE_MASK_RAM +
>
> Not sure I'm a huge fan of this specific style change;
> Seems like we could easily manage without these macros.

I tried first and ended up with really long lines that I did not like.

Generally speaking, feel free to treat any of my compile-time warning
fix patches as simple bug reports and apply a different fix that seems
more appropriate. I mainly send it in patch form since that seems to be
the quickest way to address any issues.

      Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web