While looking into the filecache CPU soft lock-up issue, I ran
across this problem. I thought I could run it down in just an
afternoon (I was wrong) and that this problem probably affects more
users than the soft lock-up (jury's still out).
Anyway, NFSD's new READDIRPLUS dirent encoder blows past the end of
the directory payload xdr_stream when the client requests more than
a page worth of directory entries. I tracked this down to how
xdr_get_next_encode_buffer() computes xdr->end. First patch in this
series is the fix. The remaining patches are clean-ups and
optimizations.
I want to get this into 5.19-rc quickly. I would appreciate getting
at least two R-b's for this series.
---
Chuck Lever (5):
SUNRPC: Fix the calculation of xdr->end in xdr_get_next_encode_buffer()
SUNRPC: Optimize xdr_reserve_space()
SUNRPC: Clean up xdr_commit_encode()
SUNRPC: Clean up xdr_get_next_encode_buffer()
SUNRPC: Remove pointer type casts from xdr_get_next_encode_buffer()
net/sunrpc/xdr.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
--
Chuck Lever
Both the ::iov_len field and the third parameter of memcpy() and
memmove() are size_t. There's no reason for the implicit conversion
from size_t to int and back. Change the type of @shift to make the
code easier to read and understand.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/xdr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 08a85375b311..de8f71468637 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -933,14 +933,16 @@ EXPORT_SYMBOL_GPL(xdr_init_encode);
*/
inline void xdr_commit_encode(struct xdr_stream *xdr)
{
- int shift = xdr->scratch.iov_len;
+ size_t shift = xdr->scratch.iov_len;
void *page;
if (shift == 0)
return;
+
page = page_address(*xdr->page_ptr);
memcpy(xdr->scratch.iov_base, page, shift);
memmove(page, page + shift, (void *)xdr->p - page);
+
xdr_reset_scratch_buffer(xdr);
}
EXPORT_SYMBOL_GPL(xdr_commit_encode);
On Tue, 07 Jun 2022, Chuck Lever III wrote:
>
> > On Jun 6, 2022, at 9:05 PM, NeilBrown <[email protected]> wrote:
> >
> > On Mon, 06 Jun 2022, Chuck Lever wrote:
> >> Both the ::iov_len field and the third parameter of memcpy() and
> >> memmove() are size_t. There's no reason for the implicit conversion
> >> from size_t to int and back. Change the type of @shift to make the
> >> code easier to read and understand.
> >
> > I wouldn't object to "shift" being renamed "frag1bytes" to make the
> > connection with xdr_get_next_encode_buffer() more blatant..
>
> I thought of that. Readers would wonder why frag1bytes in
> xdr_commit_encode() was a size_t, but in xdr_get_next_encode_buffer()
> it's a signed int. It started to become a longer string to pull.
> Maybe it's worth it?
>
Probably worth it. Not necessarily worth it today. I have no strong
preference.
NeilBrown
>
> > Reviewed-by: NeilBrown <[email protected]>
> >
> > Thanks,
> > NeilBrown
> >
> >
> >>
> >> Signed-off-by: Chuck Lever <[email protected]>
> >> ---
> >> net/sunrpc/xdr.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >> index 08a85375b311..de8f71468637 100644
> >> --- a/net/sunrpc/xdr.c
> >> +++ b/net/sunrpc/xdr.c
> >> @@ -933,14 +933,16 @@ EXPORT_SYMBOL_GPL(xdr_init_encode);
> >> */
> >> inline void xdr_commit_encode(struct xdr_stream *xdr)
> >> {
> >> - int shift = xdr->scratch.iov_len;
> >> + size_t shift = xdr->scratch.iov_len;
> >> void *page;
> >>
> >> if (shift == 0)
> >> return;
> >> +
> >> page = page_address(*xdr->page_ptr);
> >> memcpy(xdr->scratch.iov_base, page, shift);
> >> memmove(page, page + shift, (void *)xdr->p - page);
> >> +
> >> xdr_reset_scratch_buffer(xdr);
> >> }
> >> EXPORT_SYMBOL_GPL(xdr_commit_encode);
> >>
> >>
> >>
>
> --
> Chuck Lever
>
>
>
>
> On Jun 6, 2022, at 9:05 PM, NeilBrown <[email protected]> wrote:
>
> On Mon, 06 Jun 2022, Chuck Lever wrote:
>> Both the ::iov_len field and the third parameter of memcpy() and
>> memmove() are size_t. There's no reason for the implicit conversion
>> from size_t to int and back. Change the type of @shift to make the
>> code easier to read and understand.
>
> I wouldn't object to "shift" being renamed "frag1bytes" to make the
> connection with xdr_get_next_encode_buffer() more blatant..
I thought of that. Readers would wonder why frag1bytes in
xdr_commit_encode() was a size_t, but in xdr_get_next_encode_buffer()
it's a signed int. It started to become a longer string to pull.
Maybe it's worth it?
> Reviewed-by: NeilBrown <[email protected]>
>
> Thanks,
> NeilBrown
>
>
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>> net/sunrpc/xdr.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 08a85375b311..de8f71468637 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -933,14 +933,16 @@ EXPORT_SYMBOL_GPL(xdr_init_encode);
>> */
>> inline void xdr_commit_encode(struct xdr_stream *xdr)
>> {
>> - int shift = xdr->scratch.iov_len;
>> + size_t shift = xdr->scratch.iov_len;
>> void *page;
>>
>> if (shift == 0)
>> return;
>> +
>> page = page_address(*xdr->page_ptr);
>> memcpy(xdr->scratch.iov_base, page, shift);
>> memmove(page, page + shift, (void *)xdr->p - page);
>> +
>> xdr_reset_scratch_buffer(xdr);
>> }
>> EXPORT_SYMBOL_GPL(xdr_commit_encode);
>>
>>
>>
--
Chuck Lever
On Mon, 06 Jun 2022, Chuck Lever wrote:
> Both the ::iov_len field and the third parameter of memcpy() and
> memmove() are size_t. There's no reason for the implicit conversion
> from size_t to int and back. Change the type of @shift to make the
> code easier to read and understand.
I wouldn't object to "shift" being renamed "frag1bytes" to make the
connection with xdr_get_next_encode_buffer() more blatant..
Reviewed-by: NeilBrown <[email protected]>
Thanks,
NeilBrown
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> net/sunrpc/xdr.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 08a85375b311..de8f71468637 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -933,14 +933,16 @@ EXPORT_SYMBOL_GPL(xdr_init_encode);
> */
> inline void xdr_commit_encode(struct xdr_stream *xdr)
> {
> - int shift = xdr->scratch.iov_len;
> + size_t shift = xdr->scratch.iov_len;
> void *page;
>
> if (shift == 0)
> return;
> +
> page = page_address(*xdr->page_ptr);
> memcpy(xdr->scratch.iov_base, page, shift);
> memmove(page, page + shift, (void *)xdr->p - page);
> +
> xdr_reset_scratch_buffer(xdr);
> }
> EXPORT_SYMBOL_GPL(xdr_commit_encode);
>
>
>
On Mon, 06 Jun 2022, Chuck Lever wrote:
> While looking into the filecache CPU soft lock-up issue, I ran
> across this problem. I thought I could run it down in just an
> afternoon (I was wrong) and that this problem probably affects more
> users than the soft lock-up (jury's still out).
>
> Anyway, NFSD's new READDIRPLUS dirent encoder blows past the end of
> the directory payload xdr_stream when the client requests more than
> a page worth of directory entries. I tracked this down to how
> xdr_get_next_encode_buffer() computes xdr->end. First patch in this
> series is the fix. The remaining patches are clean-ups and
> optimizations.
>
> I want to get this into 5.19-rc quickly. I would appreciate getting
> at least two R-b's for this series.
Just for completeness:
Reviewed-by: NeilBrown <[email protected]>
for the whole series. Nothing I saw would justify any delay.
NeilBrown
>
> ---
>
> Chuck Lever (5):
> SUNRPC: Fix the calculation of xdr->end in xdr_get_next_encode_buffer()
> SUNRPC: Optimize xdr_reserve_space()
> SUNRPC: Clean up xdr_commit_encode()
> SUNRPC: Clean up xdr_get_next_encode_buffer()
> SUNRPC: Remove pointer type casts from xdr_get_next_encode_buffer()
>
>
> net/sunrpc/xdr.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> --
> Chuck Lever
>
>
> On Jun 7, 2022, at 7:37 AM, NeilBrown <[email protected]> wrote:
>
> On Mon, 06 Jun 2022, Chuck Lever wrote:
>> While looking into the filecache CPU soft lock-up issue, I ran
>> across this problem. I thought I could run it down in just an
>> afternoon (I was wrong) and that this problem probably affects more
>> users than the soft lock-up (jury's still out).
>>
>> Anyway, NFSD's new READDIRPLUS dirent encoder blows past the end of
>> the directory payload xdr_stream when the client requests more than
>> a page worth of directory entries. I tracked this down to how
>> xdr_get_next_encode_buffer() computes xdr->end. First patch in this
>> series is the fix. The remaining patches are clean-ups and
>> optimizations.
>>
>> I want to get this into 5.19-rc quickly. I would appreciate getting
>> at least two R-b's for this series.
>
> Just for completeness:
> Reviewed-by: NeilBrown <[email protected]>
> for the whole series. Nothing I saw would justify any delay.
Thanks for your review. I have some thoughts about how to deal
idiomatically with your inlining optimization suggestion. I will
post a v2, I think that patch will be the only update.
> NeilBrown
>
>>
>> ---
>>
>> Chuck Lever (5):
>> SUNRPC: Fix the calculation of xdr->end in xdr_get_next_encode_buffer()
>> SUNRPC: Optimize xdr_reserve_space()
>> SUNRPC: Clean up xdr_commit_encode()
>> SUNRPC: Clean up xdr_get_next_encode_buffer()
>> SUNRPC: Remove pointer type casts from xdr_get_next_encode_buffer()
>>
>>
>> net/sunrpc/xdr.c | 31 ++++++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> --
>> Chuck Lever
--
Chuck Lever