linux-kernel - Re: [GIT PULL] bcachefs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=whaiVhuO7W1tb8Yb-CuUHWn7bBnJ3bM7bvcQiEQwv_WrQ@mail.gmail.com>
Date:   Wed, 6 Sep 2023 13:02:32 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kent Overstreet <kent.overstreet@...ux.dev>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-bcachefs@...r.kernel.org
Subject: Re: [GIT PULL] bcachefs

On Wed, 6 Sept 2023 at 12:36, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> There are a few other issues that I have with this, and Christoph did
> mention a big one: it's not been in linux-next.

Oh, and the fact that it hasn't been in linux-next became immediately
clear, and it's a real practical problem.

I get a compile error when just doing a basic built-test:

  fs/bcachefs/btree_key_cache.c: In function ‘btree_key_cache_create’:
  fs/bcachefs/btree_key_cache.c:356:56: error: implicit conversion
from ‘enum btree_node_locked_type’ to ‘enum six_lock_type’
[-Werror=enum-conversion]
    356 |                 mark_btree_node_locked(trans, path, 0,
BTREE_NODE_UNLOCKED);
        |
^~~~~~~~~~~~~~~~~~~

and that compile error is actually a sign of a pretty serious bug.

BTREE_NODE_UNLOCKED is of the wrong enum type, and has a value (-1)
that simply cannot be represented in a 'enum six_lock_type'.

I'm pretty sure that the compiler is actually allowed to assume it is
in the range of the source enum.

Yes, the inline function then does a cast to 'enum
btree_node_locked_type', but because the compiler may assume that the
incoming enum has values 0..2, it's not clear that it will necessarily
cast the value -1 to the expected value.

It probably works in practice on x86 (without -fshort-enum, I think
gcc will always use an 'int'), but that code really is wrong. On other
platforms, gcc defaults -fshort-enums on, and in that case a 'enum
six_lock_type' is actually of underlying type 'unsigned char'.

And guess what happens when you have (unsigned char)-1? It does *not*
cast back to -1.

I don't know whether any of the architectures we support has that
"default to -fshort-enums" behaviour, and at least hexagon uses
"-fno-short-enums" to avoid this issue, but it really is a serious
bug.

A Clang build also results in objtool complaints about bcachefs, with

  fs/bcachefs/buckets.o: warning: objtool:
bch2_trans_mark_metadata_bucket() falls through to next function
bch2_trans_mark_dev_sb()
  fs/bcachefs/fsck.o: warning: objtool: write_inode() falls through to
next function hash_check_key()

and sure enough, the code generation ends up being

          movq    %rbx, %rdi
          movl    %r12d, %esi
          callq   bch2_trans_restart_error
  .Lfunc_end25:
          .size   write_inode, .Lfunc_end25-write_inode

and the issue is that bch2_trans_restart_error() is marked __noreturn,
but objtool hasn't been told about it, so objtool is very unhappy
about that "code seems to fall off the side of the earth" kind of
issue.

Both of these are very much signs of "this has never been tested in
other environments", and would presumably have been found immediately
in linux-next.

As it is, I'm not convinced I want to even continue to go through this
all, when it fails at the very first hurdle. A clean build is not some
"would be nice" feature, and it is big part of why we have automation
for new code.

                     Linus