| Message ID | 20230512145356.396567-1-azeemshaikh38@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] NFSD: Remove all occurences of strlcpy | expand |
> 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
> 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
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>
> 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
> > 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.
> 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
> > Please send v3 along, and I will apply it to nfsd-fixes. > Done. Thanks!
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,
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(-)