[PATCH] xfs_repair: don't let bplist index go negative in prefetch

[PATCH] xfs_repair: don't let bplist index go negative in prefetch

[Eric Sandeen]

After:

bbd3275 repair: don't unlock prefetch tree to read discontig buffers

Coverity spotted that it's possible for us to arrive at the loop
below with num == 1, and then we decrement it to 0, and try to
index bplist[num-1].

I think this was possible before the change, i.e. it's probably
not a regression.

Fix this by not trying to shrink the window unless we have
more than one buffer in the array.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


diff --git a/repair/prefetch.c b/repair/prefetch.c
index 4595310..b6d4755 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -505,7 +505,7 @@ pf_batch_read(
 		first_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[0]));
 		last_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[num-1])) +
 			XFS_BUF_SIZE(bplist[num-1]);
-		while (last_off - first_off > pf_max_bytes) {
+		while (num > 1 && last_off - first_off > pf_max_bytes) {
 			num--;
 			last_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[num-1])) +
 				XFS_BUF_SIZE(bplist[num-1]);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs_repair: don't let bplist index go negative in prefetch

[Eric Sandeen]

On 5/15/14, 1:59 PM, Eric Sandeen wrote:
> After:
> 
> bbd3275 repair: don't unlock prefetch tree to read discontig buffers
> 
> Coverity spotted that it's possible for us to arrive at the loop
> below with num == 1, and then we decrement it to 0, and try to
> index bplist[num-1].
> 
> I think this was possible before the change, i.e. it's probably
> not a regression.
> 
> Fix this by not trying to shrink the window unless we have
> more than one buffer in the array.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

FWIW, I'm not sure this can actually be hit; see below.

> 
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 4595310..b6d4755 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -505,7 +505,7 @@ pf_batch_read(
>  		first_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[0]));
>  		last_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[num-1])) +
>  			XFS_BUF_SIZE(bplist[num-1]);

Indexing bplist[num-1] after we do num-- is only a problem if num==1.

If num==1, then last_off - first_off == XFS_BUF_SIZE(bplist[0]) above.

> -		while (last_off - first_off > pf_max_bytes) {

so we can only go here if XFS_BUF_SIZE(bplist[0] > pf_max_bytes, and

        pf_max_bytes = sysconf(_SC_PAGE_SIZE) << 7;

for a 4k page that's 512k.

So unless XFS_BUF_SIZE(bplist[0]) > 512k, we won't run into trouble.

And I don't ... think that can happen, right?  So it's probably impossible
to hit; worth being defensive, but not critical.

That's my take anyhoo.

-Eric

> +		while (num > 1 && last_off - first_off > pf_max_bytes) {
>  			num--;
>  			last_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[num-1])) +
>  				XFS_BUF_SIZE(bplist[num-1]);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs_repair: don't let bplist index go negative in prefetch

[Dave Chinner]

On Thu, May 15, 2014 at 09:22:40PM -0500, Eric Sandeen wrote:
> On 5/15/14, 1:59 PM, Eric Sandeen wrote:
> > After:
> > 
> > bbd3275 repair: don't unlock prefetch tree to read discontig buffers
> > 
> > Coverity spotted that it's possible for us to arrive at the loop
> > below with num == 1, and then we decrement it to 0, and try to
> > index bplist[num-1].
> > 
> > I think this was possible before the change, i.e. it's probably
> > not a regression.
> > 
> > Fix this by not trying to shrink the window unless we have
> > more than one buffer in the array.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> 
> FWIW, I'm not sure this can actually be hit; see below.
> 
> > 
> > diff --git a/repair/prefetch.c b/repair/prefetch.c
> > index 4595310..b6d4755 100644
> > --- a/repair/prefetch.c
> > +++ b/repair/prefetch.c
> > @@ -505,7 +505,7 @@ pf_batch_read(
> >  		first_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[0]));
> >  		last_off = LIBXFS_BBTOOFF64(XFS_BUF_ADDR(bplist[num-1])) +
> >  			XFS_BUF_SIZE(bplist[num-1]);
> 
> Indexing bplist[num-1] after we do num-- is only a problem if num==1.
> 
> If num==1, then last_off - first_off == XFS_BUF_SIZE(bplist[0]) above.
> 
> > -		while (last_off - first_off > pf_max_bytes) {
> 
> so we can only go here if XFS_BUF_SIZE(bplist[0] > pf_max_bytes, and
> 
>         pf_max_bytes = sysconf(_SC_PAGE_SIZE) << 7;
> 
> for a 4k page that's 512k.
> 
> So unless XFS_BUF_SIZE(bplist[0]) > 512k, we won't run into trouble.

For prefetch, it can't be more than 64k (the maximum size of a
metadata block), so I think we're safe right at the moment.

> And I don't ... think that can happen, right?  So it's probably impossible
> to hit; worth being defensive, but not critical.

Agreed, it doesn't appear like a critical fix. I'll queue it up for
after the 3.2.0 release.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs