* [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
@ 2020-11-23 20:20 min.li.xe
2020-11-24 1:23 ` Richard Cochran
0 siblings, 1 reply; 6+ messages in thread
From: min.li.xe @ 2020-11-23 20:20 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Feed kstrtou8 with NULL terminated string.
Changes since v1:
-Use strscpy instead of strncpy for safety.
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_clockmatrix.c | 60 ++++++++++++++++++++++++++++++-------------
tools/bpf/example | 12 +++++++++
tools/bpf/novlan | 7 +++++
3 files changed, 61 insertions(+), 18 deletions(-)
create mode 100644 tools/bpf/example
create mode 100644 tools/bpf/novlan
diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index e020faf..d4e434b 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -103,42 +103,66 @@ static int timespec_to_char_array(struct timespec64 const *ts,
return 0;
}
-static int idtcm_strverscmp(const char *ver1, const char *ver2)
+static int idtcm_strverscmp(const char *version1, const char *version2)
{
u8 num1;
u8 num2;
int result = 0;
+ char ver1[16];
+ char ver2[16];
+ char *cur1;
+ char *cur2;
+ char *next1;
+ char *next2;
+
+ if (strscpy(ver1, version1, 16) < 0 ||
+ strscpy(ver2, version2, 16) < 0)
+ return -1;
+ cur1 = ver1;
+ cur2 = ver2;
/* loop through each level of the version string */
while (result == 0) {
+ next1 = strchr(cur1, '.');
+ next2 = strchr(cur2, '.');
+
+ /* kstrtou8 could fail for dot */
+ if (next1) {
+ *next1 = '\0';
+ next1++;
+ }
+
+ if (next2) {
+ *next2 = '\0';
+ next2++;
+ }
+
/* extract leading version numbers */
- if (kstrtou8(ver1, 10, &num1) < 0)
+ if (kstrtou8(cur1, 10, &num1) < 0)
return -1;
- if (kstrtou8(ver2, 10, &num2) < 0)
+ if (kstrtou8(cur2, 10, &num2) < 0)
return -1;
/* if numbers differ, then set the result */
if (num1 < num2)
+ return -1;
+ if (num1 > num2)
+ return 1;
+
+ /* if numbers are the same, go to next level */
+ if (!next1 && !next2)
+ break;
+ else if (!next1) {
result = -1;
- else if (num1 > num2)
+ } else if (!next2) {
result = 1;
- else {
- /* if numbers are the same, go to next level */
- ver1 = strchr(ver1, '.');
- ver2 = strchr(ver2, '.');
- if (!ver1 && !ver2)
- break;
- else if (!ver1)
- result = -1;
- else if (!ver2)
- result = 1;
- else {
- ver1++;
- ver2++;
- }
+ } else {
+ cur1 = next1;
+ cur2 = next2;
}
}
+
return result;
}
diff --git a/tools/bpf/example b/tools/bpf/example
new file mode 100644
index 0000000..a0ac81f
--- /dev/null
+++ b/tools/bpf/example
@@ -0,0 +1,12 @@
+ ldh [12]
+ jne #0x8100, nonvlan
+ ldh [16]
+ jne #0x88f7, bad
+ ldb [18]
+ ja test
+ nonvlan: jne #0x88f7, bad
+ ldb [14]
+ test: and #0x8
+ jeq #0, bad
+ good: ret #1500
+ bad: ret #0
diff --git a/tools/bpf/novlan b/tools/bpf/novlan
new file mode 100644
index 0000000..fe35288
--- /dev/null
+++ b/tools/bpf/novlan
@@ -0,0 +1,7 @@
+ ldh [12]
+ jne #0x88f7, bad
+ ldb [14]
+ and #0x8
+ jeq #0, bad
+ good: ret #1500
+ bad: ret #0
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
2020-11-23 20:20 [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp min.li.xe
@ 2020-11-24 1:23 ` Richard Cochran
0 siblings, 0 replies; 6+ messages in thread
From: Richard Cochran @ 2020-11-24 1:23 UTC (permalink / raw)
To: min.li.xe; +Cc: netdev, linux-kernel
On Mon, Nov 23, 2020 at 03:20:06PM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Use strscpy instead of strncpy for safety.
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> ---
> drivers/ptp/ptp_clockmatrix.c | 60 ++++++++++++++++++++++++++++++-------------
> tools/bpf/example | 12 +++++++++
> tools/bpf/novlan | 7 +++++
> 3 files changed, 61 insertions(+), 18 deletions(-)
> create mode 100644 tools/bpf/example
> create mode 100644 tools/bpf/novlan
>
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index e020faf..d4e434b 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c
> @@ -103,42 +103,66 @@ static int timespec_to_char_array(struct timespec64 const *ts,
> return 0;
> }
>
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + if (strscpy(ver1, version1, 16) < 0 ||
> + strscpy(ver2, version2, 16) < 0)
> + return -1;
> + cur1 = ver1;
> + cur2 = ver2;
>
> /* loop through each level of the version string */
> while (result == 0) {
> + next1 = strchr(cur1, '.');
> + next2 = strchr(cur2, '.');
> +
> + /* kstrtou8 could fail for dot */
> + if (next1) {
> + *next1 = '\0';
> + next1++;
> + }
> +
> + if (next2) {
> + *next2 = '\0';
> + next2++;
> + }
> +
All of this looping and ad-hoc string parsing can be make MUCH
simpler by using sscanf() and then comparing the binary values
directly.
> /* extract leading version numbers */
> - if (kstrtou8(ver1, 10, &num1) < 0)
> + if (kstrtou8(cur1, 10, &num1) < 0)
> return -1;
>
> - if (kstrtou8(ver2, 10, &num2) < 0)
> + if (kstrtou8(cur2, 10, &num2) < 0)
> return -1;
>
> /* if numbers differ, then set the result */
> if (num1 < num2)
> + return -1;
> + if (num1 > num2)
> + return 1;
> +
> + /* if numbers are the same, go to next level */
> + if (!next1 && !next2)
> + break;
> + else if (!next1) {
> result = -1;
> - else if (num1 > num2)
> + } else if (!next2) {
> result = 1;
> - else {
> - /* if numbers are the same, go to next level */
> - ver1 = strchr(ver1, '.');
> - ver2 = strchr(ver2, '.');
> - if (!ver1 && !ver2)
> - break;
> - else if (!ver1)
> - result = -1;
> - else if (!ver2)
> - result = 1;
> - else {
> - ver1++;
> - ver2++;
> - }
> + } else {
> + cur1 = next1;
> + cur2 = next2;
> }
> }
> +
> return result;
> }
>
> diff --git a/tools/bpf/example b/tools/bpf/example
> new file mode 100644
> index 0000000..a0ac81f
> --- /dev/null
> +++ b/tools/bpf/example
> @@ -0,0 +1,12 @@
> + ldh [12]
> + jne #0x8100, nonvlan
> + ldh [16]
> + jne #0x88f7, bad
> + ldb [18]
> + ja test
> + nonvlan: jne #0x88f7, bad
> + ldb [14]
> + test: and #0x8
> + jeq #0, bad
> + good: ret #1500
> + bad: ret #0
Looks like this hunk and the next got included by mistake.
Thanks,
Richard
> diff --git a/tools/bpf/novlan b/tools/bpf/novlan
> new file mode 100644
> index 0000000..fe35288
> --- /dev/null
> +++ b/tools/bpf/novlan
> @@ -0,0 +1,7 @@
> + ldh [12]
> + jne #0x88f7, bad
> + ldb [14]
> + and #0x8
> + jeq #0, bad
> + good: ret #1500
> + bad: ret #0
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
2020-11-24 16:01 min.li.xe
@ 2020-11-24 23:59 ` Richard Cochran
0 siblings, 0 replies; 6+ messages in thread
From: Richard Cochran @ 2020-11-24 23:59 UTC (permalink / raw)
To: min.li.xe; +Cc: netdev, linux-kernel
On Tue, Nov 24, 2020 at 11:01:26AM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Use sscanf to get rid of adhoc string parse.
This is much nicer. Small issue remains...
> + u8 ver1[3], ver2[3];
> + int i;
> +
> + if (sscanf(version1, "%hhu.%hhu.%hhu",
> + &ver1[0], &ver1[1], &ver1[2]) < 0)
> + return -1;
The sscanf function returns the number of scanned items, and so you
should check that it returns 3 (three).
> + if (sscanf(version2, "%hhu.%hhu.%hhu",
> + &ver2[0], &ver2[1], &ver2[2]) < 0)
> + return -1;
Same here.
Thanks,
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
@ 2020-11-24 16:01 min.li.xe
2020-11-24 23:59 ` Richard Cochran
0 siblings, 1 reply; 6+ messages in thread
From: min.li.xe @ 2020-11-24 16:01 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Feed kstrtou8 with NULL terminated string.
Changes since v1:
-Use sscanf to get rid of adhoc string parse.
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_clockmatrix.c | 53 +++++++++++++++----------------------------
1 file changed, 18 insertions(+), 35 deletions(-)
diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index e020faf..12d939f 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -103,43 +103,26 @@ static int timespec_to_char_array(struct timespec64 const *ts,
return 0;
}
-static int idtcm_strverscmp(const char *ver1, const char *ver2)
+static int idtcm_strverscmp(const char *version1, const char *version2)
{
- u8 num1;
- u8 num2;
- int result = 0;
-
- /* loop through each level of the version string */
- while (result == 0) {
- /* extract leading version numbers */
- if (kstrtou8(ver1, 10, &num1) < 0)
- return -1;
-
- if (kstrtou8(ver2, 10, &num2) < 0)
- return -1;
-
- /* if numbers differ, then set the result */
- if (num1 < num2)
- result = -1;
- else if (num1 > num2)
- result = 1;
- else {
- /* if numbers are the same, go to next level */
- ver1 = strchr(ver1, '.');
- ver2 = strchr(ver2, '.');
- if (!ver1 && !ver2)
- break;
- else if (!ver1)
- result = -1;
- else if (!ver2)
- result = 1;
- else {
- ver1++;
- ver2++;
- }
- }
+ u8 ver1[3], ver2[3];
+ int i;
+
+ if (sscanf(version1, "%hhu.%hhu.%hhu",
+ &ver1[0], &ver1[1], &ver1[2]) < 0)
+ return -1;
+ if (sscanf(version2, "%hhu.%hhu.%hhu",
+ &ver2[0], &ver2[1], &ver2[2]) < 0)
+ return -1;
+
+ for (i = 0; i < 3; i++) {
+ if (ver1[i] > ver2[i])
+ return 1;
+ if (ver1[i] < ver2[i])
+ return -1;
}
- return result;
+
+ return 0;
}
static int idtcm_xfer_read(struct idtcm *idtcm,
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
2020-11-19 3:50 min.li.xe
@ 2020-11-21 21:40 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2020-11-21 21:40 UTC (permalink / raw)
To: min.li.xe; +Cc: richardcochran, netdev, linux-kernel
On Wed, 18 Nov 2020 22:50:24 -0500 min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Only strcpy 15 characters to leave 1 space for '\0'
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + strncpy(ver1, version1, 15);
> + strncpy(ver2, version2, 15);
> + cur1 = ver1;
> + cur2 = ver2;
Now there is no guarantee that the buffer is null terminated.
You need to use strscpy(ver... 16) or add ver[15] = '\0';
> /* loop through each level of the version string */
> while (result == 0) {
> + next1 = strchr(cur1, '.');
> + next2 = strchr(cur2, '.');
> +
> + /* kstrtou8 could fail for dot */
> + if (next1) {
> + *next1 = '\0';
> + next1++;
> + }
> +
> + if (next2) {
> + *next2 = '\0';
> + next2++;
> + }
> +
> /* extract leading version numbers */
> - if (kstrtou8(ver1, 10, &num1) < 0)
> + if (kstrtou8(cur1, 10, &num1) < 0)
> return -1;
>
> - if (kstrtou8(ver2, 10, &num2) < 0)
> + if (kstrtou8(cur2, 10, &num2) < 0)
> return -1;
>
> /* if numbers differ, then set the result */
> - if (num1 < num2)
> + if (num1 < num2) {
> result = -1;
Why do you set the result to something instead of just returning that
value? The kstrtou8() checks above just return value directly...
If you use a return you can save yourself all the else branches.
> - else if (num1 > num2)
> + } else if (num1 > num2) {
> result = 1;
> - else {
> + } else {
> /* if numbers are the same, go to next level */
> - ver1 = strchr(ver1, '.');
> - ver2 = strchr(ver2, '.');
> - if (!ver1 && !ver2)
> + if (!next1 && !next2)
> break;
> - else if (!ver1)
> + else if (!next1) {
> result = -1;
> - else if (!ver2)
> + } else if (!next2) {
> result = 1;
> - else {
> - ver1++;
> - ver2++;
> + } else {
> + cur1 = next1;
> + cur2 = next2;
> }
> }
> }
> +
> return result;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
@ 2020-11-19 3:50 min.li.xe
2020-11-21 21:40 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: min.li.xe @ 2020-11-19 3:50 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Feed kstrtou8 with NULL terminated string.
Changes since v1:
-Only strcpy 15 characters to leave 1 space for '\0'
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_clockmatrix.c | 52 +++++++++++++++++++++++++++++++------------
1 file changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index e020faf..efe5639 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -103,42 +103,66 @@ static int timespec_to_char_array(struct timespec64 const *ts,
return 0;
}
-static int idtcm_strverscmp(const char *ver1, const char *ver2)
+static int idtcm_strverscmp(const char *version1, const char *version2)
{
u8 num1;
u8 num2;
int result = 0;
+ char ver1[16];
+ char ver2[16];
+ char *cur1;
+ char *cur2;
+ char *next1;
+ char *next2;
+
+ strncpy(ver1, version1, 15);
+ strncpy(ver2, version2, 15);
+ cur1 = ver1;
+ cur2 = ver2;
/* loop through each level of the version string */
while (result == 0) {
+ next1 = strchr(cur1, '.');
+ next2 = strchr(cur2, '.');
+
+ /* kstrtou8 could fail for dot */
+ if (next1) {
+ *next1 = '\0';
+ next1++;
+ }
+
+ if (next2) {
+ *next2 = '\0';
+ next2++;
+ }
+
/* extract leading version numbers */
- if (kstrtou8(ver1, 10, &num1) < 0)
+ if (kstrtou8(cur1, 10, &num1) < 0)
return -1;
- if (kstrtou8(ver2, 10, &num2) < 0)
+ if (kstrtou8(cur2, 10, &num2) < 0)
return -1;
/* if numbers differ, then set the result */
- if (num1 < num2)
+ if (num1 < num2) {
result = -1;
- else if (num1 > num2)
+ } else if (num1 > num2) {
result = 1;
- else {
+ } else {
/* if numbers are the same, go to next level */
- ver1 = strchr(ver1, '.');
- ver2 = strchr(ver2, '.');
- if (!ver1 && !ver2)
+ if (!next1 && !next2)
break;
- else if (!ver1)
+ else if (!next1) {
result = -1;
- else if (!ver2)
+ } else if (!next2) {
result = 1;
- else {
- ver1++;
- ver2++;
+ } else {
+ cur1 = next1;
+ cur2 = next2;
}
}
}
+
return result;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-24 23:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-23 20:20 [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp min.li.xe
2020-11-24 1:23 ` Richard Cochran
-- strict thread matches above, loose matches on Subject: below --
2020-11-24 16:01 min.li.xe
2020-11-24 23:59 ` Richard Cochran
2020-11-19 3:50 min.li.xe
2020-11-21 21:40 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).