Re: [PATCH] scsi: bnx2i: make bnx2i_process_iscsi_error simpler and more robust

2021-03-29 Thread Martin K. Petersen
On Wed, 10 Mar 2021 23:16:02 +0100, Rasmus Villemoes wrote:

> Instead of strcpy'ing into a stack buffer, just let additional_notice
> point to a string literal living in .rodata. This is better in a few
> ways:
> 
> - Smaller .text - instead of gcc compiling the strcpys as a bunch of
>   immediate stores (effectively encoding the string literal in the
>   instruction stream), we only pay the price of storing the literal in
>   .rodata.
> 
> [...]

Applied to 5.13/scsi-queue, thanks!

[1/1] scsi: bnx2i: make bnx2i_process_iscsi_error simpler and more robust
  https://git.kernel.org/mkp/scsi/c/adb253433dc8

-- 
Martin K. Petersen  Oracle Linux Engineering


Re: [PATCH] scsi: bnx2i: make bnx2i_process_iscsi_error simpler and more robust

2021-03-24 Thread Martin K. Petersen


Rasmus,

> Instead of strcpy'ing into a stack buffer, just let additional_notice
> point to a string literal living in .rodata. This is better in a few
> ways:

Applied to 5.13/scsi-staging, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering


[PATCH] scsi: bnx2i: make bnx2i_process_iscsi_error simpler and more robust

2021-03-10 Thread Rasmus Villemoes
Instead of strcpy'ing into a stack buffer, just let additional_notice
point to a string literal living in .rodata. This is better in a few
ways:

- Smaller .text - instead of gcc compiling the strcpys as a bunch of
  immediate stores (effectively encoding the string literal in the
  instruction stream), we only pay the price of storing the literal in
  .rodata.

- Faster, because there's no string copying.

- Smaller stack usage (with my compiler, 72 bytes instead of 176 for
  the sole caller, bnx2i_indicate_kcqe)

Moreover, it's currently possible for additional_notice[] to get used
uninitialized, so some random stack garbage would be passed to
printk() - in the worst case without any '\0' anywhere in those 64
bytes. That could be fixed by initializing additional_notice[0], but
the same is achieved here by initializing the new pointer variable to
"".

Also give the message pointer a similar treatment - there's no point
making temporary copies on the stack of those two strings.

Signed-off-by: Rasmus Villemoes 
---
 drivers/scsi/bnx2i/bnx2i_hwi.c | 85 --
 1 file changed, 41 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index bad396e5c601..43e8a1dafec0 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -2206,10 +2206,8 @@ static void bnx2i_process_iscsi_error(struct bnx2i_hba 
*hba,
 {
struct bnx2i_conn *bnx2i_conn;
u32 iscsi_cid;
-   char warn_notice[] = "iscsi_warning";
-   char error_notice[] = "iscsi_error";
-   char additional_notice[64];
-   char *message;
+   const char *additional_notice = "";
+   const char *message;
int need_recovery;
u64 err_mask64;
 
@@ -2224,133 +,132 @@ static void bnx2i_process_iscsi_error(struct 
bnx2i_hba *hba,
 
if (err_mask64 & iscsi_error_mask) {
need_recovery = 0;
-   message = warn_notice;
+   message = "iscsi_warning";
} else {
need_recovery = 1;
-   message = error_notice;
+   message = "iscsi_error";
}
 
switch (iscsi_err->completion_status) {
case ISCSI_KCQE_COMPLETION_STATUS_HDR_DIG_ERR:
-   strcpy(additional_notice, "hdr digest err");
+   additional_notice = "hdr digest err";
break;
case ISCSI_KCQE_COMPLETION_STATUS_DATA_DIG_ERR:
-   strcpy(additional_notice, "data digest err");
+   additional_notice = "data digest err";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_OPCODE:
-   strcpy(additional_notice, "wrong opcode rcvd");
+   additional_notice = "wrong opcode rcvd";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_AHS_LEN:
-   strcpy(additional_notice, "AHS len > 0 rcvd");
+   additional_notice = "AHS len > 0 rcvd";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_ITT:
-   strcpy(additional_notice, "invalid ITT rcvd");
+   additional_notice = "invalid ITT rcvd";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_STATSN:
-   strcpy(additional_notice, "wrong StatSN rcvd");
+   additional_notice = "wrong StatSN rcvd";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_EXP_DATASN:
-   strcpy(additional_notice, "wrong DataSN rcvd");
+   additional_notice = "wrong DataSN rcvd";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_PEND_R2T:
-   strcpy(additional_notice, "pend R2T violation");
+   additional_notice = "pend R2T violation";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_O_U_0:
-   strcpy(additional_notice, "ERL0, UO");
+   additional_notice = "ERL0, UO";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_O_U_1:
-   strcpy(additional_notice, "ERL0, U1");
+   additional_notice = "ERL0, U1";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_O_U_2:
-   strcpy(additional_notice, "ERL0, U2");
+   additional_notice = "ERL0, U2";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_O_U_3:
-   strcpy(additional_notice, "ERL0, U3");
+   additional_notice = "ERL0, U3";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_O_U_4:
-   strcpy(additional_notice, "ERL0, U4");
+   additional_notice = "ERL0, U4";
break;
case ISCSI_KCQE_COMPLETION_STATUS_PROTOCOL_ERR_O_U_5:
-   strcpy(additional_notice, "ERL0, U5");
+   additional_notice = "ERL0, U5";
break;
case