[Ocfs2-devel] [PATCH 0/7] Fix a pile of 4GB file problems on 32-bit

[Ocfs2-devel] [PATCH 0/7] Fix a pile of 4GB file problems on 32-bit

[Matthew Wilcox]

I caught a bug in my own code where I forgot to cast to loff_t before
shifting.  So I thought I'd grep around and see if I could find any
other occurrences.  I found a few that were clearly bugs, and they're
fixed below.  There are other places where we don't cast, and I think
they're OK.  For example, some places we have a 'nr_pages' being shifted
by PAGE_SHIFT, and that's probably OK because it's probably a single I/O.

Also, I didn't touch AFFS or ROMFS or some other filesystems which
probably have never seen a 4GB file in their lives.  Might be worth
fixing to be sure nobody copies bad code from them, but not worth cc'ing
stable for.

I didn't look for SECTOR_SHIFT or SECTOR_SIZE (or bare 9/512), just
PAGE_SIZE and PAGE_SHIFT.

I can't find a GCC warning to enable for this pattern, so I filed
https://urldefense.com/v3/__https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97287__;!!GqivPVa7Brio!JmJRfLO1W1WY_UNpWJIfK5J2F0viNTrZ3l7xIrCKzl03yThvlPmVBeZHbHY6hZu2IJ2u7Q$ 

Matthew Wilcox (Oracle) (7):
  9P: Cast to loff_t before multiplying
  buffer: Promote to unsigned long long before shifting
  ceph: Promote to unsigned long long before shifting
  ocfs2: Promote to unsigned long long before shifting
  btrfs: Promote to unsigned long long before shifting
  btrfs: Promote to unsigned long long before shifting
  btrfs: Promote to unsigned long long before multiplying

 fs/9p/vfs_file.c  |  4 ++--
 fs/btrfs/ioctl.c  |  6 +++---
 fs/btrfs/raid56.c |  2 +-
 fs/btrfs/scrub.c  | 25 ++++++++++++++++---------
 fs/buffer.c       |  2 +-
 fs/ceph/addr.c    |  2 +-
 fs/ocfs2/alloc.c  |  2 +-
 7 files changed, 25 insertions(+), 18 deletions(-)

-- 
2.28.0

[Ocfs2-devel] [PATCH 1/7] 9P: Cast to loff_t before multiplying

[Matthew Wilcox]

On 32-bit systems, this multiplication will overflow for files larger
than 4GB.

Cc: stable at vger.kernel.org
Fixes: fb89b45cdfdc ("9P: introduction of a new cache=mmap model.")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/9p/vfs_file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 3576123d8299..6d97b6b4d34b 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -612,9 +612,9 @@ static void v9fs_mmap_vm_close(struct vm_area_struct *vma)
 	struct writeback_control wbc = {
 		.nr_to_write = LONG_MAX,
 		.sync_mode = WB_SYNC_ALL,
-		.range_start = vma->vm_pgoff * PAGE_SIZE,
+		.range_start = (loff_t)vma->vm_pgoff * PAGE_SIZE,
 		 /* absolute end, byte at end included */
-		.range_end = vma->vm_pgoff * PAGE_SIZE +
+		.range_end = (loff_t)vma->vm_pgoff * PAGE_SIZE +
 			(vma->vm_end - vma->vm_start - 1),
 	};
 
-- 
2.28.0

[Ocfs2-devel] [PATCH 1/7] 9P: Cast to loff_t before multiplying

[Christoph Hellwig]

On Sun, Oct 04, 2020 at 07:04:22PM +0100, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this multiplication will overflow for files larger
> than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: fb89b45cdfdc ("9P: introduction of a new cache=mmap model.")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/9p/vfs_file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 3576123d8299..6d97b6b4d34b 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -612,9 +612,9 @@ static void v9fs_mmap_vm_close(struct vm_area_struct *vma)
>  	struct writeback_control wbc = {
>  		.nr_to_write = LONG_MAX,
>  		.sync_mode = WB_SYNC_ALL,
> -		.range_start = vma->vm_pgoff * PAGE_SIZE,
> +		.range_start = (loff_t)vma->vm_pgoff * PAGE_SIZE,

Given the may places where this issue shows up I think we really need
a vma_offset or similar helper for it.  Much better than chasing missing
casts everywhere.

[Ocfs2-devel] [PATCH 1/7] 9P: Cast to loff_t before multiplying

[Matthew Wilcox]

On Wed, Oct 07, 2020 at 06:48:49AM +0100, Christoph Hellwig wrote:
> > -		.range_start = vma->vm_pgoff * PAGE_SIZE,
> > +		.range_start = (loff_t)vma->vm_pgoff * PAGE_SIZE,
> 
> Given the may places where this issue shows up I think we really need
> a vma_offset or similar helper for it.  Much better than chasing missing
> casts everywhere.

Good point.  I think these patches need to go in to fix the bugs in
the various stable releases, but we should definitely have a helper
for the future.  Also, several of these patches are for non-VMA
pgoff_t.

vma_offset() is a bit weird for me -- vmas have all kinds of offsets.
vma_file_offset() would work or vma_fpos().  I tend to prefer the shorter
function name ;-)

A quick grep shows we probably want a vmf_fpos() too:

arch/powerpc/platforms/cell/spufs/file.c:       unsigned long area, offset = vmf->pgoff << PAGE_SHIFT;
arch/x86/entry/vdso/vma.c:      sym_offset = (long)(vmf->pgoff << PAGE_SHIFT) +
drivers/gpu/drm/gma500/framebuffer.c:   address = vmf->address - (vmf->pgoff << PAGE_SHIFT);
drivers/scsi/cxlflash/ocxl_hw.c:        offset = vmf->pgoff << PAGE_SHIFT;

I'm sure a lot of this will never run on a 32-bit kernel or with a 4GB
file, but it's not good to have bad code around for people to copy from.

[Ocfs2-devel] [PATCH 1/7] 9P: Cast to loff_t before multiplying

[Dominique Martinet]

Matthew Wilcox (Oracle) wrote on Sun, Oct 04, 2020:
> On 32-bit systems, this multiplication will overflow for files larger
> than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: fb89b45cdfdc ("9P: introduction of a new cache=mmap model.")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

I realize I hadn't sent a mail -- FWIW this 9p patch has been merged,
thanks!
-- 
Dominique

[Ocfs2-devel] [PATCH 2/7] buffer: Promote to unsigned long long before shifting

[Matthew Wilcox]

On 32-bit systems, this shift will overflow for files larger than 4GB.

Cc: stable at vger.kernel.org
Fixes: 5417169026c3 ("[FS] Implement block_page_mkwrite.")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 50bbc99e3d96..66f4765e60ee 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2515,7 +2515,7 @@ int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
 	}
 
 	/* page is wholly or partially inside EOF */
-	if (((page->index + 1) << PAGE_SHIFT) > size)
+	if (((page->index + 1ULL) << PAGE_SHIFT) > size)
 		end = size & ~PAGE_MASK;
 	else
 		end = PAGE_SIZE;
-- 
2.28.0

[Ocfs2-devel] [PATCH 3/7] ceph: Promote to unsigned long long before shifting

[Matthew Wilcox]

On 32-bit systems, this shift will overflow for files larger than 4GB.

Cc: stable at vger.kernel.org
Fixes: 61f68816211e ("ceph: check caps in filemap_fault and page_mkwrite")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/ceph/addr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 6ea761c84494..970e5a094035 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1522,7 +1522,7 @@ static vm_fault_t ceph_filemap_fault(struct vm_fault *vmf)
 	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_file_info *fi = vma->vm_file->private_data;
 	struct page *pinned_page = NULL;
-	loff_t off = vmf->pgoff << PAGE_SHIFT;
+	loff_t off = (loff_t)vmf->pgoff << PAGE_SHIFT;
 	int want, got, err;
 	sigset_t oldset;
 	vm_fault_t ret = VM_FAULT_SIGBUS;
-- 
2.28.0

[Ocfs2-devel] [PATCH 3/7] ceph: Promote to unsigned long long before shifting

[Jeff Layton]

