* [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint
@ 2021-06-25 15:12 Chuck Lever
2021-06-25 16:28 ` Trond Myklebust
0 siblings, 1 reply; 5+ messages in thread
From: Chuck Lever @ 2021-06-25 15:12 UTC (permalink / raw)
To: bfields; +Cc: linux-nfs
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,
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint 2021-06-25 15:12 [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint Chuck Lever @ 2021-06-25 16:28 ` Trond Myklebust 2021-06-25 18:05 ` Chuck Lever III 0 siblings, 1 reply; 5+ messages in thread From: Trond Myklebust @ 2021-06-25 16:28 UTC (permalink / raw) To: bfields, chuck.lever; +Cc: linux-nfs 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? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint 2021-06-25 16:28 ` Trond Myklebust @ 2021-06-25 18:05 ` Chuck Lever III 2021-06-25 18:57 ` Trond Myklebust 0 siblings, 1 reply; 5+ messages in thread From: Chuck Lever III @ 2021-06-25 18:05 UTC (permalink / raw) To: Trond Myklebust; +Cc: Bruce Fields, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint 2021-06-25 18:05 ` Chuck Lever III @ 2021-06-25 18:57 ` Trond Myklebust 2021-06-25 19:38 ` Chuck Lever III 0 siblings, 1 reply; 5+ messages in thread From: Trond Myklebust @ 2021-06-25 18:57 UTC (permalink / raw) To: chuck.lever; +Cc: bfields, linux-nfs 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" -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint 2021-06-25 18:57 ` Trond Myklebust @ 2021-06-25 19:38 ` Chuck Lever III 0 siblings, 0 replies; 5+ messages in thread From: Chuck Lever III @ 2021-06-25 19:38 UTC (permalink / raw) To: Trond Myklebust; +Cc: Bruce Fields, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-25 19:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-25 15:12 [PATCH] NFSD: Prevent a possible oops in the nfs_dirent() tracepoint Chuck Lever 2021-06-25 16:28 ` Trond Myklebust 2021-06-25 18:05 ` Chuck Lever III 2021-06-25 18:57 ` Trond Myklebust 2021-06-25 19:38 ` Chuck Lever III
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).