Re: [PATCH 11/13] proc: readdir /proc/*/task
On Wed, Aug 29, 2018 at 04:43:30PM -0700, Andrew Morton wrote: > On Tue, 28 Aug 2018 22:35:02 +0300 Alexey Dobriyan > wrote: > > > > Are we really in a super hot path to justify all that? > > > > /proc is very slow, try profiling just about anything involving /proc. > > So how much does this patchset help? Some timing measurements would > really help things along, if they show goodness. Benchmarking results are in individual patches. They are ~1-7-20% better overall. However those are microbenchmarks.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Wed, Aug 29, 2018 at 04:43:30PM -0700, Andrew Morton wrote: > On Tue, 28 Aug 2018 22:35:02 +0300 Alexey Dobriyan > wrote: > > > > Are we really in a super hot path to justify all that? > > > > /proc is very slow, try profiling just about anything involving /proc. > > So how much does this patchset help? Some timing measurements would > really help things along, if they show goodness. Benchmarking results are in individual patches. They are ~1-7-20% better overall. However those are microbenchmarks.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, 28 Aug 2018 22:35:02 +0300 Alexey Dobriyan wrote: > > Are we really in a super hot path to justify all that? > > /proc is very slow, try profiling just about anything involving /proc. So how much does this patchset help? Some timing measurements would really help things along, if they show goodness.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, 28 Aug 2018 22:35:02 +0300 Alexey Dobriyan wrote: > > Are we really in a super hot path to justify all that? > > /proc is very slow, try profiling just about anything involving /proc. So how much does this patchset help? Some timing measurements would really help things along, if they show goodness.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 01:04:40PM +, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 12:36:22PM +, Ahmed S. Darwish wrote: > > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > > --- > > > fs/proc/base.c | 8 > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > Missing description and S-o-b. Further comments below.. > > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > > index 33f444721965..668e465c86b3 100644 > > > --- a/fs/proc/base.c > > > +++ b/fs/proc/base.c > > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > > > struct dir_context *ctx) > > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > >task; > > >task = next_tid(task), ctx->pos++) { > > > - char name[10 + 1]; > > > - unsigned int len; > > > + char name[10], *p = name + sizeof(name); > > > + > > > > Multiple issues: > > > > - len should be 11, as was in the original code > > (0x = 4294967295, 10 letters) > > > > - while we're at it, let's use a constant for the '11' instead of > > mysterious magic numbers > > > > - 'p' is clearly overflowing the stack here > > > > See below: > > > > tid = task_pid_nr_ns(task, ns); > > > - len = snprintf(name, sizeof(name), "%u", tid); > > > - if (!proc_fill_cache(file, ctx, name, len, > > > + p = _print_integer_u32(p, tid); > > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > > > You're replacing snprintf() code __that did proper len checking__ > > with code that does not. That's not good. > > > > I can't see how the fourth proc_fill_cache() parameter, ``name + > > sizeof(name)'' safely ever replace the original 'len' parameter. > > It's a pointer value .. (!) > > > > Ok, there's a "- p" in the end, so the length looks to be Ok. > > Nonetheless, the whole patch series is introducing funny code > like: > > +/* > + * Print an integer in decimal. > + * "p" initially points PAST THE END OF THE BUFFER! > + * > + * DO NOT USE THESE FUNCTIONS! > + * > + * Do not copy these functions. > + * Do not document these functions. > + * Do not move these functions to lib/ or elsewhere. > + * Do not export these functions to modules. > + * Do not tell anyone about these functions. > + */ > +noinline > +char *_print_integer_u32(char *p, u32 x) > +{ > + do { > + *--p = '0' + (x % 10); > + x /= 10; > + } while (x != 0); > + return p; > +} > > And thus the code using these functions is throwing invalid > past-the-stack pointers and strings with no NULL terminators > like there's no tomorrow... > > IMHO It's an accident waiting to happen to sprinkle pointers > like that everywhere. It is not if people will be prohibited from moving this code to lib/ and "improving" it by adding more parameters. > Are we really in a super hot path to justify all that? /proc is very slow, try profiling just about anything involving /proc.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 01:04:40PM +, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 12:36:22PM +, Ahmed S. Darwish wrote: > > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > > --- > > > fs/proc/base.c | 8 > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > Missing description and S-o-b. Further comments below.. > > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > > index 33f444721965..668e465c86b3 100644 > > > --- a/fs/proc/base.c > > > +++ b/fs/proc/base.c > > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > > > struct dir_context *ctx) > > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > >task; > > >task = next_tid(task), ctx->pos++) { > > > - char name[10 + 1]; > > > - unsigned int len; > > > + char name[10], *p = name + sizeof(name); > > > + > > > > Multiple issues: > > > > - len should be 11, as was in the original code > > (0x = 4294967295, 10 letters) > > > > - while we're at it, let's use a constant for the '11' instead of > > mysterious magic numbers > > > > - 'p' is clearly overflowing the stack here > > > > See below: > > > > tid = task_pid_nr_ns(task, ns); > > > - len = snprintf(name, sizeof(name), "%u", tid); > > > - if (!proc_fill_cache(file, ctx, name, len, > > > + p = _print_integer_u32(p, tid); > > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > > > You're replacing snprintf() code __that did proper len checking__ > > with code that does not. That's not good. > > > > I can't see how the fourth proc_fill_cache() parameter, ``name + > > sizeof(name)'' safely ever replace the original 'len' parameter. > > It's a pointer value .. (!) > > > > Ok, there's a "- p" in the end, so the length looks to be Ok. > > Nonetheless, the whole patch series is introducing funny code > like: > > +/* > + * Print an integer in decimal. > + * "p" initially points PAST THE END OF THE BUFFER! > + * > + * DO NOT USE THESE FUNCTIONS! > + * > + * Do not copy these functions. > + * Do not document these functions. > + * Do not move these functions to lib/ or elsewhere. > + * Do not export these functions to modules. > + * Do not tell anyone about these functions. > + */ > +noinline > +char *_print_integer_u32(char *p, u32 x) > +{ > + do { > + *--p = '0' + (x % 10); > + x /= 10; > + } while (x != 0); > + return p; > +} > > And thus the code using these functions is throwing invalid > past-the-stack pointers and strings with no NULL terminators > like there's no tomorrow... > > IMHO It's an accident waiting to happen to sprinkle pointers > like that everywhere. It is not if people will be prohibited from moving this code to lib/ and "improving" it by adding more parameters. > Are we really in a super hot path to justify all that? /proc is very slow, try profiling just about anything involving /proc.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 12:36:22PM +, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > --- > > fs/proc/base.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Missing description and S-o-b. Further comments below.. > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 33f444721965..668e465c86b3 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > > struct dir_context *ctx) > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > task; > > task = next_tid(task), ctx->pos++) { > > - char name[10 + 1]; > > - unsigned int len; > > + char name[10], *p = name + sizeof(name); > > + > > Multiple issues: > > - len should be 11, as was in the original code > (0x = 4294967295, 10 letters) len should be 10. > - while we're at it, let's use a constant for the '11' instead of > mysterious magic numbers > > - 'p' is clearly overflowing the stack here > > > tid = task_pid_nr_ns(task, ns); > > - len = snprintf(name, sizeof(name), "%u", tid); > > - if (!proc_fill_cache(file, ctx, name, len, > > + p = _print_integer_u32(p, tid); > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > You're replacing snprintf() code __that did proper len checking__ > with code that does not. That's not good. Yes, the whole point of the patch is to skip length checking. > I can't see how the fourth proc_fill_cache() parameter, ``name + > sizeof(name)'' safely ever replace the original 'len' parameter. > It's a pointer value .. (!) > > Overall this looks like a broken patch submitted by mistake.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 12:36:22PM +, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > --- > > fs/proc/base.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Missing description and S-o-b. Further comments below.. > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 33f444721965..668e465c86b3 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > > struct dir_context *ctx) > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > task; > > task = next_tid(task), ctx->pos++) { > > - char name[10 + 1]; > > - unsigned int len; > > + char name[10], *p = name + sizeof(name); > > + > > Multiple issues: > > - len should be 11, as was in the original code > (0x = 4294967295, 10 letters) len should be 10. > - while we're at it, let's use a constant for the '11' instead of > mysterious magic numbers > > - 'p' is clearly overflowing the stack here > > > tid = task_pid_nr_ns(task, ns); > > - len = snprintf(name, sizeof(name), "%u", tid); > > - if (!proc_fill_cache(file, ctx, name, len, > > + p = _print_integer_u32(p, tid); > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > You're replacing snprintf() code __that did proper len checking__ > with code that does not. That's not good. Yes, the whole point of the patch is to skip length checking. > I can't see how the fourth proc_fill_cache() parameter, ``name + > sizeof(name)'' safely ever replace the original 'len' parameter. > It's a pointer value .. (!) > > Overall this looks like a broken patch submitted by mistake.
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 12:36:22PM +, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > --- > > fs/proc/base.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Missing description and S-o-b. Further comments below.. > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 33f444721965..668e465c86b3 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > > struct dir_context *ctx) > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > task; > > task = next_tid(task), ctx->pos++) { > > - char name[10 + 1]; > > - unsigned int len; > > + char name[10], *p = name + sizeof(name); > > + > > Multiple issues: > > - len should be 11, as was in the original code > (0x = 4294967295, 10 letters) > > - while we're at it, let's use a constant for the '11' instead of > mysterious magic numbers > > - 'p' is clearly overflowing the stack here > See below: > > tid = task_pid_nr_ns(task, ns); > > - len = snprintf(name, sizeof(name), "%u", tid); > > - if (!proc_fill_cache(file, ctx, name, len, > > + p = _print_integer_u32(p, tid); > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > You're replacing snprintf() code __that did proper len checking__ > with code that does not. That's not good. > > I can't see how the fourth proc_fill_cache() parameter, ``name + > sizeof(name)'' safely ever replace the original 'len' parameter. > It's a pointer value .. (!) > Ok, there's a "- p" in the end, so the length looks to be Ok. Nonetheless, the whole patch series is introducing funny code like: +/* + * Print an integer in decimal. + * "p" initially points PAST THE END OF THE BUFFER! + * + * DO NOT USE THESE FUNCTIONS! + * + * Do not copy these functions. + * Do not document these functions. + * Do not move these functions to lib/ or elsewhere. + * Do not export these functions to modules. + * Do not tell anyone about these functions. + */ +noinline +char *_print_integer_u32(char *p, u32 x) +{ + do { + *--p = '0' + (x % 10); + x /= 10; + } while (x != 0); + return p; +} And thus the code using these functions is throwing invalid past-the-stack pointers and strings with no NULL terminators like there's no tomorrow... IMHO It's an accident waiting to happen to sprinkle pointers like that everywhere. Are we really in a super hot path to justify all that? /me confused -- Darwish http://darwish.chasingpointers.com
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 12:36:22PM +, Ahmed S. Darwish wrote: > On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > > --- > > fs/proc/base.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Missing description and S-o-b. Further comments below.. > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 33f444721965..668e465c86b3 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > > struct dir_context *ctx) > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > task; > > task = next_tid(task), ctx->pos++) { > > - char name[10 + 1]; > > - unsigned int len; > > + char name[10], *p = name + sizeof(name); > > + > > Multiple issues: > > - len should be 11, as was in the original code > (0x = 4294967295, 10 letters) > > - while we're at it, let's use a constant for the '11' instead of > mysterious magic numbers > > - 'p' is clearly overflowing the stack here > See below: > > tid = task_pid_nr_ns(task, ns); > > - len = snprintf(name, sizeof(name), "%u", tid); > > - if (!proc_fill_cache(file, ctx, name, len, > > + p = _print_integer_u32(p, tid); > > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, > > You're replacing snprintf() code __that did proper len checking__ > with code that does not. That's not good. > > I can't see how the fourth proc_fill_cache() parameter, ``name + > sizeof(name)'' safely ever replace the original 'len' parameter. > It's a pointer value .. (!) > Ok, there's a "- p" in the end, so the length looks to be Ok. Nonetheless, the whole patch series is introducing funny code like: +/* + * Print an integer in decimal. + * "p" initially points PAST THE END OF THE BUFFER! + * + * DO NOT USE THESE FUNCTIONS! + * + * Do not copy these functions. + * Do not document these functions. + * Do not move these functions to lib/ or elsewhere. + * Do not export these functions to modules. + * Do not tell anyone about these functions. + */ +noinline +char *_print_integer_u32(char *p, u32 x) +{ + do { + *--p = '0' + (x % 10); + x /= 10; + } while (x != 0); + return p; +} And thus the code using these functions is throwing invalid past-the-stack pointers and strings with no NULL terminators like there's no tomorrow... IMHO It's an accident waiting to happen to sprinkle pointers like that everywhere. Are we really in a super hot path to justify all that? /me confused -- Darwish http://darwish.chasingpointers.com
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > --- > fs/proc/base.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > Missing description and S-o-b. Further comments below.. > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 33f444721965..668e465c86b3 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > struct dir_context *ctx) > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); >task; >task = next_tid(task), ctx->pos++) { > - char name[10 + 1]; > - unsigned int len; > + char name[10], *p = name + sizeof(name); > + Multiple issues: - len should be 11, as was in the original code (0x = 4294967295, 10 letters) - while we're at it, let's use a constant for the '11' instead of mysterious magic numbers - 'p' is clearly overflowing the stack here > tid = task_pid_nr_ns(task, ns); > - len = snprintf(name, sizeof(name), "%u", tid); > - if (!proc_fill_cache(file, ctx, name, len, > + p = _print_integer_u32(p, tid); > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, You're replacing snprintf() code __that did proper len checking__ with code that does not. That's not good. I can't see how the fourth proc_fill_cache() parameter, ``name + sizeof(name)'' safely ever replace the original 'len' parameter. It's a pointer value .. (!) Overall this looks like a broken patch submitted by mistake. Thanks, -- Darwish http://darwish.chasingpointers.com
Re: [PATCH 11/13] proc: readdir /proc/*/task
On Tue, Aug 28, 2018 at 02:15:01AM +0300, Alexey Dobriyan wrote: > --- > fs/proc/base.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > Missing description and S-o-b. Further comments below.. > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 33f444721965..668e465c86b3 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -3549,11 +3549,11 @@ static int proc_task_readdir(struct file *file, > struct dir_context *ctx) > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); >task; >task = next_tid(task), ctx->pos++) { > - char name[10 + 1]; > - unsigned int len; > + char name[10], *p = name + sizeof(name); > + Multiple issues: - len should be 11, as was in the original code (0x = 4294967295, 10 letters) - while we're at it, let's use a constant for the '11' instead of mysterious magic numbers - 'p' is clearly overflowing the stack here > tid = task_pid_nr_ns(task, ns); > - len = snprintf(name, sizeof(name), "%u", tid); > - if (!proc_fill_cache(file, ctx, name, len, > + p = _print_integer_u32(p, tid); > + if (!proc_fill_cache(file, ctx, p, name + sizeof(name) - p, You're replacing snprintf() code __that did proper len checking__ with code that does not. That's not good. I can't see how the fourth proc_fill_cache() parameter, ``name + sizeof(name)'' safely ever replace the original 'len' parameter. It's a pointer value .. (!) Overall this looks like a broken patch submitted by mistake. Thanks, -- Darwish http://darwish.chasingpointers.com