| Message ID | 20240716090105.72179-1-r.smirnov@omp.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: buffer: set the expression type to unsigned long in folio_create_buffers() | expand |
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
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.
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. >
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; }
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(-)