On Sun, 2020-10-04 at 19:04 +0100, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this shift will overflow for files larger than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: 61f68816211e ("ceph: check caps in filemap_fault and page_mkwrite")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/ceph/addr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 6ea761c84494..970e5a094035 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -1522,7 +1522,7 @@ static vm_fault_t ceph_filemap_fault(struct vm_fault *vmf)
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  	struct ceph_file_info *fi = vma->vm_file->private_data;
>  	struct page *pinned_page = NULL;
> -	loff_t off = vmf->pgoff << PAGE_SHIFT;
> +	loff_t off = (loff_t)vmf->pgoff << PAGE_SHIFT;
>  	int want, got, err;
>  	sigset_t oldset;
>  	vm_fault_t ret = VM_FAULT_SIGBUS;

Good catch! Would you like us to take this in via the ceph tree, or are
you planning to submit altogether upstream? Either way:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

[Ocfs2-devel] [PATCH 3/7] ceph: Promote to unsigned long long before shifting

[Jeff Layton]

On Sun, 2020-10-04 at 19:04 +0100, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this shift will overflow for files larger than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: 61f68816211e ("ceph: check caps in filemap_fault and page_mkwrite")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/ceph/addr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 6ea761c84494..970e5a094035 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -1522,7 +1522,7 @@ static vm_fault_t ceph_filemap_fault(struct vm_fault *vmf)
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  	struct ceph_file_info *fi = vma->vm_file->private_data;
>  	struct page *pinned_page = NULL;
> -	loff_t off = vmf->pgoff << PAGE_SHIFT;
> +	loff_t off = (loff_t)vmf->pgoff << PAGE_SHIFT;
>  	int want, got, err;
>  	sigset_t oldset;
>  	vm_fault_t ret = VM_FAULT_SIGBUS;


I went ahead and merged this into the ceph-client/testing branch. Given
how old this bug is, I don't see a real need to rush this into v5.9, but
if we have any other patches going in before that ships, then it might
be good to send this one along too.
-- 
Jeff Layton <jlayton@kernel.org>

[Ocfs2-devel] [PATCH 4/7] ocfs2: Promote to unsigned long long before shifting

[Matthew Wilcox]

On 32-bit systems, this shift will overflow for files larger than 4GB.

Cc: stable at vger.kernel.org
Fixes: 35edec1d52c0 ("ocfs2: update truncate handling of partial clusters")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/ocfs2/alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 4c1b90442d6f..26eff79ecb50 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -6867,7 +6867,7 @@ static void ocfs2_zero_cluster_pages(struct inode *inode, loff_t start,
 		ocfs2_map_and_dirty_page(inode, handle, from, to, page, 1,
 					 &phys);
 
-		start = (page->index + 1) << PAGE_SHIFT;
+		start = (page->index + 1ULL) << PAGE_SHIFT;
 	}
 out:
 	if (pages)
-- 
2.28.0

[Ocfs2-devel] [PATCH 5/7] btrfs: Promote to unsigned long long before shifting

[Matthew Wilcox]

On 32-bit systems, this shift will overflow for files larger than 4GB.

Cc: stable at vger.kernel.org
Fixes: df480633b891 ("btrfs: extent-tree: Switch to new delalloc space reserve and release")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ac45f022b495..4d3b7e4ae53a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1277,7 +1277,7 @@ static int cluster_pages_for_defrag(struct inode *inode,
 	page_cnt = min_t(u64, (u64)num_pages, (u64)file_end - start_index + 1);
 
 	ret = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
-			start_index << PAGE_SHIFT,
+			(loff_t)start_index << PAGE_SHIFT,
 			page_cnt << PAGE_SHIFT);
 	if (ret)
 		return ret;
@@ -1367,7 +1367,7 @@ static int cluster_pages_for_defrag(struct inode *inode,
 		btrfs_mod_outstanding_extents(BTRFS_I(inode), 1);
 		spin_unlock(&BTRFS_I(inode)->lock);
 		btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved,
-				start_index << PAGE_SHIFT,
+				(loff_t)start_index << PAGE_SHIFT,
 				(page_cnt - i_done) << PAGE_SHIFT, true);
 	}
 
@@ -1395,7 +1395,7 @@ static int cluster_pages_for_defrag(struct inode *inode,
 		put_page(pages[i]);
 	}
 	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved,
