proc: fold kmalloc() + strcpy() into kmemdup()

[PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

Posted by Alexey Dobriyan 2 months, 3 weeks ago

strcpy() will recalculate string length second time which is
unnecessary in this case.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/generic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -464,9 +464,9 @@ struct proc_dir_entry *proc_symlink(const char *name,
 			  (S_IFLNK | S_IRUGO | S_IWUGO | S_IXUGO),1);
 
 	if (ent) {
-		ent->data = kmalloc((ent->size=strlen(dest))+1, GFP_KERNEL);
+		ent->size = strlen(dest);
+		ent->data = kmemdup(dest, ent->size + 1, GFP_KERNEL);
 		if (ent->data) {
-			strcpy((char*)ent->data,dest);
 			ent->proc_iops = &proc_link_inode_operations;
 			ent = proc_register(parent, ent);
 		} else {

RE: [PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

Posted by David Laight 2 months, 3 weeks ago

From: Alexey Dobriyan
> Sent: 08 September 2024 10:28
> 
> strcpy() will recalculate string length second time which is
> unnecessary in this case.

There is also definitely scope for the string being changed.
Maybe you can prove it doesn't happen?

Which also means the code would be better explicitly writing
the terminating '\0' rather than relying on the one from the
input buffer.

	David

> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  fs/proc/generic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -464,9 +464,9 @@ struct proc_dir_entry *proc_symlink(const char *name,
>  			  (S_IFLNK | S_IRUGO | S_IWUGO | S_IXUGO),1);
> 
>  	if (ent) {
> -		ent->data = kmalloc((ent->size=strlen(dest))+1, GFP_KERNEL);
> +		ent->size = strlen(dest);
> +		ent->data = kmemdup(dest, ent->size + 1, GFP_KERNEL);
>  		if (ent->data) {
> -			strcpy((char*)ent->data,dest);
>  			ent->proc_iops = &proc_link_inode_operations;
>  			ent = proc_register(parent, ent);
>  		} else {

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

Posted by Alexey Dobriyan 2 months, 2 weeks ago

On Mon, Sep 09, 2024 at 03:13:04PM +0000, David Laight wrote:
> From: Alexey Dobriyan
> > Sent: 08 September 2024 10:28
> > 
> > strcpy() will recalculate string length second time which is
> > unnecessary in this case.
> 
> There is also definitely scope for the string being changed.
> Maybe you can prove it doesn't happen?

No, no, no. It is caller's responsibility to make sure the symlink
target stays stable for the duration of the call.

Kernel does it for strncpy_from_user() because userspace, but not here.

> Which also means the code would be better explicitly writing
> the terminating '\0' rather than relying on the one from the
> input buffer.
> 
> 	David
> 
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> > 
> >  fs/proc/generic.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -464,9 +464,9 @@ struct proc_dir_entry *proc_symlink(const char *name,
> >  			  (S_IFLNK | S_IRUGO | S_IWUGO | S_IXUGO),1);
> > 
> >  	if (ent) {
> > -		ent->data = kmalloc((ent->size=strlen(dest))+1, GFP_KERNEL);
> > +		ent->size = strlen(dest);
> > +		ent->data = kmemdup(dest, ent->size + 1, GFP_KERNEL);
> >  		if (ent->data) {
> > -			strcpy((char*)ent->data,dest);
> >  			ent->proc_iops = &proc_link_inode_operations;
> >  			ent = proc_register(parent, ent);
> >  		} else {

Re: [PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

Posted by Christian Brauner 2 months, 3 weeks ago

On Sun, 08 Sep 2024 12:27:45 +0300, Alexey Dobriyan wrote:
> strcpy() will recalculate string length second time which is
> unnecessary in this case.
> 
> 

Applied to the vfs.procfs branch of the vfs/vfs.git tree.
Patches in the vfs.procfs branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.procfs

[1/1] proc: fold kmalloc() + strcpy() into kmemdup()
      https://git.kernel.org/vfs/vfs/c/4ad5f9a021bd

Re: [PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

Posted by Markus Elfring 2 months, 3 weeks ago

> strcpy() will recalculate string length second time which is
> unnecessary in this case.

Can corresponding imperative wordings be preferred for a better change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc6#n94

Regards,
Markus