fs: buffer: set the expression type to unsigned long in folio_create_buffers()

[PATCH] fs: buffer: set the expression type to unsigned long in folio_create_buffers()

Posted by Roman Smirnov 1 year, 6 months ago

Shift without specifying the type casts the expression to int,
which is then passed as an unsigned long argument. It is necessary
to use 1UL instead.

Found by Linux Verification Center (linuxtesting.org) with Svace.

Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 8c19e705b9c3..40dc18f1cba5 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1782,7 +1782,7 @@ static struct buffer_head *folio_create_buffers(struct folio *folio,
 	bh = folio_buffers(folio);
 	if (!bh)
 		bh = create_empty_buffers(folio,
-				1 << READ_ONCE(inode->i_blkbits), b_state);
+				1UL << READ_ONCE(inode->i_blkbits), b_state);
 	return bh;
 }
 
-- 
2.34.1

Re: [PATCH] fs: buffer: set the expression type to unsigned long in folio_create_buffers()

Posted by Sergey Shtylyov 1 year, 6 months ago

On 7/16/24 12:01 PM, Roman Smirnov wrote:

> Shift without specifying the type casts the expression to int,

   You mean the result of the shift? Or what expression?

> which is then passed as an unsigned long argument. It is necessary

   And here we'll have at least one potential problem (that you neglected
to describe): with 1 << 31, that 1 will land in a sign bit and then, when
it's implicitly cast to *unsigned long*, the 32-bit value will be sign-
extended to 64-bit on 64-bit arches) and then we'll have an incorrect size
(0xffffffff80000000) passed to create_empty_buffers()...

> to use 1UL instead.

   Perphas was worth noting that using 1UL saves us 1 movsx instruction on
x86_64...

> Found by Linux Verification Center (linuxtesting.org) with Svace.
> 
> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
[...]

MBR, Sergey

Re: [PATCH] fs: buffer: set the expression type to unsigned long in folio_create_buffers()

Posted by Matthew Wilcox 1 year, 6 months ago

On Tue, Jul 16, 2024 at 06:41:49PM +0300, Sergey Shtylyov wrote:
>    And here we'll have at least one potential problem (that you neglected
> to describe): with 1 << 31, that 1 will land in a sign bit and then, when
> it's implicitly cast to *unsigned long*, the 32-bit value will be sign-
> extended to 64-bit on 64-bit arches) and then we'll have an incorrect size
> (0xffffffff80000000) passed to create_empty_buffers()...

Tell me more about this block device you have with a 2GB block size ... ?

(ie note that this is a purely theoretical issue)

> > to use 1UL instead.
> 
>    Perphas was worth noting that using 1UL saves us 1 movsx instruction on
> x86_64...

That is a worthwhile addition to the change log.

Also, you should cc the person who wrote that code, ie me.

Re: [PATCH] fs: buffer: set the expression type to unsigned long in folio_create_buffers()

Posted by Jan Kara 1 year, 6 months ago

On Tue 16-07-24 16:51:17, Matthew Wilcox wrote:
> On Tue, Jul 16, 2024 at 06:41:49PM +0300, Sergey Shtylyov wrote:
> >    And here we'll have at least one potential problem (that you neglected
> > to describe): with 1 << 31, that 1 will land in a sign bit and then, when
> > it's implicitly cast to *unsigned long*, the 32-bit value will be sign-
> > extended to 64-bit on 64-bit arches) and then we'll have an incorrect size
> > (0xffffffff80000000) passed to create_empty_buffers()...
> 
> Tell me more about this block device you have with a 2GB block size ... ?
> 
> (ie note that this is a purely theoretical issue)

Yeah, this just does not make huge amount of sense. Maybe a proper fix
would be to just make blocksize uint? There are a lot of places where
blocksize is actually stored in a 32-bit type...

								Honza

> 
> > > to use 1UL instead.
> > 
> >    Perphas was worth noting that using 1UL saves us 1 movsx instruction on
> > x86_64...
> 
> That is a worthwhile addition to the change log.
> 
> Also, you should cc the person who wrote that code, ie me.
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR