| Message ID | 20230107034557.1156022-1-quanfafu@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tracing/eprobe: Replace kzalloc with kmalloc | expand |
Le 07/01/2023 à 04:45, Quanfa Fu a écrit : > Since this memory will be filled soon below, I feel that there is > no need for a memory of all zeros here. 'snprintf' does not return > negative num according to ISO C99, so I feel that no judgment is > needed here. > > No functional change intended. > > Signed-off-by: Quanfa Fu <quanfafu@gmail.com> > --- > kernel/trace/trace_eprobe.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > index 352b65e2b910..cd1d271a74e7 100644 > --- a/kernel/trace/trace_eprobe.c > +++ b/kernel/trace/trace_eprobe.c > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch > for (i = 0; i < argc; i++) > len += strlen(argv[i]) + 1; > > - ep->filter_str = kzalloc(len, GFP_KERNEL); > + ep->filter_str = kmalloc(len, GFP_KERNEL); > if (!ep->filter_str) > return -ENOMEM; > > p = ep->filter_str; > for (i = 0; i < argc; i++) { > ret = snprintf(p, len, "%s ", argv[i]); > - if (ret < 0) > - goto error; > if (ret > len) { Hi, as per [1]: * The return value is the number of characters which would be * generated for the given input, excluding the trailing null, * as per ISO C99. If the return is greater than *or equal* to * @size, the resulting string is truncated. So, should this test be: if (ret >= len) ~~~~ Also, isn't the p[-1] = '\0' after the loop eating the last character? argc = 1; argv[0] = "a"; Before the loop: =============== len = 1 + 1 = 2; ep->filter_str = 0x00 0x00 ^ |___ p After the loop: =============== ep->filter_str = 0x61 0x00 ^ |___ p len = 1; After p[-1]: ============ ep->filter_str = 0x00 0x00 ~~ ^ |___ p Did I miss something obvious? I don't know the intent here, or if it is an issue at all, but it looks odd. CJ [1]: https://elixir.bootlin.com/linux/v6.2-rc1/source/lib/vsprintf.c#L2925 > ret = -E2BIG; > goto error;
On Sat, Jan 7, 2023 at 4:42 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 07/01/2023 à 04:45, Quanfa Fu a écrit : > > Since this memory will be filled soon below, I feel that there is > > no need for a memory of all zeros here. 'snprintf' does not return > > negative num according to ISO C99, so I feel that no judgment is > > needed here. > > > > No functional change intended. > > > > Signed-off-by: Quanfa Fu <quanfafu@gmail.com> > > --- > > kernel/trace/trace_eprobe.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > > index 352b65e2b910..cd1d271a74e7 100644 > > --- a/kernel/trace/trace_eprobe.c > > +++ b/kernel/trace/trace_eprobe.c > > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch > > for (i = 0; i < argc; i++) > > len += strlen(argv[i]) + 1; > > > > - ep->filter_str = kzalloc(len, GFP_KERNEL); > > + ep->filter_str = kmalloc(len, GFP_KERNEL); > > if (!ep->filter_str) > > return -ENOMEM; > > > > p = ep->filter_str; > > for (i = 0; i < argc; i++) { > > ret = snprintf(p, len, "%s ", argv[i]); > > - if (ret < 0) > > - goto error; > > if (ret > len) { > > Hi, > > as per [1]: > * The return value is the number of characters which would be > * generated for the given input, excluding the trailing null, > * as per ISO C99. If the return is greater than *or equal* to > * @size, the resulting string is truncated. > > So, should this test be: > if (ret >= len) > ~~~~ > > > Also, isn't the p[-1] = '\0' after the loop eating the last character? > argc = 1; > argv[0] = "a"; > > Before the loop: > =============== > len = 1 + 1 = 2; > ep->filter_str = 0x00 0x00 > ^ > |___ p > > After the loop: > =============== > ep->filter_str = 0x61 0x00 > ^ > |___ p > len = 1; > > After p[-1]: > ============ > ep->filter_str = 0x00 0x00 > ~~ ^ > |___ p > > Did I miss something obvious? > I don't know the intent here, or if it is an issue at all, but it looks odd. > > CJ > > > [1]: https://elixir.bootlin.com/linux/v6.2-rc1/source/lib/vsprintf.c#L2925 > > ret = -E2BIG; > > goto error; > I think that: for example, argc = 2, argv = {"a", "b"}; Before the loop =============== len = (1 + 1) + (1 + 1) = 4; ep->filter_str = 0x00 0x00 0x00 0x00 ^ |__ p After the loop: =============== i = 1, snprintf write: 'a' and ' ', so ret = 2 i = 2, snprintf write: 'b' and ' ', so ret = 2 =============== ep->filter_str = 0x61 0x20 0x62 0x00 // Since the length of the last argv is not enough to write, the space is replaced by null ^ |__ p p = ep->fiter_str + 2 (ret1) +2 (ret2) = ep->filter_str + 4 =============== so After p[-1] = *(ep->filter_str + 3) = '\0'; ep->filter_str = 0x61 0x20 0x62 0x00 According to ISO C99: " If the output was truncated due to this limit hen the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available" The last snprintf will end with 'NULL', so I think p[-1] = '\0' can also be deleted
On Sat, Jan 7, 2023 at 4:42 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 07/01/2023 à 04:45, Quanfa Fu a écrit : > > Since this memory will be filled soon below, I feel that there is > > no need for a memory of all zeros here. 'snprintf' does not return > > negative num according to ISO C99, so I feel that no judgment is > > needed here. > > > > No functional change intended. > > > > Signed-off-by: Quanfa Fu <quanfafu@gmail.com> > > --- > > kernel/trace/trace_eprobe.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > > index 352b65e2b910..cd1d271a74e7 100644 > > --- a/kernel/trace/trace_eprobe.c > > +++ b/kernel/trace/trace_eprobe.c > > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch > > for (i = 0; i < argc; i++) > > len += strlen(argv[i]) + 1; > > > > - ep->filter_str = kzalloc(len, GFP_KERNEL); > > + ep->filter_str = kmalloc(len, GFP_KERNEL); > > if (!ep->filter_str) > > return -ENOMEM; > > > > p = ep->filter_str; > > for (i = 0; i < argc; i++) { > > ret = snprintf(p, len, "%s ", argv[i]); > > - if (ret < 0) > > - goto error; > > if (ret > len) { > > Hi, > > as per [1]: > * The return value is the number of characters which would be > * generated for the given input, excluding the trailing null, > * as per ISO C99. If the return is greater than *or equal* to > * @size, the resulting string is truncated. > > So, should this test be: > if (ret >= len) > ~~~~ > > > Also, isn't the p[-1] = '\0' after the loop eating the last character? > argc = 1; > argv[0] = "a"; > > Before the loop: > =============== > len = 1 + 1 = 2; > ep->filter_str = 0x00 0x00 > ^ > |___ p > > After the loop: > =============== > ep->filter_str = 0x61 0x00 > ^ > |___ p > len = 1; > > After p[-1]: > ============ > ep->filter_str = 0x00 0x00 > ~~ ^ > |___ p > > Did I miss something obvious? > I don't know the intent here, or if it is an issue at all, but it looks odd. > > CJ > > > [1]: https://elixir.bootlin.com/linux/v6.2-rc1/source/lib/vsprintf.c#L2925 > > ret = -E2BIG; > > goto error; > I think that: for example, argc = 2, argv = {"a", "b"}; Before the loop =============== len = (1 + 1) + (1 + 1) = 4; ep->filter_str = 0x00 0x00 0x00 0x00 ^ |__ p After the loop: =============== i = 1, snprintf write: 'a' and ' ', so ret = 2 i = 2, snprintf write: 'b' and ' ', so ret = 2 =============== Since the length of the last argv is not enough to write, the space is replaced by null ep->filter_str = 0x61 0x20 0x62 0x00 ^ |__ p p = ep->fiter_str + 2 (ret1) +2 (ret2) = ep->filter_str + 4 =============== so After p[-1] = *(ep->filter_str + 3) = '\0'; ep->filter_str = 0x61 0x20 0x62 0x00 According to ISO C99: " If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available" The last snprintf will end with 'NULL', so I think p[-1] = '\0' can also be deleted
Le 07/01/2023 à 13:23, Quanfa Fu a écrit : > On Sat, Jan 7, 2023 at 4:42 PM Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> Le 07/01/2023 à 04:45, Quanfa Fu a écrit : >>> Since this memory will be filled soon below, I feel that there is >>> no need for a memory of all zeros here. 'snprintf' does not return >>> negative num according to ISO C99, so I feel that no judgment is >>> needed here. >>> >>> No functional change intended. >>> >>> Signed-off-by: Quanfa Fu <quanfafu@gmail.com> >>> --- >>> kernel/trace/trace_eprobe.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c >>> index 352b65e2b910..cd1d271a74e7 100644 >>> --- a/kernel/trace/trace_eprobe.c >>> +++ b/kernel/trace/trace_eprobe.c >>> @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch >>> for (i = 0; i < argc; i++) >>> len += strlen(argv[i]) + 1; >>> >>> - ep->filter_str = kzalloc(len, GFP_KERNEL); >>> + ep->filter_str = kmalloc(len, GFP_KERNEL); >>> if (!ep->filter_str) >>> return -ENOMEM; >>> >>> p = ep->filter_str; >>> for (i = 0; i < argc; i++) { >>> ret = snprintf(p, len, "%s ", argv[i]); >>> - if (ret < 0) >>> - goto error; >>> if (ret > len) { >> > > I think that: > > for example, argc = 2, argv = {"a", "b"}; > > Before the loop > =============== > len = (1 + 1) + (1 + 1) = 4; > ep->filter_str = 0x00 0x00 0x00 0x00 > ^ > |__ p > > After the loop: > =============== > i = 1, snprintf write: 'a' and ' ', so ret = 2 > i = 2, snprintf write: 'b' and ' ', so ret = 2 > =============== Ok, I missed the space after the %s. Sorry for the noise. > Since the length of the last argv is not enough > to write, the space is replaced by null > > ep->filter_str = 0x61 0x20 0x62 0x00 > ^ > |__ p > p = ep->fiter_str + 2 (ret1) +2 (ret2) = ep->filter_str + 4 > =============== > so After p[-1] = *(ep->filter_str + 3) = '\0'; > ep->filter_str = 0x61 0x20 0x62 0x00 > > According to ISO C99: " If the output was truncated due to this limit > then the return value is the number of characters (excluding the > terminating null byte) which would have been written to the final > string if enough space had been available" > > The last snprintf will end with 'NULL', so I think p[-1] = '\0' can > also be deleted Hmm, now that I see it, I think that it is there to remove the last space (even if there shouldn't be any because the last snprintf will be truncated). Code LGTM as-is, even if puzzling. CJ >
On Sat, 7 Jan 2023 11:45:57 +0800 Quanfa Fu <quanfafu@gmail.com> wrote: > Since this memory will be filled soon below, I feel that there is > no need for a memory of all zeros here. 'snprintf' does not return > negative num according to ISO C99, so I feel that no judgment is > needed here. > > No functional change intended. > Ah, good catch. I didn't notice that snprintf() doesn't return error code. (I confirmed that the linux internal snprintf() also doesn't return the error code) Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you! > Signed-off-by: Quanfa Fu <quanfafu@gmail.com> > --- > kernel/trace/trace_eprobe.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > index 352b65e2b910..cd1d271a74e7 100644 > --- a/kernel/trace/trace_eprobe.c > +++ b/kernel/trace/trace_eprobe.c > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch > for (i = 0; i < argc; i++) > len += strlen(argv[i]) + 1; > > - ep->filter_str = kzalloc(len, GFP_KERNEL); > + ep->filter_str = kmalloc(len, GFP_KERNEL); > if (!ep->filter_str) > return -ENOMEM; > > p = ep->filter_str; > for (i = 0; i < argc; i++) { > ret = snprintf(p, len, "%s ", argv[i]); > - if (ret < 0) > - goto error; > if (ret > len) { > ret = -E2BIG; > goto error; > -- > 2.31.1 >
On Sun, 8 Jan 2023 01:01:24 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > Ah, good catch. I didn't notice that snprintf() doesn't return > error code. (I confirmed that the linux internal snprintf() also > doesn't return the error code) > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> I replied to to the first instance of this patch ;-) https://lore.kernel.org/all/20230108162222.146d136f@rorschach.local.home/ -- Steve
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c index 352b65e2b910..cd1d271a74e7 100644 --- a/kernel/trace/trace_eprobe.c +++ b/kernel/trace/trace_eprobe.c @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch for (i = 0; i < argc; i++) len += strlen(argv[i]) + 1; - ep->filter_str = kzalloc(len, GFP_KERNEL); + ep->filter_str = kmalloc(len, GFP_KERNEL); if (!ep->filter_str) return -ENOMEM; p = ep->filter_str; for (i = 0; i < argc; i++) { ret = snprintf(p, len, "%s ", argv[i]); - if (ret < 0) - goto error; if (ret > len) { ret = -E2BIG; goto error;
Since this memory will be filled soon below, I feel that there is no need for a memory of all zeros here. 'snprintf' does not return negative num according to ISO C99, so I feel that no judgment is needed here. No functional change intended. Signed-off-by: Quanfa Fu <quanfafu@gmail.com> --- kernel/trace/trace_eprobe.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)