diff mbox

clean_bdev_aliases: Prevent cleaning blocks that are not in block range

Message ID 1482672663-17122-1-git-send-email-chandan@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chandan Rajendra Dec. 25, 2016, 1:31 p.m. UTC
The first block to be cleaned may start at a non-zero page offset. In
such a scenario clean_bdev_aliases() will end up cleaning blocks that
do not fall in the range of blocks to be cleaned. This commit fixes the
issue by skipping blocks that do not fall in valid block range.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jan Kara Jan. 2, 2017, 3:58 p.m. UTC | #1
On Sun 25-12-16 19:01:03, Chandan Rajendra wrote:
> The first block to be cleaned may start at a non-zero page offset. In
> such a scenario clean_bdev_aliases() will end up cleaning blocks that
> do not fall in the range of blocks to be cleaned. This commit fixes the
> issue by skipping blocks that do not fall in valid block range.
> 
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

Ah, very good catch! How did you spot this?

The patch looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

Jens, please merge this fix quickly as we may end up discarding changes to
innocent metadata blocks due to this... Thanks!

								Honza
> ---
>  fs/buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 1df2bd5..28484b3 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
>  			head = page_buffers(page);
>  			bh = head;
>  			do {
> -				if (!buffer_mapped(bh))
> +				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
>  					goto next;
>  				if (bh->b_blocknr >= block + len)
>  					break;
> -- 
> 2.5.5
>
Eryu Guan Jan. 2, 2017, 4:21 p.m. UTC | #2
On Mon, Jan 02, 2017 at 04:58:03PM +0100, Jan Kara wrote:
> On Sun 25-12-16 19:01:03, Chandan Rajendra wrote:
> > The first block to be cleaned may start at a non-zero page offset. In
> > such a scenario clean_bdev_aliases() will end up cleaning blocks that
> > do not fall in the range of blocks to be cleaned. This commit fixes the
> > issue by skipping blocks that do not fall in valid block range.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> 
> Ah, very good catch! How did you spot this?

I failed to notice this patch, and I came up with a same patch today
myself, and I'm still testing it.

I found this by xfstests, many tests (tens of tests) failed fsck after
test when testing extN if blocksize < pagesize. E.g. generic/013 could
reproduce the fs corruption quite reliablely for me.

Reviewed-by: Eryu Guan <eguan@redhat.com>

> 
> The patch looks good. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Jens, please merge this fix quickly as we may end up discarding changes to
> innocent metadata blocks due to this... Thanks!
> 
> 								Honza
> > ---
> >  fs/buffer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 1df2bd5..28484b3 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
> >  			head = page_buffers(page);
> >  			bh = head;
> >  			do {
> > -				if (!buffer_mapped(bh))
> > +				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
> >  					goto next;
> >  				if (bh->b_blocknr >= block + len)
> >  					break;
> > -- 
> > 2.5.5
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe Jan. 2, 2017, 4:35 p.m. UTC | #3
On 12/25/2016 06:31 AM, Chandan Rajendra wrote:
> The first block to be cleaned may start at a non-zero page offset. In
> such a scenario clean_bdev_aliases() will end up cleaning blocks that
> do not fall in the range of blocks to be cleaned. This commit fixes the
> issue by skipping blocks that do not fall in valid block range.

Queued up, thanks.
Jan Kara Jan. 2, 2017, 4:46 p.m. UTC | #4
On Tue 03-01-17 00:21:45, Eryu Guan wrote:
> On Mon, Jan 02, 2017 at 04:58:03PM +0100, Jan Kara wrote:
> > On Sun 25-12-16 19:01:03, Chandan Rajendra wrote:
> > > The first block to be cleaned may start at a non-zero page offset. In
> > > such a scenario clean_bdev_aliases() will end up cleaning blocks that
> > > do not fall in the range of blocks to be cleaned. This commit fixes the
> > > issue by skipping blocks that do not fall in valid block range.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > 
> > Ah, very good catch! How did you spot this?
> 
> I failed to notice this patch, and I came up with a same patch today
> myself, and I'm still testing it.
> 
> I found this by xfstests, many tests (tens of tests) failed fsck after
> test when testing extN if blocksize < pagesize. E.g. generic/013 could
> reproduce the fs corruption quite reliablely for me.
> 
> Reviewed-by: Eryu Guan <eguan@redhat.com>

Thanks! Yeah, I had run xfstests with those patches but not with blocksize
< pagesize :-| Shit happens... need to be more careful next time.

								Honza

> > The patch looks good. You can add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > Jens, please merge this fix quickly as we may end up discarding changes to
> > innocent metadata blocks due to this... Thanks!
> > 
> > 								Honza
> > > ---
> > >  fs/buffer.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/buffer.c b/fs/buffer.c
> > > index 1df2bd5..28484b3 100644
> > > --- a/fs/buffer.c
> > > +++ b/fs/buffer.c
> > > @@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
> > >  			head = page_buffers(page);
> > >  			bh = head;
> > >  			do {
> > > -				if (!buffer_mapped(bh))
> > > +				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
> > >  					goto next;
> > >  				if (bh->b_blocknr >= block + len)
> > >  					break;
> > > -- 
> > > 2.5.5
> > > 
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-block" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/buffer.c b/fs/buffer.c
index 1df2bd5..28484b3 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1660,7 +1660,7 @@  void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
 			head = page_buffers(page);
 			bh = head;
 			do {
-				if (!buffer_mapped(bh))
+				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
 					goto next;
 				if (bh->b_blocknr >= block + len)
 					break;