-			start_index << PAGE_SHIFT,
+			(loff_t)start_index << PAGE_SHIFT,
 			page_cnt << PAGE_SHIFT, true);
 	btrfs_delalloc_release_extents(BTRFS_I(inode), page_cnt << PAGE_SHIFT);
 	extent_changeset_free(data_reserved);
-- 
2.28.0

[Ocfs2-devel] [PATCH 5/7] btrfs: Promote to unsigned long long before shifting

[Josef Bacik]

On 10/4/20 2:04 PM, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this shift will overflow for files larger than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: df480633b891 ("btrfs: extent-tree: Switch to new delalloc space reserve and release")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[Ocfs2-devel] [PATCH 5/7] btrfs: Promote to unsigned long long before shifting

[David Sterba]

On Sun, Oct 04, 2020 at 07:04:26PM +0100, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this shift will overflow for files larger than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: df480633b891 ("btrfs: extent-tree: Switch to new delalloc space reserve and release")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/btrfs/ioctl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ac45f022b495..4d3b7e4ae53a 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1277,7 +1277,7 @@ static int cluster_pages_for_defrag(struct inode *inode,
>  	page_cnt = min_t(u64, (u64)num_pages, (u64)file_end - start_index + 1);
>  
>  	ret = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
> -			start_index << PAGE_SHIFT,
> +			(loff_t)start_index << PAGE_SHIFT,

> -				start_index << PAGE_SHIFT,
> +				(loff_t)start_index << PAGE_SHIFT,

> -			start_index << PAGE_SHIFT,
> +			(loff_t)start_index << PAGE_SHIFT,

As this repeats 3 times I've added a variable. Patch added to misc-next,
thanks.

[Ocfs2-devel] [PATCH 6/7] btrfs: Promote to unsigned long long before shifting

[Matthew Wilcox]

On 32-bit systems, this shift will overflow for files larger than 4GB.

Cc: stable at vger.kernel.org
Fixes: 53b381b3abeb ("Btrfs: RAID5 and RAID6")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/raid56.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 255490f42b5d..5ee0a53301bd 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1089,7 +1089,7 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
 	u64 disk_start;
 
 	stripe = &rbio->bbio->stripes[stripe_nr];
-	disk_start = stripe->physical + (page_index << PAGE_SHIFT);
+	disk_start = stripe->physical + ((loff_t)page_index << PAGE_SHIFT);
 
 	/* if the device is missing, just fail this stripe */
 	if (!stripe->dev->bdev)
-- 
2.28.0

[Ocfs2-devel] [PATCH 6/7] btrfs: Promote to unsigned long long before shifting

[Josef Bacik]

On 10/4/20 2:04 PM, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this shift will overflow for files larger than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: 53b381b3abeb ("Btrfs: RAID5 and RAID6")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[Ocfs2-devel] [PATCH 6/7] btrfs: Promote to unsigned long long before shifting

[David Sterba]

On Sun, Oct 04, 2020 at 07:04:27PM +0100, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, this shift will overflow for files larger than 4GB.
> 
> Cc: stable at vger.kernel.org
> Fixes: 53b381b3abeb ("Btrfs: RAID5 and RAID6")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/btrfs/raid56.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 255490f42b5d..5ee0a53301bd 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1089,7 +1089,7 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
>  	u64 disk_start;
>  
>  	stripe = &rbio->bbio->stripes[stripe_nr];
> -	disk_start = stripe->physical + (page_index << PAGE_SHIFT);
> +	disk_start = stripe->physical + ((loff_t)page_index << PAGE_SHIFT);

It seems that this patch is mechanical replacement. If you check the
callers, the page_index is passed from an int that iterates over bits
set in an unsigned long (bitmap). The result won't overflow.

[Ocfs2-devel] [PATCH 6/7] btrfs: Promote to unsigned long long before shifting

[Matthew Wilcox]

On Mon, Oct 26, 2020 at 05:35:46PM +0100, David Sterba wrote:
> On Sun, Oct 04, 2020 at 07:04:27PM +0100, Matthew Wilcox (Oracle) wrote:
> > On 32-bit systems, this shift will overflow for files larger than 4GB.
> > 
> > Cc: stable at vger.kernel.org
> > Fixes: 53b381b3abeb ("Btrfs: RAID5 and RAID6")
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  fs/btrfs/raid56.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> > index 255490f42b5d..5ee0a53301bd 100644
> > --- a/fs/btrfs/raid56.c
> > +++ b/fs/btrfs/raid56.c
> > @@ -1089,7 +1089,7 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
> >  	u64 disk_start;
> >  
> >  	stripe = &rbio->bbio->stripes[stripe_nr];
> > -	disk_start = stripe->physical + (page_index << PAGE_SHIFT);
> > +	disk_start = stripe->physical + ((loff_t)page_index << PAGE_SHIFT);
> 
> It seems that this patch is mechanical replacement. If you check the
> callers, the page_index is passed from an int that iterates over bits
> set in an unsigned long (bitmap). The result won't overflow.

Not mechanical, but I clearly made mistakes.  Will you pick up the
patches which actually fix bugs?

[Ocfs2-devel] [PATCH 6/7] btrfs: Promote to unsigned long long before shifting

[David Sterba]

On Mon, Oct 26, 2020 at 04:44:42PM +0000, Matthew Wilcox wrote:
> On Mon, Oct 26, 2020 at 05:35:46PM +0100, David Sterba wrote:
> > On Sun, Oct 04, 2020 at 07:04:27PM +0100, Matthew Wilcox (Oracle) wrote:
> > > On 32-bit systems, this shift will overflow for files larger than 4GB.
> > > 
> > > Cc: stable at vger.kernel.org
> > > Fixes: 53b381b3abeb ("Btrfs: RAID5 and RAID6")
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > ---
> > >  fs/btrfs/raid56.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> > > index 255490f42b5d..5ee0a53301bd 100644
> > > --- a/fs/btrfs/raid56.c
> > > +++ b/fs/btrfs/raid56.c
> > > @@ -1089,7 +1089,7 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
> > >  	u64 disk_start;
> > >  
> > >  	stripe = &rbio->bbio->stripes[stripe_nr];
> > > -	disk_start = stripe->physical + (page_index << PAGE_SHIFT);
> > > +	disk_start = stripe->physical + ((loff_t)page_index << PAGE_SHIFT);
> > 
> > It seems that this patch is mechanical replacement. If you check the
> > callers, the page_index is passed from an int that iterates over bits
> > set in an unsigned long (bitmap). The result won't overflow.
> 
> Not mechanical, but I clearly made mistakes.  Will you pick up the
> patches which actually fix bugs?

Yes, I just replied to the first patch, that does fix an overflow.

[Ocfs2-devel] [PATCH 7/7] btrfs: Promote to unsigned long long before multiplying

[Matthew Wilcox]

On 32-bit systems, these shifts will overflow for files larger than 4GB.
Add helper functions to avoid this problem coming back.

Cc: stable at vger.kernel.org
Fixes: 73ff61dbe5ed ("Btrfs: fix device replace of a missing RAID 5/6 device")
Fixes: be50a8ddaae1 ("Btrfs: Simplify scrub_setup_recheck_block()'s argument")
Fixes: ff023aac3119 ("Btrfs: add code to scrub to copy read data to another disk")
Fixes: b5d67f64f9bc ("Btrfs: change scrub to support big blocks")
Fixes: a2de733c78fa ("btrfs: scrub")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/scrub.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 354ab9985a34..ccbaf9c6e87a 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1262,12 +1262,17 @@ static inline void scrub_stripe_index_and_offset(u64 logical, u64 map_type,
 	}
 }
 
