| Message ID | 90af27c1-0b86-47a6-a6c8-61a58b8aa747@p183 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | proc: fold kmalloc() + strcpy() into kmemdup() | expand |
> 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
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
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)
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 {
--- 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 {
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(-)