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 <[email protected]>
---
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,
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 <[email protected]>
> ---
> 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
[email protected]
> On Jun 25, 2021, at 12:28 PM, Trond Myklebust <[email protected]> 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 <[email protected]>
>> ---
>> 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
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
[email protected]
> On Jun 25, 2021, at 2:57 PM, Trond Myklebust <[email protected]> wrote:
>
> On Fri, 2021-06-25 at 18:05 +0000, Chuck Lever III wrote:
>>
>>
>>> On Jun 25, 2021, at 12:28 PM, Trond Myklebust
>>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
> [email protected]
--
Chuck Lever