+static u64 sblock_length(struct scrub_block *sblock)
+{
+	return (u64)sblock->page_count * PAGE_SIZE;
+}
+
 static int scrub_setup_recheck_block(struct scrub_block *original_sblock,
 				     struct scrub_block *sblocks_for_recheck)
 {
 	struct scrub_ctx *sctx = original_sblock->sctx;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	u64 length = original_sblock->page_count * PAGE_SIZE;
+	u64 length = sblock_length(original_sblock);
 	u64 logical = original_sblock->pagev[0]->logical;
 	u64 generation = original_sblock->pagev[0]->generation;
 	u64 flags = original_sblock->pagev[0]->flags;
@@ -1610,6 +1615,11 @@ static void scrub_write_block_to_dev_replace(struct scrub_block *sblock)
 	}
 }
 
+static u64 sbio_length(struct scrub_bio *sbio)
+{
+	return (u64)sbio->page_count * PAGE_SIZE;
+}
+
 static int scrub_write_page_to_dev_replace(struct scrub_block *sblock,
 					   int page_num)
 {
@@ -1659,10 +1669,9 @@ static int scrub_add_page_to_wr_bio(struct scrub_ctx *sctx,
 		bio->bi_iter.bi_sector = sbio->physical >> 9;
 		bio->bi_opf = REQ_OP_WRITE;
 		sbio->status = 0;
-	} else if (sbio->physical + sbio->page_count * PAGE_SIZE !=
+	} else if (sbio->physical + sbio_length(sbio) !=
 		   spage->physical_for_dev_replace ||
-		   sbio->logical + sbio->page_count * PAGE_SIZE !=
-		   spage->logical) {
+		   sbio->logical + sbio_length(sbio) != spage->logical) {
 		scrub_wr_submit(sctx);
 		goto again;
 	}
@@ -2005,10 +2014,8 @@ static int scrub_add_page_to_rd_bio(struct scrub_ctx *sctx,
 		bio->bi_iter.bi_sector = sbio->physical >> 9;
 		bio->bi_opf = REQ_OP_READ;
 		sbio->status = 0;
-	} else if (sbio->physical + sbio->page_count * PAGE_SIZE !=
-		   spage->physical ||
-		   sbio->logical + sbio->page_count * PAGE_SIZE !=
-		   spage->logical ||
+	} else if (sbio->physical + sbio_length(sbio) != spage->physical ||
+		   sbio->logical + sbio_length(sbio) != spage->logical ||
 		   sbio->dev != spage->dev) {
 		scrub_submit(sctx);
 		goto again;
@@ -2094,7 +2101,7 @@ static void scrub_missing_raid56_pages(struct scrub_block *sblock)
 {
 	struct scrub_ctx *sctx = sblock->sctx;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	u64 length = sblock->page_count * PAGE_SIZE;
+	u64 length = sblock_length(sblock);
 	u64 logical = sblock->pagev[0]->logical;
 	struct btrfs_bio *bbio = NULL;
 	struct bio *bio;
-- 
2.28.0

[Ocfs2-devel] [PATCH 7/7] btrfs: Promote to unsigned long long before multiplying

[David Sterba]

On Sun, Oct 04, 2020 at 07:04:28PM +0100, Matthew Wilcox (Oracle) wrote:
> On 32-bit systems, these shifts will overflow for files larger than 4GB.
> Add helper functions to avoid this problem coming back.
> 
> Cc: stable at vger.kernel.org
> Fixes: 73ff61dbe5ed ("Btrfs: fix device replace of a missing RAID 5/6 device")
> Fixes: be50a8ddaae1 ("Btrfs: Simplify scrub_setup_recheck_block()'s argument")
> Fixes: ff023aac3119 ("Btrfs: add code to scrub to copy read data to another disk")
> Fixes: b5d67f64f9bc ("Btrfs: change scrub to support big blocks")
> Fixes: a2de733c78fa ("btrfs: scrub")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/btrfs/scrub.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 354ab9985a34..ccbaf9c6e87a 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -1262,12 +1262,17 @@ static inline void scrub_stripe_index_and_offset(u64 logical, u64 map_type,
>  	}
>  }
>  
> +static u64 sblock_length(struct scrub_block *sblock)
> +{
> +	return (u64)sblock->page_count * PAGE_SIZE;

page_count will be 32 at most, the type is int and this will never
overflow. The value is usualy number of pages in the arrays scrub_bio::pagev or
scrub_block::pagev bounded by SCRUB_PAGES_PER_WR_BIO (32) or
SCRUB_MAX_PAGES_PER_BLOCK (16).  The scrub code does not use mappings
and it reads raw blocks to own pages and does the checksum verification.