[PATCH] sysctl: drop the pointer dance in proc_put_char()

Bradley Morgan posted 1 patch 1 month ago
kernel/sysctl.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
[PATCH] sysctl: drop the pointer dance in proc_put_char()
Posted by Bradley Morgan 1 month ago
proc_put_char() still drags around a char **buffer alias, writing
the char through it, advancing it, then copying it back into the
slot it was loaded from. That only made sense when the buffer was
__user and the char went through put_user() (which could fail).

Since commit 32927393dc1c ("sysctl: pass kernel pointers to
->proc_handler") the buffer is just a kernel pointer, so the alias
is dead weight. proc_put_long() and the skip helpers already advance
*buf directly, so do the same here. No functional change.

Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 kernel/sysctl.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f7b7598..2b92b30 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -350,12 +350,9 @@ static void proc_put_long(void **buf, size_t *size, unsigned long val, bool neg)
 static void proc_put_char(void **buf, size_t *size, char c)
 {
 	if (*size) {
-		char **buffer = (char **)buf;
-		**buffer = c;
-
+		*(char *)*buf = c;
 		(*size)--;
-		(*buffer)++;
-		*buf = *buffer;
+		(*buf)++;
 	}
 }
 
-- 
2.47.3
Re: [PATCH] sysctl: drop the pointer dance in proc_put_char()
Posted by Joel Granados 1 week, 4 days ago
On Sat, Aug 22, 2026 at 06:42:44PM +0000, Bradley Morgan wrote:
> proc_put_char() still drags around a char **buffer alias, writing
> the char through it, advancing it, then copying it back into the
> slot it was loaded from. That only made sense when the buffer was
> __user and the char went through put_user() (which could fail).
> 
> Since commit 32927393dc1c ("sysctl: pass kernel pointers to
> ->proc_handler") the buffer is just a kernel pointer, so the alias
> is dead weight. proc_put_long() and the skip helpers already advance
> *buf directly, so do the same here. No functional change.
> 
> Signed-off-by: Bradley Morgan <include@grrlz.net>
> ---
>  kernel/sysctl.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f7b7598..2b92b30 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -350,12 +350,9 @@ static void proc_put_long(void **buf, size_t *size, unsigned long val, bool neg)
>  static void proc_put_char(void **buf, size_t *size, char c)
>  {
>  	if (*size) {
> -		char **buffer = (char **)buf;
> -		**buffer = c;
> -
> +		*(char *)*buf = c;
>  		(*size)--;
> -		(*buffer)++;
> -		*buf = *buffer;
> +		(*buf)++;
>  	}
>  }
>  
> -- 
> 2.47.3
> 

I was thinking more along these lines https://lore.kernel.org/all/20260916-lklm-sysctl-void-vs-char-v1-1-c3f8b2eb8b1e@kernel.org/

Best
Re: [PATCH] sysctl: drop the pointer dance in proc_put_char()
Posted by David Laight 1 week, 4 days ago
On Wed, 16 Sep 2026 13:46:40 +0200
Joel Granados <joel.granados@kernel.org> wrote:

> On Sat, Aug 22, 2026 at 06:42:44PM +0000, Bradley Morgan wrote:
> > proc_put_char() still drags around a char **buffer alias, writing
> > the char through it, advancing it, then copying it back into the
> > slot it was loaded from. That only made sense when the buffer was
> > __user and the char went through put_user() (which could fail).
> > 
> > Since commit 32927393dc1c ("sysctl: pass kernel pointers to  
> > ->proc_handler") the buffer is just a kernel pointer, so the alias  
> > is dead weight. proc_put_long() and the skip helpers already advance
> > *buf directly, so do the same here. No functional change.
> > 
> > Signed-off-by: Bradley Morgan <include@grrlz.net>
> > ---
> >  kernel/sysctl.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index f7b7598..2b92b30 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -350,12 +350,9 @@ static void proc_put_long(void **buf, size_t *size, unsigned long val, bool neg)
> >  static void proc_put_char(void **buf, size_t *size, char c)
> >  {
> >  	if (*size) {
> > -		char **buffer = (char **)buf;
> > -		**buffer = c;
> > -
> > +		*(char *)*buf = c;
> >  		(*size)--;
> > -		(*buffer)++;
> > -		*buf = *buffer;
> > +		(*buf)++;
> >  	}
> >  }
> >  
> > -- 
> > 2.47.3
> >   
> 
> I was thinking more along these lines https://lore.kernel.org/all/20260916-lklm-sysctl-void-vs-char-v1-1-c3f8b2eb8b1e@kernel.org/

I think I'd decrement *size before writing to buf.
Should ensure the compiler never reloads *size.
Could also be worth an __always_inline - likely to make the code smaller.

David

> 
> Best
Re: [PATCH] sysctl: drop the pointer dance in proc_put_char()
Posted by Joel Granados 2 days, 20 hours ago
On Wed, Sep 16, 2026 at 01:47:00PM +0100, David Laight wrote:
> On Wed, 16 Sep 2026 13:46:40 +0200
> Joel Granados <joel.granados@kernel.org> wrote:
> 
> > On Sat, Aug 22, 2026 at 06:42:44PM +0000, Bradley Morgan wrote:
> > > proc_put_char() still drags around a char **buffer alias, writing
> > > the char through it, advancing it, then copying it back into the
> > > slot it was loaded from. That only made sense when the buffer was
> > > __user and the char went through put_user() (which could fail).
> > > 
> > > Since commit 32927393dc1c ("sysctl: pass kernel pointers to  
> > > ->proc_handler") the buffer is just a kernel pointer, so the alias  
> > > is dead weight. proc_put_long() and the skip helpers already advance
> > > *buf directly, so do the same here. No functional change.
> > > 
> > > Signed-off-by: Bradley Morgan <include@grrlz.net>
> > > ---
> > >  kernel/sysctl.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > > index f7b7598..2b92b30 100644
> > > --- a/kernel/sysctl.c
> > > +++ b/kernel/sysctl.c
> > > @@ -350,12 +350,9 @@ static void proc_put_long(void **buf, size_t *size, unsigned long val, bool neg)
> > >  static void proc_put_char(void **buf, size_t *size, char c)
> > >  {
> > >  	if (*size) {
> > > -		char **buffer = (char **)buf;
> > > -		**buffer = c;
> > > -
> > > +		*(char *)*buf = c;
> > >  		(*size)--;
> > > -		(*buffer)++;
> > > -		*buf = *buffer;
> > > +		(*buf)++;
> > >  	}
> > >  }
> > >  
> > > -- 
> > > 2.47.3
> > >   
> > 
> > I was thinking more along these lines https://lore.kernel.org/all/20260916-lklm-sysctl-void-vs-char-v1-1-c3f8b2eb8b1e@kernel.org/
> 
> I think I'd decrement *size before writing to buf.
> Should ensure the compiler never reloads *size.
> Could also be worth an __always_inline - likely to make the code smaller.

This seems sensible. I'll update my the one in [1]

[1] https://lore.kernel.org/all/20260916-lklm-sysctl-void-vs-char-v1-1-c3f8b2eb8b1e@kernel.org/