* [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() @ 2021-06-09 21:07 schumaker.anna 2021-06-14 23:14 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: schumaker.anna @ 2021-06-09 21:07 UTC (permalink / raw) To: Trond.Myklebust, linux-nfs; +Cc: Anna.Schumaker From: Anna Schumaker <Anna.Schumaker@Netapp.com> This seems to happen fairly easily during READ_PLUS testing on NFS v4.2. I found that we could end up accessing xdr->buf->pages[pgnr] with a pgnr greater than the number of pages in the array. So let's just return early if we're setting base to a point at the end of the page data and let xdr_set_tail_base() handle setting up the buffer pointers instead. Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> --- net/sunrpc/xdr.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c index 3964ff74ee51..ca10ba2626f2 100644 --- a/net/sunrpc/xdr.c +++ b/net/sunrpc/xdr.c @@ -1230,10 +1230,9 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr, void *kaddr; maxlen = xdr->buf->page_len; - if (base >= maxlen) { - base = maxlen; - maxlen = 0; - } else + if (base >= maxlen) + return 0; + else maxlen -= base; if (len > maxlen) len = maxlen; -- 2.32.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() 2021-06-09 21:07 [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() schumaker.anna @ 2021-06-14 23:14 ` J. Bruce Fields 2021-08-12 20:32 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2021-06-14 23:14 UTC (permalink / raw) To: schumaker.anna; +Cc: Trond.Myklebust, linux-nfs, Anna.Schumaker On Wed, Jun 09, 2021 at 05:07:29PM -0400, schumaker.anna@gmail.com wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > This seems to happen fairly easily during READ_PLUS testing on NFS v4.2. Yep, I hit a KASAN warning here every time, and this fixes it, thanks.--b. > I found that we could end up accessing xdr->buf->pages[pgnr] with a pgnr > greater than the number of pages in the array. So let's just return > early if we're setting base to a point at the end of the page data and > let xdr_set_tail_base() handle setting up the buffer pointers instead. > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > net/sunrpc/xdr.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > index 3964ff74ee51..ca10ba2626f2 100644 > --- a/net/sunrpc/xdr.c > +++ b/net/sunrpc/xdr.c > @@ -1230,10 +1230,9 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr, > void *kaddr; > > maxlen = xdr->buf->page_len; > - if (base >= maxlen) { > - base = maxlen; > - maxlen = 0; > - } else > + if (base >= maxlen) > + return 0; > + else > maxlen -= base; > if (len > maxlen) > len = maxlen; > -- > 2.32.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() 2021-06-14 23:14 ` J. Bruce Fields @ 2021-08-12 20:32 ` J. Bruce Fields 2021-08-26 19:44 ` Anna Schumaker 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2021-08-12 20:32 UTC (permalink / raw) To: schumaker.anna; +Cc: Trond.Myklebust, linux-nfs, Anna.Schumaker On Mon, Jun 14, 2021 at 07:14:40PM -0400, bfields wrote: > On Wed, Jun 09, 2021 at 05:07:29PM -0400, schumaker.anna@gmail.com wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > This seems to happen fairly easily during READ_PLUS testing on NFS v4.2. > > Yep, I hit a KASAN warning here every time, and this fixes it, > thanks.--b. By the way, config NFS_V4_2_READ_PLUS still says: This is intended for developers only. The READ_PLUS operation has been shown to have issues under specific conditions and should not be used in production. But this warning was the only thing I was seeing. Is there another known issue remaining? --b. > > > I found that we could end up accessing xdr->buf->pages[pgnr] with a pgnr > > greater than the number of pages in the array. So let's just return > > early if we're setting base to a point at the end of the page data and > > let xdr_set_tail_base() handle setting up the buffer pointers instead. > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > > net/sunrpc/xdr.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > > index 3964ff74ee51..ca10ba2626f2 100644 > > --- a/net/sunrpc/xdr.c > > +++ b/net/sunrpc/xdr.c > > @@ -1230,10 +1230,9 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr, > > void *kaddr; > > > > maxlen = xdr->buf->page_len; > > - if (base >= maxlen) { > > - base = maxlen; > > - maxlen = 0; > > - } else > > + if (base >= maxlen) > > + return 0; > > + else > > maxlen -= base; > > if (len > maxlen) > > len = maxlen; > > -- > > 2.32.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() 2021-08-12 20:32 ` J. Bruce Fields @ 2021-08-26 19:44 ` Anna Schumaker 2021-08-26 20:42 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: Anna Schumaker @ 2021-08-26 19:44 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, Linux NFS Mailing List On Thu, Aug 12, 2021 at 4:32 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Mon, Jun 14, 2021 at 07:14:40PM -0400, bfields wrote: > > On Wed, Jun 09, 2021 at 05:07:29PM -0400, schumaker.anna@gmail.com wrote: > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > > > This seems to happen fairly easily during READ_PLUS testing on NFS v4.2. > > > > Yep, I hit a KASAN warning here every time, and this fixes it, > > thanks.--b. > > By the way, config NFS_V4_2_READ_PLUS still says: > > This is intended for developers only. The READ_PLUS operation > has been shown to have issues under specific conditions and > should not be used in production. > > But this warning was the only thing I was seeing. Is there another > known issue remaining? I think it was an issue around using lseek to generate the reply. The file contents could change between each call, leading to inconsistent results (and a new failing xfstest that previously passed) Anna > > --b. > > > > > > I found that we could end up accessing xdr->buf->pages[pgnr] with a pgnr > > > greater than the number of pages in the array. So let's just return > > > early if we're setting base to a point at the end of the page data and > > > let xdr_set_tail_base() handle setting up the buffer pointers instead. > > > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > --- > > > net/sunrpc/xdr.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > > > index 3964ff74ee51..ca10ba2626f2 100644 > > > --- a/net/sunrpc/xdr.c > > > +++ b/net/sunrpc/xdr.c > > > @@ -1230,10 +1230,9 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr, > > > void *kaddr; > > > > > > maxlen = xdr->buf->page_len; > > > - if (base >= maxlen) { > > > - base = maxlen; > > > - maxlen = 0; > > > - } else > > > + if (base >= maxlen) > > > + return 0; > > > + else > > > maxlen -= base; > > > if (len > maxlen) > > > len = maxlen; > > > -- > > > 2.32.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() 2021-08-26 19:44 ` Anna Schumaker @ 2021-08-26 20:42 ` J. Bruce Fields 2021-08-26 20:51 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2021-08-26 20:42 UTC (permalink / raw) To: Anna Schumaker; +Cc: Trond Myklebust, Linux NFS Mailing List On Thu, Aug 26, 2021 at 03:44:32PM -0400, Anna Schumaker wrote: > On Thu, Aug 12, 2021 at 4:32 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > > > On Mon, Jun 14, 2021 at 07:14:40PM -0400, bfields wrote: > > > On Wed, Jun 09, 2021 at 05:07:29PM -0400, schumaker.anna@gmail.com wrote: > > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > > > > > This seems to happen fairly easily during READ_PLUS testing on NFS v4.2. > > > > > > Yep, I hit a KASAN warning here every time, and this fixes it, > > > thanks.--b. > > > > By the way, config NFS_V4_2_READ_PLUS still says: > > > > This is intended for developers only. The READ_PLUS operation > > has been shown to have issues under specific conditions and > > should not be used in production. > > > > But this warning was the only thing I was seeing. Is there another > > known issue remaining? > > I think it was an issue around using lseek to generate the reply. The > file contents could change between each call, leading to inconsistent > results (and a new failing xfstest that previously passed) OK, thanks, I see now that you mentioned in 21e31401fc45 "NFS: Disable READ_PLUS by default" that there were generic/091 and generic/263 failures. Looks like they're both testing concurrent direct and buffered IO. I don't know what we try to guarantee in that case. --b. > > Anna > > > > > --b. > > > > > > > > > I found that we could end up accessing xdr->buf->pages[pgnr] with a pgnr > > > > greater than the number of pages in the array. So let's just return > > > > early if we're setting base to a point at the end of the page data and > > > > let xdr_set_tail_base() handle setting up the buffer pointers instead. > > > > > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > --- > > > > net/sunrpc/xdr.c | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > > > > index 3964ff74ee51..ca10ba2626f2 100644 > > > > --- a/net/sunrpc/xdr.c > > > > +++ b/net/sunrpc/xdr.c > > > > @@ -1230,10 +1230,9 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr, > > > > void *kaddr; > > > > > > > > maxlen = xdr->buf->page_len; > > > > - if (base >= maxlen) { > > > > - base = maxlen; > > > > - maxlen = 0; > > > > - } else > > > > + if (base >= maxlen) > > > > + return 0; > > > > + else > > > > maxlen -= base; > > > > if (len > maxlen) > > > > len = maxlen; > > > > -- > > > > 2.32.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() 2021-08-26 20:42 ` J. Bruce Fields @ 2021-08-26 20:51 ` J. Bruce Fields 0 siblings, 0 replies; 6+ messages in thread From: J. Bruce Fields @ 2021-08-26 20:51 UTC (permalink / raw) To: Anna Schumaker; +Cc: Trond Myklebust, Linux NFS Mailing List On Thu, Aug 26, 2021 at 04:42:21PM -0400, J. Bruce Fields wrote: > On Thu, Aug 26, 2021 at 03:44:32PM -0400, Anna Schumaker wrote: > > On Thu, Aug 12, 2021 at 4:32 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > > > > > On Mon, Jun 14, 2021 at 07:14:40PM -0400, bfields wrote: > > > > On Wed, Jun 09, 2021 at 05:07:29PM -0400, schumaker.anna@gmail.com wrote: > > > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > > > > > > > This seems to happen fairly easily during READ_PLUS testing on NFS v4.2. > > > > > > > > Yep, I hit a KASAN warning here every time, and this fixes it, > > > > thanks.--b. > > > > > > By the way, config NFS_V4_2_READ_PLUS still says: > > > > > > This is intended for developers only. The READ_PLUS operation > > > has been shown to have issues under specific conditions and > > > should not be used in production. > > > > > > But this warning was the only thing I was seeing. Is there another > > > known issue remaining? > > > > I think it was an issue around using lseek to generate the reply. The > > file contents could change between each call, leading to inconsistent > > results (and a new failing xfstest that previously passed) > > OK, thanks, I see now that you mentioned in 21e31401fc45 "NFS: Disable > READ_PLUS by default" that there were generic/091 and generic/263 > failures. > > Looks like they're both testing concurrent direct and buffered IO. I > don't know what we try to guarantee in that case. But I'd assumed generic/263 just wasn't supported on nfs anyway.--b. generic/263 81s ... [failed, exit status 1]- output mismatch (see /root/xfstests-dev/results//generic/263.out.bad) --- tests/generic/263.out 2019-12-20 17:34:10.493343575 -0500 +++ /root/xfstests-dev/results//generic/263.out.bad 2021-08-26 16:43:40.751891500 -0400 @@ -1,3 +1,262 @@ QA output created by 263 fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z +Seed set to 1 +main: filesystem does not support fallocate mode FALLOC_FL_KEEP_SIZE, disabling! +main: filesystem does not support fallocate mode FALLOC_FL_ZERO_RANGE, disabling! +main: filesystem does not support fallocate mode FALLOC_FL_COLLAPSE_RANGE, disabling! ... (Run 'diff -u /root/xfstests-dev/tests/generic/263.out /root/xfstests-dev/results//generic/263.out.bad' to see the entire diff) Ran: generic/263 Failures: generic/263 Failed 1 of 1 tests ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-26 20:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-09 21:07 [PATCH] sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() schumaker.anna 2021-06-14 23:14 ` J. Bruce Fields 2021-08-12 20:32 ` J. Bruce Fields 2021-08-26 19:44 ` Anna Schumaker 2021-08-26 20:42 ` J. Bruce Fields 2021-08-26 20:51 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.