Message ID | 20210609210729.254578-1-Anna.Schumaker@Netapp.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | sunrpc: Avoid a KASAN slab-out-of-bounds bug in xdr_set_page_base() | expand |
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
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
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
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
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
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;