On Thu, Sep 14, 2023 at 03:51:44PM +0200, Josh Poimboeuf wrote:
> On Wed, Sep 13, 2023 at 06:01:42PM -0700, Kees Cook wrote:
> > > +++ b/fs/bcachefs/buckets.h
> > > @@ -180,7 +180,7 @@ static inline u64 bch2_dev_buckets_reserved(struct bch_dev *ca, enum bch_waterma
> > >
> > > switch (watermark) {
> > > case BCH_WATERMARK_NR:
> > > - unreachable();
> > > + BUG();
> >
> > Linus gets really upset about new BUG() usage (takes out the entire
> > system):
> > https://docs.kernel.org/process/deprecated.html#bug-and-bug-on
> >
> > It'd be nicer to actually handle the impossible case. (WARN and return
> > 0?)
>
> Sure, see below.
Looks good to me; thanks!
-Kees
>
> BTW, I'm about to go off grid for 1.5 weeks, so there will be no v3
> coming from me anytime soon :-)
>
> ---8<---
>
> From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> Subject: [PATCH v2] bcachefs: Remove undefined behavior in bch2_dev_buckets_reserved()
>
> In general it's a good idea to avoid using bare unreachable() because it
> introduces undefined behavior in compiled code. In this case it even
> confuses GCC into emitting an empty unused
> bch2_dev_buckets_reserved.part.0() function.
>
> Use WARN_ON(1) instead, which is nice and defined. While in theory it
> should never trigger, if something were to go awry and the
> BCH_WATERMARK_NR case were to actually hit, the failure mode is more
> robust.
>
> Fixes the following warnings:
>
> vmlinux.o: warning: objtool: bch2_bucket_alloc_trans() falls through to next function bch2_reset_alloc_cursors()
> vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
>
> Reported-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
> fs/bcachefs/buckets.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
> index f192809f50cf..211f054bf83d 100644
> --- a/fs/bcachefs/buckets.h
> +++ b/fs/bcachefs/buckets.h
> @@ -180,7 +180,8 @@ static inline u64 bch2_dev_buckets_reserved(struct bch_dev *ca, enum bch_waterma
>
> switch (watermark) {
> case BCH_WATERMARK_NR:
> - unreachable();
> + WARN_ON(1);
> + break;
> case BCH_WATERMARK_stripe:
> reserved += ca->mi.nbuckets >> 6;
> fallthrough;
> --
> 2.41.0
>
--
Kees Cook
