| Message ID | 20240812103619.2720-2-thorsten.blum@toblux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Annotate structs with __counted_by() | expand |
On 12.08.24 12:37, Thorsten Blum wrote: > Add the __counted_by compiler attribute to the flexible array member > stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > CONFIG_FORTIFY_SOURCE. > > Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> > --- > fs/btrfs/volumes.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 37a09ebb34dd..f28fa318036b 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -551,7 +551,7 @@ struct btrfs_io_context { > * stripes[data_stripes + 1]: The Q stripe (only for RAID6). > */ > u64 full_stripe_logical; > - struct btrfs_io_stripe stripes[]; > + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); > }; > > struct btrfs_device_info { > @@ -591,7 +591,7 @@ struct btrfs_chunk_map { > int io_width; > int num_stripes; > int sub_stripes; > - struct btrfs_io_stripe stripes[]; > + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); > }; > > #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \ Looks good to me, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Out of curiosity, have you encountered any issues with this patch applied?
On Mon, Aug 12, 2024 at 12:36:20PM +0200, Thorsten Blum wrote: > Add the __counted_by compiler attribute to the flexible array member > stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > CONFIG_FORTIFY_SOURCE. > > Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> Reviewed-by: David Sterba <dsterba@suse.com>
On 12. Aug 2024, at 12:54, Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > On 12.08.24 12:37, Thorsten Blum wrote: >> Add the __counted_by compiler attribute to the flexible array member >> stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and >> CONFIG_FORTIFY_SOURCE. >> >> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> >> --- >> fs/btrfs/volumes.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >> index 37a09ebb34dd..f28fa318036b 100644 >> --- a/fs/btrfs/volumes.h >> +++ b/fs/btrfs/volumes.h >> @@ -551,7 +551,7 @@ struct btrfs_io_context { >> * stripes[data_stripes + 1]: The Q stripe (only for RAID6). >> */ >> u64 full_stripe_logical; >> - struct btrfs_io_stripe stripes[]; >> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); >> }; >> >> struct btrfs_device_info { >> @@ -591,7 +591,7 @@ struct btrfs_chunk_map { >> int io_width; >> int num_stripes; >> int sub_stripes; >> - struct btrfs_io_stripe stripes[]; >> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); >> }; >> >> #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \ > > Looks good to me, > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Out of curiosity, have you encountered any issues with this patch applied? I only compile-tested it.
On Mon, Aug 12, 2024 at 02:03:44PM +0200, Thorsten Blum wrote: > On 12. Aug 2024, at 12:54, Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > > On 12.08.24 12:37, Thorsten Blum wrote: > >> Add the __counted_by compiler attribute to the flexible array member > >> stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > >> CONFIG_FORTIFY_SOURCE. > >> > >> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> > >> --- > >> fs/btrfs/volumes.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > >> index 37a09ebb34dd..f28fa318036b 100644 > >> --- a/fs/btrfs/volumes.h > >> +++ b/fs/btrfs/volumes.h > >> @@ -551,7 +551,7 @@ struct btrfs_io_context { > >> * stripes[data_stripes + 1]: The Q stripe (only for RAID6). > >> */ > >> u64 full_stripe_logical; > >> - struct btrfs_io_stripe stripes[]; > >> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); > >> }; > >> > >> struct btrfs_device_info { > >> @@ -591,7 +591,7 @@ struct btrfs_chunk_map { > >> int io_width; > >> int num_stripes; > >> int sub_stripes; > >> - struct btrfs_io_stripe stripes[]; > >> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); > >> }; > >> > >> #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \ > > > > Looks good to me, > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > > > Out of curiosity, have you encountered any issues with this patch applied? > > I only compile-tested it. This change is now in next-20240814 and I see a UBSAN warning at runtime as a result because the assignment of ->num_stripes happens after accessing ->stripes[] (which breaks one of the requirements for using __counted_by [1]), meaning that UBSAN thinks this is a zero sized array due to bioc being allocated with kzalloc(). [ 24.992264] ------------[ cut here ]------------ [ 25.009196] UBSAN: array-index-out-of-bounds in fs/btrfs/volumes.c:6602:11 [ 25.021963] index 1 is out of range for type 'struct btrfs_io_stripe[] __counted_by(num_stripes)' (aka 'struct btrfs_io_stripe[]') [ 25.036463] CPU: 28 UID: 0 PID: 1171 Comm: systemd-random- Not tainted 6.11.0-rc3-next-20240814 #1 [ 25.048172] Hardware name: ADLINK Ampere Altra Developer Platform/Ampere Altra Developer Platform, BIOS TianoCore 2.04.100.11 (SYS: 2.06.20220308) 11/06/2 [ 25.064754] Call trace: [ 25.069965] dump_backtrace+0x114/0x19c [ 25.076564] show_stack+0x28/0x3c [ 25.082642] dump_stack_lvl+0x48/0x94 [ 25.089068] __ubsan_handle_out_of_bounds+0x10c/0x184 [ 25.096883] btrfs_map_block+0x540/0xb3c [ 25.103570] btrfs_submit_bio+0xf8/0x654 [ 25.110256] write_one_eb+0x290/0x444 [ 25.116682] btree_write_cache_pages+0x44c/0x5a8 [ 25.124063] btree_writepages+0x2c/0x8c [ 25.130662] do_writepages+0x10c/0x34c [ 25.137175] filemap_fdatawrite_wbc+0x84/0xb0 [ 25.144295] filemap_fdatawrite_range+0x74/0xac [ 25.151589] btrfs_write_marked_extents+0xa0/0x140 [ 25.159143] btrfs_sync_log+0x298/0xa98 [ 25.165743] btrfs_sync_file+0x440/0x608 [ 25.172429] __arm64_sys_fsync+0x90/0xd4 [ 25.179115] invoke_syscall+0x8c/0x11c [ 25.185628] el0_svc_common [ 25.191185] do_el0_svc+0x2c/0x3c [ 25.197264] el0_svc+0x48/0xf0 [ 25.203083] el0t_64_sync_handler+0x98/0x108 [ 25.210118] el0t_64_sync+0x19c/0x1a0 [ 25.216552] ---[ end trace ]--- The fix might be as simple as something like diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 4a259bdaa21c..0cabc2ebde71 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -6561,6 +6561,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op, } bioc->map_type = map->type; + bioc->num_stripes = io_geom.num_stripes; /* * For RAID56 full map, we need to make sure the stripes[] follows the * rule that data stripes are all ordered, then followed with P and Q @@ -6621,7 +6622,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op, } *bioc_ret = bioc; - bioc->num_stripes = io_geom.num_stripes; bioc->max_errors = io_geom.max_errors; bioc->mirror_num = io_geom.mirror_num; but I am not sure of the implications of this change on quick glance with regards to error handling and such. [1]: https://people.kernel.org/gustavoars/how-to-use-the-new-counted_by-attribute-in-c-and-linux Cheers, Nathan
On 14. Aug 2024, at 20:02, Nathan Chancellor <nathan@kernel.org> wrote: > On Mon, Aug 12, 2024 at 02:03:44PM +0200, Thorsten Blum wrote: >> On 12. Aug 2024, at 12:54, Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: >>> On 12.08.24 12:37, Thorsten Blum wrote: >>>> Add the __counted_by compiler attribute to the flexible array member >>>> stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and >>>> CONFIG_FORTIFY_SOURCE. >>>> >>>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> >>>> --- >>>> fs/btrfs/volumes.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >>>> index 37a09ebb34dd..f28fa318036b 100644 >>>> --- a/fs/btrfs/volumes.h >>>> +++ b/fs/btrfs/volumes.h >>>> @@ -551,7 +551,7 @@ struct btrfs_io_context { >>>> * stripes[data_stripes + 1]: The Q stripe (only for RAID6). >>>> */ >>>> u64 full_stripe_logical; >>>> - struct btrfs_io_stripe stripes[]; >>>> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); >>>> }; >>>> >>>> struct btrfs_device_info { >>>> @@ -591,7 +591,7 @@ struct btrfs_chunk_map { >>>> int io_width; >>>> int num_stripes; >>>> int sub_stripes; >>>> - struct btrfs_io_stripe stripes[]; >>>> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); >>>> }; >>>> >>>> #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \ >>> >>> Looks good to me, >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> >>> Out of curiosity, have you encountered any issues with this patch applied? >> >> I only compile-tested it. > > This change is now in next-20240814 and I see a UBSAN warning at runtime > as a result because the assignment of ->num_stripes happens after > accessing ->stripes[] (which breaks one of the requirements for using > __counted_by [1]), meaning that UBSAN thinks this is a zero sized array > due to bioc being allocated with kzalloc(). > > [ 24.992264] ------------[ cut here ]------------ > [ 25.009196] UBSAN: array-index-out-of-bounds in fs/btrfs/volumes.c:6602:11 > [ 25.021963] index 1 is out of range for type 'struct btrfs_io_stripe[] __counted_by(num_stripes)' (aka 'struct btrfs_io_stripe[]') > [ 25.036463] CPU: 28 UID: 0 PID: 1171 Comm: systemd-random- Not tainted 6.11.0-rc3-next-20240814 #1 > [ 25.048172] Hardware name: ADLINK Ampere Altra Developer Platform/Ampere Altra Developer Platform, BIOS TianoCore 2.04.100.11 (SYS: 2.06.20220308) 11/06/2 > [ 25.064754] Call trace: > [ 25.069965] dump_backtrace+0x114/0x19c > [ 25.076564] show_stack+0x28/0x3c > [ 25.082642] dump_stack_lvl+0x48/0x94 > [ 25.089068] __ubsan_handle_out_of_bounds+0x10c/0x184 > [ 25.096883] btrfs_map_block+0x540/0xb3c > [ 25.103570] btrfs_submit_bio+0xf8/0x654 > [ 25.110256] write_one_eb+0x290/0x444 > [ 25.116682] btree_write_cache_pages+0x44c/0x5a8 > [ 25.124063] btree_writepages+0x2c/0x8c > [ 25.130662] do_writepages+0x10c/0x34c > [ 25.137175] filemap_fdatawrite_wbc+0x84/0xb0 > [ 25.144295] filemap_fdatawrite_range+0x74/0xac > [ 25.151589] btrfs_write_marked_extents+0xa0/0x140 > [ 25.159143] btrfs_sync_log+0x298/0xa98 > [ 25.165743] btrfs_sync_file+0x440/0x608 > [ 25.172429] __arm64_sys_fsync+0x90/0xd4 > [ 25.179115] invoke_syscall+0x8c/0x11c > [ 25.185628] el0_svc_common > [ 25.191185] do_el0_svc+0x2c/0x3c > [ 25.197264] el0_svc+0x48/0xf0 > [ 25.203083] el0t_64_sync_handler+0x98/0x108 > [ 25.210118] el0t_64_sync+0x19c/0x1a0 > [ 25.216552] ---[ end trace ]--- > > The fix might be as simple as something like > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 4a259bdaa21c..0cabc2ebde71 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -6561,6 +6561,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op, > } > bioc->map_type = map->type; > > + bioc->num_stripes = io_geom.num_stripes; > /* > * For RAID56 full map, we need to make sure the stripes[] follows the > * rule that data stripes are all ordered, then followed with P and Q > @@ -6621,7 +6622,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op, > } > > *bioc_ret = bioc; > - bioc->num_stripes = io_geom.num_stripes; > bioc->max_errors = io_geom.max_errors; > bioc->mirror_num = io_geom.mirror_num; > > > but I am not sure of the implications of this change on quick glance > with regards to error handling and such. > > [1]: https://people.kernel.org/gustavoars/how-to-use-the-new-counted_by-attribute-in-c-and-linux > > Cheers, > Nathan My patch should probably be reverted as I somehow missed quite a few things and need more time to rework this properly. Sorry about that and thanks for reporting this!
On Wed, Aug 14, 2024 at 09:01:42PM +0200, Thorsten Blum wrote: > On 14. Aug 2024, at 20:02, Nathan Chancellor <nathan@kernel.org> wrote: > > On Mon, Aug 12, 2024 at 02:03:44PM +0200, Thorsten Blum wrote: > >> On 12. Aug 2024, at 12:54, Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > >>> On 12.08.24 12:37, Thorsten Blum wrote: > >>>> Add the __counted_by compiler attribute to the flexible array member > >>>> stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > >>>> CONFIG_FORTIFY_SOURCE. > >>>> > >>>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> > >>>> --- > >>>> fs/btrfs/volumes.h | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > >>>> index 37a09ebb34dd..f28fa318036b 100644 > >>>> --- a/fs/btrfs/volumes.h > >>>> +++ b/fs/btrfs/volumes.h > >>>> @@ -551,7 +551,7 @@ struct btrfs_io_context { > >>>> * stripes[data_stripes + 1]: The Q stripe (only for RAID6). > >>>> */ > >>>> u64 full_stripe_logical; > >>>> - struct btrfs_io_stripe stripes[]; > >>>> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); > >>>> }; > >>>> > >>>> struct btrfs_device_info { > >>>> @@ -591,7 +591,7 @@ struct btrfs_chunk_map { > >>>> int io_width; > >>>> int num_stripes; > >>>> int sub_stripes; > >>>> - struct btrfs_io_stripe stripes[]; > >>>> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); > >>>> }; > >>>> > >>>> #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \ > >>> > >>> Looks good to me, > >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > >>> > >>> Out of curiosity, have you encountered any issues with this patch applied? > >> > >> I only compile-tested it. > > > > This change is now in next-20240814 and I see a UBSAN warning at runtime > > as a result because the assignment of ->num_stripes happens after > > accessing ->stripes[] (which breaks one of the requirements for using > > __counted_by [1]), meaning that UBSAN thinks this is a zero sized array > > due to bioc being allocated with kzalloc(). > > > > [ 24.992264] ------------[ cut here ]------------ > > [ 25.009196] UBSAN: array-index-out-of-bounds in fs/btrfs/volumes.c:6602:11 > > [ 25.021963] index 1 is out of range for type 'struct btrfs_io_stripe[] __counted_by(num_stripes)' (aka 'struct btrfs_io_stripe[]') > > [ 25.036463] CPU: 28 UID: 0 PID: 1171 Comm: systemd-random- Not tainted 6.11.0-rc3-next-20240814 #1 > > [ 25.048172] Hardware name: ADLINK Ampere Altra Developer Platform/Ampere Altra Developer Platform, BIOS TianoCore 2.04.100.11 (SYS: 2.06.20220308) 11/06/2 > > [ 25.064754] Call trace: > > [ 25.069965] dump_backtrace+0x114/0x19c > > [ 25.076564] show_stack+0x28/0x3c > > [ 25.082642] dump_stack_lvl+0x48/0x94 > > [ 25.089068] __ubsan_handle_out_of_bounds+0x10c/0x184 > > [ 25.096883] btrfs_map_block+0x540/0xb3c > > [ 25.103570] btrfs_submit_bio+0xf8/0x654 > > [ 25.110256] write_one_eb+0x290/0x444 > > [ 25.116682] btree_write_cache_pages+0x44c/0x5a8 > > [ 25.124063] btree_writepages+0x2c/0x8c > > [ 25.130662] do_writepages+0x10c/0x34c > > [ 25.137175] filemap_fdatawrite_wbc+0x84/0xb0 > > [ 25.144295] filemap_fdatawrite_range+0x74/0xac > > [ 25.151589] btrfs_write_marked_extents+0xa0/0x140 > > [ 25.159143] btrfs_sync_log+0x298/0xa98 > > [ 25.165743] btrfs_sync_file+0x440/0x608 > > [ 25.172429] __arm64_sys_fsync+0x90/0xd4 > > [ 25.179115] invoke_syscall+0x8c/0x11c > > [ 25.185628] el0_svc_common > > [ 25.191185] do_el0_svc+0x2c/0x3c > > [ 25.197264] el0_svc+0x48/0xf0 > > [ 25.203083] el0t_64_sync_handler+0x98/0x108 > > [ 25.210118] el0t_64_sync+0x19c/0x1a0 > > [ 25.216552] ---[ end trace ]--- > > > > The fix might be as simple as something like > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 4a259bdaa21c..0cabc2ebde71 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -6561,6 +6561,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op, > > } > > bioc->map_type = map->type; > > > > + bioc->num_stripes = io_geom.num_stripes; > > /* > > * For RAID56 full map, we need to make sure the stripes[] follows the > > * rule that data stripes are all ordered, then followed with P and Q > > @@ -6621,7 +6622,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op, > > } > > > > *bioc_ret = bioc; > > - bioc->num_stripes = io_geom.num_stripes; > > bioc->max_errors = io_geom.max_errors; > > bioc->mirror_num = io_geom.mirror_num; > > > > > > but I am not sure of the implications of this change on quick glance > > with regards to error handling and such. > > > > [1]: https://people.kernel.org/gustavoars/how-to-use-the-new-counted_by-attribute-in-c-and-linux > > > > Cheers, > > Nathan > > My patch should probably be reverted as I somehow missed quite a few > things and need more time to rework this properly. > > Sorry about that and thanks for reporting this! Patch removed from for-next.
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 37a09ebb34dd..f28fa318036b 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -551,7 +551,7 @@ struct btrfs_io_context { * stripes[data_stripes + 1]: The Q stripe (only for RAID6). */ u64 full_stripe_logical; - struct btrfs_io_stripe stripes[]; + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); }; struct btrfs_device_info { @@ -591,7 +591,7 @@ struct btrfs_chunk_map { int io_width; int num_stripes; int sub_stripes; - struct btrfs_io_stripe stripes[]; + struct btrfs_io_stripe stripes[] __counted_by(num_stripes); }; #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \
Add the __counted_by compiler attribute to the flexible array member stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com> --- fs/btrfs/volumes.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)