On 9/11/23 22:26, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20230911:
>
> New tree: bcachefs
>
> The bcachefs tree gained a semantic conflict against Linus' tree for
> which I applied a patch.
>
> The wireless-next tree gaind a conflict against the wireless tree.
>
> Non-merge commits (relative to Linus' tree): 4095
> 1552 files changed, 346893 insertions(+), 22945 deletions(-)
>
> ----------------------------------------------------------------------------
on x86_64:
vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
Full randconfig file is attached.
--
~Randy
On Tue, Sep 12, 2023 at 04:36:55PM -0700, Randy Dunlap wrote:
>
>
> On 9/11/23 22:26, Stephen Rothwell wrote:
> > Hi all,
> >
> > Changes since 20230911:
> >
> > New tree: bcachefs
> >
> > The bcachefs tree gained a semantic conflict against Linus' tree for
> > which I applied a patch.
> >
> > The wireless-next tree gaind a conflict against the wireless tree.
> >
> > Non-merge commits (relative to Linus' tree): 4095
> > 1552 files changed, 346893 insertions(+), 22945 deletions(-)
> >
> > ----------------------------------------------------------------------------
>
> on x86_64:
>
> vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
Here ya go:
---8<---
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] 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 BUG() 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 much 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 <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
fs/bcachefs/buckets.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
index f192809f50cf..0eff05c79c65 100644
--- a/fs/bcachefs/buckets.h
+++ 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();
case BCH_WATERMARK_stripe:
reserved += ca->mi.nbuckets >> 6;
fallthrough;
--
2.41.0
On 9/13/23 14:08, Josh Poimboeuf wrote:
> On Tue, Sep 12, 2023 at 04:36:55PM -0700, Randy Dunlap wrote:
>>
>>
>> On 9/11/23 22:26, Stephen Rothwell wrote:
>>> Hi all,
>>>
>>> Changes since 20230911:
>>>
>>> New tree: bcachefs
>>>
>>> The bcachefs tree gained a semantic conflict against Linus' tree for
>>> which I applied a patch.
>>>
>>> The wireless-next tree gaind a conflict against the wireless tree.
>>>
>>> Non-merge commits (relative to Linus' tree): 4095
>>> 1552 files changed, 346893 insertions(+), 22945 deletions(-)
>>>
>>> ----------------------------------------------------------------------------
>>
>> on x86_64:
>>
>> vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
>
> Here ya go:
>
> ---8<---
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] 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 BUG() 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 much 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 <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Tested-by: Randy Dunlap <[email protected]>
Reviewed-by: Randy Dunlap <[email protected]>
Thanks, Josh.
> ---
> fs/bcachefs/buckets.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
> index f192809f50cf..0eff05c79c65 100644
> --- a/fs/bcachefs/buckets.h
> +++ 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();
> case BCH_WATERMARK_stripe:
> reserved += ca->mi.nbuckets >> 6;
> fallthrough;
--
~Randy
On Wed, Sep 13, 2023 at 11:08:29PM +0200, Josh Poimboeuf wrote:
> On Tue, Sep 12, 2023 at 04:36:55PM -0700, Randy Dunlap wrote:
> >
> >
> > On 9/11/23 22:26, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > Changes since 20230911:
> > >
> > > New tree: bcachefs
> > >
> > > The bcachefs tree gained a semantic conflict against Linus' tree for
> > > which I applied a patch.
> > >
> > > The wireless-next tree gaind a conflict against the wireless tree.
> > >
> > > Non-merge commits (relative to Linus' tree): 4095
> > > 1552 files changed, 346893 insertions(+), 22945 deletions(-)
> > >
> > > ----------------------------------------------------------------------------
> >
> > on x86_64:
> >
> > vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
>
> Here ya go:
>
> ---8<---
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] 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 BUG() 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 much 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 <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> fs/bcachefs/buckets.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
> index f192809f50cf..0eff05c79c65 100644
> --- a/fs/bcachefs/buckets.h
> +++ 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?)
-Kees
> case BCH_WATERMARK_stripe:
> reserved += ca->mi.nbuckets >> 6;
> fallthrough;
> --
> 2.41.0
>
--
Kees Cook
On Wed, Sep 13, 2023 at 11:08:29PM +0200, Josh Poimboeuf wrote:
> On Tue, Sep 12, 2023 at 04:36:55PM -0700, Randy Dunlap wrote:
> >
> >
> > On 9/11/23 22:26, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > Changes since 20230911:
> > >
> > > New tree: bcachefs
> > >
> > > The bcachefs tree gained a semantic conflict against Linus' tree for
> > > which I applied a patch.
> > >
> > > The wireless-next tree gaind a conflict against the wireless tree.
> > >
> > > Non-merge commits (relative to Linus' tree): 4095
> > > 1552 files changed, 346893 insertions(+), 22945 deletions(-)
> > >
> > > ----------------------------------------------------------------------------
> >
> > on x86_64:
> >
> > vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
>
> Here ya go:
>
> ---8<---
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] 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 BUG() 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 much more robust.
Thanks, want to do the other two cases too? :)
>
> 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 <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> fs/bcachefs/buckets.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
> index f192809f50cf..0eff05c79c65 100644
> --- a/fs/bcachefs/buckets.h
> +++ 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();
> case BCH_WATERMARK_stripe:
> reserved += ca->mi.nbuckets >> 6;
> fallthrough;
> --
> 2.41.0
>
On Wed, Sep 13, 2023 at 07:06:26PM -0400, Kent Overstreet wrote:
> On Wed, Sep 13, 2023 at 11:08:29PM +0200, Josh Poimboeuf wrote:
> > On Tue, Sep 12, 2023 at 04:36:55PM -0700, Randy Dunlap wrote:
> > >
> > >
> > > On 9/11/23 22:26, Stephen Rothwell wrote:
> > > > Hi all,
> > > >
> > > > Changes since 20230911:
> > > >
> > > > New tree: bcachefs
> > > >
> > > > The bcachefs tree gained a semantic conflict against Linus' tree for
> > > > which I applied a patch.
> > > >
> > > > The wireless-next tree gaind a conflict against the wireless tree.
> > > >
> > > > Non-merge commits (relative to Linus' tree): 4095
> > > > 1552 files changed, 346893 insertions(+), 22945 deletions(-)
> > > >
> > > > ----------------------------------------------------------------------------
> > >
> > > on x86_64:
> > >
> > > vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
> >
> > Here ya go:
> >
> > ---8<---
> >
> > From: Josh Poimboeuf <[email protected]>
> > Subject: [PATCH] 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 BUG() 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 much more robust.
>
> Thanks, want to do the other two cases too? :)
Hm, which cases are you referring to?
--
Josh
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.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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
On Wed, Sep 13, 2023 at 06:01:42PM -0700, Kees Cook wrote:
> On Wed, Sep 13, 2023 at 11:08:29PM +0200, Josh Poimboeuf wrote:
> > On Tue, Sep 12, 2023 at 04:36:55PM -0700, Randy Dunlap wrote:
> > >
> > >
> > > On 9/11/23 22:26, Stephen Rothwell wrote:
> > > > Hi all,
> > > >
> > > > Changes since 20230911:
> > > >
> > > > New tree: bcachefs
> > > >
> > > > The bcachefs tree gained a semantic conflict against Linus' tree for
> > > > which I applied a patch.
> > > >
> > > > The wireless-next tree gaind a conflict against the wireless tree.
> > > >
> > > > Non-merge commits (relative to Linus' tree): 4095
> > > > 1552 files changed, 346893 insertions(+), 22945 deletions(-)
> > > >
> > > > ----------------------------------------------------------------------------
> > >
> > > on x86_64:
> > >
> > > vmlinux.o: warning: objtool: bch2_dev_buckets_reserved.part.0() is missing an ELF size annotation
> >
> > Here ya go:
> >
> > ---8<---
> >
> > From: Josh Poimboeuf <[email protected]>
> > Subject: [PATCH] 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 BUG() 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 much 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 <[email protected]>
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > ---
> > fs/bcachefs/buckets.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
> > index f192809f50cf..0eff05c79c65 100644
> > --- a/fs/bcachefs/buckets.h
> > +++ 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?)
No, I'm not going to be doing that.
These are truly impossible cases, i.e. if we were writing code in a
language with embedded correctness proofs this is something the compiler
would be checking and proving.
Changing all the BUG()s to WARNS() and writing error paths would mean
etiher a shit ton of error paths that never get tested, or deleting a
lot of a assertions, and I'm not doing either of those things.
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> 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