[PATCH v2] NFSD: Remove all occurences of strlcpy

[PATCH v2] NFSD: Remove all occurences of strlcpy

[Azeem Shaikh]

strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy here.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
v1: https://lore.kernel.org/all/20230510220952.3507366-1-azeemshaikh38@gmail.com/

Changes in v2:
 - Use __assign_str instead of strscpy.
 - Added the Fixes tag.

 fs/nfsd/trace.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 4183819ea082..72a906a053dc 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -1365,19 +1365,19 @@ TRACE_EVENT(nfsd_cb_setup,
 		__field(u32, cl_id)
 		__field(unsigned long, authflavor)
 		__sockaddr(addr, clp->cl_cb_conn.cb_addrlen)
-		__array(unsigned char, netid, 8)
+		__string(netid, netid)
 	),
 	TP_fast_assign(
 		__entry->cl_boot = clp->cl_clientid.cl_boot;
 		__entry->cl_id = clp->cl_clientid.cl_id;
-		strlcpy(__entry->netid, netid, sizeof(__entry->netid));
+		__assign_str(netid, netid);
 		__entry->authflavor = authflavor;
 		__assign_sockaddr(addr, &clp->cl_cb_conn.cb_addr,
 				  clp->cl_cb_conn.cb_addrlen)
 	),
 	TP_printk("addr=%pISpc client %08x:%08x proto=%s flavor=%s",
 		__get_sockaddr(addr), __entry->cl_boot, __entry->cl_id,
-		__entry->netid, show_nfsd_authflavor(__entry->authflavor))
+		__get_str(netid), show_nfsd_authflavor(__entry->authflavor))
 );
 
 TRACE_EVENT(nfsd_cb_setup_err,
-- 
2.40.1.606.ga4b1b128d6-goog

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Chuck Lever III]

> On May 12, 2023, at 7:53 AM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote:
> 
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy here.

Let's update the patch description. This change is really
a clean up -- it doesn't address the memory issues you
originally described.


> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
> v1: https://lore.kernel.org/all/20230510220952.3507366-1-azeemshaikh38@gmail.com/
> 
> Changes in v2:
> - Use __assign_str instead of strscpy.
> - Added the Fixes tag.
> 
> fs/nfsd/trace.h |    6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 4183819ea082..72a906a053dc 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -1365,19 +1365,19 @@ TRACE_EVENT(nfsd_cb_setup,
> __field(u32, cl_id)
> __field(unsigned long, authflavor)
> __sockaddr(addr, clp->cl_cb_conn.cb_addrlen)
> - __array(unsigned char, netid, 8)
> + __string(netid, netid)
> ),
> TP_fast_assign(
> __entry->cl_boot = clp->cl_clientid.cl_boot;
> __entry->cl_id = clp->cl_clientid.cl_id;
> - strlcpy(__entry->netid, netid, sizeof(__entry->netid));
> + __assign_str(netid, netid);
> __entry->authflavor = authflavor;
> __assign_sockaddr(addr, &clp->cl_cb_conn.cb_addr,
>  clp->cl_cb_conn.cb_addrlen)
> ),
> TP_printk("addr=%pISpc client %08x:%08x proto=%s flavor=%s",
> __get_sockaddr(addr), __entry->cl_boot, __entry->cl_id,
> - __entry->netid, show_nfsd_authflavor(__entry->authflavor))
> + __get_str(netid), show_nfsd_authflavor(__entry->authflavor))
> );
> 
> TRACE_EVENT(nfsd_cb_setup_err,
> -- 
> 2.40.1.606.ga4b1b128d6-goog
> 
> 

--
Chuck Lever

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Chuck Lever III]

> On May 12, 2023, at 8:09 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
> 
> 
>> On May 12, 2023, at 7:53 AM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote:
>> 
>> strlcpy() reads the entire source buffer first.
>> This read may exceed the destination size limit.
>> This is both inefficient and can lead to linear read
>> overflows if a source string is not NUL-terminated [1].
>> In an effort to remove strlcpy() completely [2], replace
>> strlcpy here.
> 
> Let's update the patch description. This change is really
> a clean up -- it doesn't address the memory issues you
> originally described.

Unless, of course, you intend to apply this patch /after/
a patch that fixes __assign_str(). In that case, no change
to the patch description is needed.

Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>


>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
>> [2] https://github.com/KSPP/linux/issues/89
>> 
>> Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
>> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
>> ---
>> v1: https://lore.kernel.org/all/20230510220952.3507366-1-azeemshaikh38@gmail.com/
>> 
>> Changes in v2:
>> - Use __assign_str instead of strscpy.
>> - Added the Fixes tag.
>> 
>> fs/nfsd/trace.h |    6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>> index 4183819ea082..72a906a053dc 100644
>> --- a/fs/nfsd/trace.h
>> +++ b/fs/nfsd/trace.h
>> @@ -1365,19 +1365,19 @@ TRACE_EVENT(nfsd_cb_setup,
>> __field(u32, cl_id)
>> __field(unsigned long, authflavor)
>> __sockaddr(addr, clp->cl_cb_conn.cb_addrlen)
>> - __array(unsigned char, netid, 8)
>> + __string(netid, netid)
>> ),
>> TP_fast_assign(
>> __entry->cl_boot = clp->cl_clientid.cl_boot;
>> __entry->cl_id = clp->cl_clientid.cl_id;
>> - strlcpy(__entry->netid, netid, sizeof(__entry->netid));
>> + __assign_str(netid, netid);
>> __entry->authflavor = authflavor;
>> __assign_sockaddr(addr, &clp->cl_cb_conn.cb_addr,
>> clp->cl_cb_conn.cb_addrlen)
>> ),
>> TP_printk("addr=%pISpc client %08x:%08x proto=%s flavor=%s",
>> __get_sockaddr(addr), __entry->cl_boot, __entry->cl_id,
>> - __entry->netid, show_nfsd_authflavor(__entry->authflavor))
>> + __get_str(netid), show_nfsd_authflavor(__entry->authflavor))
>> );
>> 
>> TRACE_EVENT(nfsd_cb_setup_err,
>> -- 
>> 2.40.1.606.ga4b1b128d6-goog
>> 
>> 
> 
> --
> Chuck Lever


--
Chuck Lever

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Azeem Shaikh]

