| Message ID | 162463396907.1820.8112792283525036426.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Prevent a possible oops in the nfs_dirent() tracepoint | expand |
On Fri, 2021-06-25 at 11:12 -0400, Chuck Lever wrote: > The double copy of the string is a mistake, plus __assign_str() > uses strlen(), which is wrong to do on a string that isn't > guaranteed to be NUL-terminated. > > Fixes: 6019ce0742ca ("NFSD: Add a tracepoint to record directory > entry encoding") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/trace.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h > index 27a93ebd1d80..89dccced526a 100644 > --- a/fs/nfsd/trace.h > +++ b/fs/nfsd/trace.h > @@ -408,7 +408,6 @@ TRACE_EVENT(nfsd_dirent, > __entry->ino = ino; > __entry->len = namlen; > memcpy(__get_str(name), name, namlen); > - __assign_str(name, name); > ), > TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", > __entry->fh_hash, __entry->ino, > > Why not just store it as a NUL terminated string and save a few bytes by getting rid of the integer-sized storage of the length?
> On Jun 25, 2021, at 12:28 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Fri, 2021-06-25 at 11:12 -0400, Chuck Lever wrote: >> The double copy of the string is a mistake, plus __assign_str() >> uses strlen(), which is wrong to do on a string that isn't >> guaranteed to be NUL-terminated. >> >> Fixes: 6019ce0742ca ("NFSD: Add a tracepoint to record directory >> entry encoding") >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> fs/nfsd/trace.h | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h >> index 27a93ebd1d80..89dccced526a 100644 >> --- a/fs/nfsd/trace.h >> +++ b/fs/nfsd/trace.h >> @@ -408,7 +408,6 @@ TRACE_EVENT(nfsd_dirent, >> __entry->ino = ino; >> __entry->len = namlen; >> memcpy(__get_str(name), name, namlen); >> - __assign_str(name, name); >> ), >> TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", >> __entry->fh_hash, __entry->ino, >> >> > > Why not just store it as a NUL terminated string and save a few bytes > by getting rid of the integer-sized storage of the length? Stephen is adding some helpers to do that in the next merge window. For now, I need to fix this oops, and this is the fastest way to do that. -- Chuck Lever
On Fri, 2021-06-25 at 18:05 +0000, Chuck Lever III wrote: > > > > On Jun 25, 2021, at 12:28 PM, Trond Myklebust > > <trondmy@hammerspace.com> wrote: > > > > On Fri, 2021-06-25 at 11:12 -0400, Chuck Lever wrote: > > > The double copy of the string is a mistake, plus __assign_str() > > > uses strlen(), which is wrong to do on a string that isn't > > > guaranteed to be NUL-terminated. > > > > > > Fixes: 6019ce0742ca ("NFSD: Add a tracepoint to record directory > > > entry encoding") > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > > --- > > > fs/nfsd/trace.h | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h > > > index 27a93ebd1d80..89dccced526a 100644 > > > --- a/fs/nfsd/trace.h > > > +++ b/fs/nfsd/trace.h > > > @@ -408,7 +408,6 @@ TRACE_EVENT(nfsd_dirent, > > > __entry->ino = ino; > > > __entry->len = namlen; > > > memcpy(__get_str(name), name, namlen); > > > - __assign_str(name, name); > > > ), > > > TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", > > > __entry->fh_hash, __entry->ino, > > > > > > > > > > Why not just store it as a NUL terminated string and save a few bytes > > by getting rid of the integer-sized storage of the length? > > Stephen is adding some helpers to do that in the next merge > window. For now, I need to fix this oops, and this is the > fastest way to do that. Won't this suffice? 8<--------------------------------------- diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index 27a93ebd1d80..799168774ccf 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -400,19 +400,17 @@ TRACE_EVENT(nfsd_dirent, TP_STRUCT__entry( __field(u32, fh_hash) __field(u64, ino) - __field(int, len) - __dynamic_array(unsigned char, name, namlen) + __dynamic_array(unsigned char, name, namlen + 1) ), TP_fast_assign( __entry->fh_hash = fhp ? knfsd_fh_hash(&fhp->fh_handle) : 0; __entry->ino = ino; - __entry->len = namlen; memcpy(__get_str(name), name, namlen); - __assign_str(name, name); + __get_str(name)[namlen] = 0; ), - TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", + TP_printk("fh_hash=0x%08x ino=%llu name=%s", __entry->fh_hash, __entry->ino, - __entry->len, __get_str(name)) + __get_str(name)) ) #include "state.h"
> On Jun 25, 2021, at 2:57 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Fri, 2021-06-25 at 18:05 +0000, Chuck Lever III wrote: >> >> >>> On Jun 25, 2021, at 12:28 PM, Trond Myklebust >>> <trondmy@hammerspace.com> wrote: >>> >>> On Fri, 2021-06-25 at 11:12 -0400, Chuck Lever wrote: >>>> The double copy of the string is a mistake, plus __assign_str() >>>> uses strlen(), which is wrong to do on a string that isn't >>>> guaranteed to be NUL-terminated. >>>> >>>> Fixes: 6019ce0742ca ("NFSD: Add a tracepoint to record directory >>>> entry encoding") >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >>>> --- >>>> fs/nfsd/trace.h | 1 - >>>> 1 file changed, 1 deletion(-) >>>> >>>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h >>>> index 27a93ebd1d80..89dccced526a 100644 >>>> --- a/fs/nfsd/trace.h >>>> +++ b/fs/nfsd/trace.h >>>> @@ -408,7 +408,6 @@ TRACE_EVENT(nfsd_dirent, >>>> __entry->ino = ino; >>>> __entry->len = namlen; >>>> memcpy(__get_str(name), name, namlen); >>>> - __assign_str(name, name); >>>> ), >>>> TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", >>>> __entry->fh_hash, __entry->ino, >>>> >>>> >>> >>> Why not just store it as a NUL terminated string and save a few bytes >>> by getting rid of the integer-sized storage of the length? >> >> Stephen is adding some helpers to do that in the next merge >> window. For now, I need to fix this oops, and this is the >> fastest way to do that. > > > Won't this suffice? Yes of course it will. As I said, Stephen has some trace macros lines up for the next merge window that I would prefer to use. I will be posting patches that use those as soon as 5.14-rc1 appears. For now, a single line fix is needed to prevent an oops in 5.13. > 8<--------------------------------------- > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h > index 27a93ebd1d80..799168774ccf 100644 > --- a/fs/nfsd/trace.h > +++ b/fs/nfsd/trace.h > @@ -400,19 +400,17 @@ TRACE_EVENT(nfsd_dirent, > TP_STRUCT__entry( > __field(u32, fh_hash) > __field(u64, ino) > - __field(int, len) > - __dynamic_array(unsigned char, name, namlen) > + __dynamic_array(unsigned char, name, namlen + 1) > ), > TP_fast_assign( > __entry->fh_hash = fhp ? knfsd_fh_hash(&fhp->fh_handle) : 0; > __entry->ino = ino; > - __entry->len = namlen; > memcpy(__get_str(name), name, namlen); > - __assign_str(name, name); > + __get_str(name)[namlen] = 0; > ), > - TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", > + TP_printk("fh_hash=0x%08x ino=%llu name=%s", > __entry->fh_hash, __entry->ino, > - __entry->len, __get_str(name)) > + __get_str(name)) > ) > > #include "state.h" > > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com -- Chuck Lever
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index 27a93ebd1d80..89dccced526a 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -408,7 +408,6 @@ TRACE_EVENT(nfsd_dirent, __entry->ino = ino; __entry->len = namlen; memcpy(__get_str(name), name, namlen); - __assign_str(name, name); ), TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", __entry->fh_hash, __entry->ino,
The double copy of the string is a mistake, plus __assign_str() uses strlen(), which is wrong to do on a string that isn't guaranteed to be NUL-terminated. Fixes: 6019ce0742ca ("NFSD: Add a tracepoint to record directory entry encoding") Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- fs/nfsd/trace.h | 1 - 1 file changed, 1 deletion(-)