Re: [PATCH 3/7] btrfs: accessors: inline eb bounds check and factor out the error report

[David Sterba <dsterba@suse.cz>]

On Wed, Jul 02, 2025 at 11:00:55AM -0700, Boris Burkov wrote:
> > @@ -56,7 +51,10 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
> >  	const int part = eb->folio_size - oil;				\
> >  	u8 lebytes[sizeof(u##bits)];					\
> >  									\
> > -	ASSERT(check_setget_bounds(eb, ptr, off, sizeof(u##bits)));	\
> > +	if (unlikely(member_offset + sizeof(u##bits) > eb->len)) {	\
> > +		report_setget_bounds(eb, ptr, off, sizeof(u##bits));	\
> 
> For full no-change compatibility would it make sense to also ASSERT
> here? (or stuff it in report, these are the only two users)

I'm not claiming no-change here and it's implied by the changelog that
the behaviour and outcome depend on CONFIG_BTRFS_ASSEERT which I wanted
to unify.

I see the point that it should keep the assert here when things go
wrong, during development. However, the type of errors it would catch is
quite rare. Most btree items are accessed by the set/get helpers with
fixed sizes, or the read_extent_buffer/write_extent_buffer with variable
length (and they have their own bounds checks and reports).

I can add an assert or rather a debug warning to the report function so
we get a noisy report. The reason I'd like use DEBUG_WARN for that is
that we get a noisy report but don't crash so we can also observe what
happens if the function returns a this is what would happen with
non-assert configs.

For future plans, this error case will also somehow mark the filesystem
as "we have a serious problem" and let it shut down. Qu has some work
for that pending so I'll connect the two once it's done.

My idea, before the shutdown works, was to set a filesystem bit and
detect it at transaction commit to abort early. The reason is that the
set/get helpers are used everywhere without error checking, so it would
be more sensible to catch that in some selected points.

> > @@ -76,7 +74,10 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
> >  	const int part = eb->folio_size - oil;				\
> >  	u8 lebytes[sizeof(u##bits)];					\
> >  									\
> > -	ASSERT(check_setget_bounds(eb, ptr, off, sizeof(u##bits)));	\
> > +	if (unlikely(member_offset + sizeof(u##bits) > eb->len)) {	\
> > +		report_setget_bounds(eb, ptr, off, sizeof(u##bits));	\
> > +		return;							\
> > +	}								\
> 
> Would a helper macro to reduce this duplication be useful? Seems
> borderline but worth mentioning. Next time you improve it you won't
> have to hit two spots.

I don't see much reason for that, the definitions are next to each
other and once the token versions got removed the code it's basically on
one screen, hard to miss the other location.