Resending as plain text.

> >> strlcpy() reads the entire source buffer first.
> >> This read may exceed the destination size limit.
> >> This is both inefficient and can lead to linear read
> >> overflows if a source string is not NUL-terminated [1].
> >> In an effort to remove strlcpy() completely [2], replace
> >> strlcpy here.
> >
> > Let's update the patch description. This change is really
> > a clean up -- it doesn't address the memory issues you
> > originally described.
>
> Unless, of course, you intend to apply this patch /after/
> a patch that fixes __assign_str(). In that case, no change
> to the patch description is needed.

No, I plan to land this patch before attempting to fix __assign_str itself.
Let me know if the below description looks good to you and I'll send
over a v3 patch:

[PATCH v3] NFSD: Remove open coding of string copy

Instead of open coding a __dynamic_array(), use the __string() and
__assign_str()
helper macros that exist for this kind of use case.

Part of an effort to remove deprecated strlcpy() [1] completely from the
kernel[2].

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Chuck Lever III]

> On May 12, 2023, at 1:34 PM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote:
> 
> Resending as plain text.
> 
>>>> strlcpy() reads the entire source buffer first.
>>>> This read may exceed the destination size limit.
>>>> This is both inefficient and can lead to linear read
>>>> overflows if a source string is not NUL-terminated [1].
>>>> In an effort to remove strlcpy() completely [2], replace
>>>> strlcpy here.
>>> 
>>> Let's update the patch description. This change is really
>>> a clean up -- it doesn't address the memory issues you
>>> originally described.
>> 
>> Unless, of course, you intend to apply this patch /after/
>> a patch that fixes __assign_str(). In that case, no change
>> to the patch description is needed.
> 
> No, I plan to land this patch before attempting to fix __assign_str itself.
> Let me know if the below description looks good to you and I'll send
> over a v3 patch:
> 
> [PATCH v3] NFSD: Remove open coding of string copy
> 
> Instead of open coding a __dynamic_array(), use the __string() and
> __assign_str()
> helper macros that exist for this kind of use case.
> 
> Part of an effort to remove deprecated strlcpy() [1] completely from the
> kernel[2].
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>

This looks good to me. So you'd like me to take this through
the nfsd tree, possibly for 6.4-rc ?


--
Chuck Lever

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Azeem Shaikh]

> > No, I plan to land this patch before attempting to fix __assign_str itself.
> > Let me know if the below description looks good to you and I'll send
> > over a v3 patch:
> >
> > [PATCH v3] NFSD: Remove open coding of string copy
> >
> > Instead of open coding a __dynamic_array(), use the __string() and
> > __assign_str()
> > helper macros that exist for this kind of use case.
> >
> > Part of an effort to remove deprecated strlcpy() [1] completely from the
> > kernel[2].
> >
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > [2] https://github.com/KSPP/linux/issues/89
> >
> > Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
> > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
>
> This looks good to me. So you'd like me to take this through
> the nfsd tree, possibly for 6.4-rc ?
>

This is my first week contributing to the Linux kernel so I might be
miscommunicating :)
By "land this patch", I meant to get this patch into to the nfsd tree.
I'll leave it up to you when you push it through to the mainline tree.
Although, it would be great to get this through to 6.4-rc if that's at
all possible.

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Chuck Lever III]

> On May 13, 2023, at 10:44 PM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote:
> 
>>> No, I plan to land this patch before attempting to fix __assign_str itself.
>>> Let me know if the below description looks good to you and I'll send
>>> over a v3 patch:
>>> 
>>> [PATCH v3] NFSD: Remove open coding of string copy
>>> 
>>> Instead of open coding a __dynamic_array(), use the __string() and
>>> __assign_str()
>>> helper macros that exist for this kind of use case.
>>> 
>>> Part of an effort to remove deprecated strlcpy() [1] completely from the
>>> kernel[2].
>>> 
>>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
>>> [2] https://github.com/KSPP/linux/issues/89
>>> 
>>> Fixes: 3c92fba557c6 ("NFSD: Enhance the nfsd_cb_setup tracepoint")
>>> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
>> 
>> This looks good to me. So you'd like me to take this through
>> the nfsd tree, possibly for 6.4-rc ?
>> 
> 
> This is my first week contributing to the Linux kernel so I might be
> miscommunicating :)
> By "land this patch", I meant to get this patch into to the nfsd tree.
> I'll leave it up to you when you push it through to the mainline tree.
> Although, it would be great to get this through to 6.4-rc if that's at
> all possible.

Please send v3 along, and I will apply it to nfsd-fixes.


--
Chuck Lever

Re: [PATCH v2] NFSD: Remove all occurences of strlcpy

[Azeem Shaikh]

>
> Please send v3 along, and I will apply it to nfsd-fixes.
>

Done. Thanks!