Support Clang context analysis for ext2

This description is mostly copied from v1:

This series adds annotations for Clang's context analysis to ext2. Clang context analysis was recently added in a series by Marco Elver [1]. This allows the compiler to validate different locking patterns at compile time.

This series enables context analysis, fixes pre-existing warnings, and adds new annotations. It is inspired by similar series in the block layer (NVMe host driver, for example [2]).

I'm starting with ext2 since it's smaller and simpler compared to ext4/btrfs/etc. After ext2, I'd be interested in converting the other filesystems and infrastructure code in fs/. I think the ultimate goal would be to enable this by default across all of fs/.

The series was built and tested with Clang 23 with CONFIG_WARN_CONTEXT_ANALYSIS enabled. I based on 7.2-rc7.

Thanks!

Changes from v1:

* A false positive has been fixed in Clang [3] and ported to Clang 23. Hence, the first patch (silencing that false positive) has been dropped.
* Use guard() and scoped_guard() instead of context_unsafe(), when possible, to express that certain fields are being protected by a newly initialized lock during init.

Link to v1: https://lore.kernel.org/linux-fsdevel/[email protected]/

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://github.com/llvm/llvm-project/pull/209796

Timothy Day (8):
ext2: mark s_next_generation as guarded by s_next_gen_lock
ext2: annotate ext2_update_dynamic_rev() as requiring s_lock
ext2: mark statfs overhead cache as guarded by s_lock
ext2: mark s_mount_state as guarded by s_lock
ext2: annotate ext2_init_block_alloc_info() as requiring
truncate_mutex
ext2: annotate block-mapping helpers as requiring truncate_mutex
ext2: annotate s_rsv_window_root as requiring s_rsv_window_lock
ext2: enable context analysis support for ext2 filesystem

 fs/ext2/Makefile |  2 ++
 fs/ext2/balloc.c |  4 ++++
 fs/ext2/ext2.h   | 19 +++++++++++--------
 fs/ext2/inode.c  |  7 +++++++
 fs/ext2/super.c  | 35 +++++++++++++++++++----------------
 5 files changed, 43 insertions(+), 24 deletions(-)

--
2.43.0

This description is mostly copied from v1:

This series adds annotations for Clang's context analysis to ext2. Clang context analysis was recently added in a series by Marco Elver [1]. This allows the compiler to validate different locking patterns at compile time.

This series enables context analysis, fixes pre-existing warnings, and adds new annotations. It is inspired by similar series in the block layer (NVMe host driver, for example [2]).

I'm starting with ext2 since it's smaller and simpler compared to ext4/btrfs/etc. After ext2, I'd be interested in converting the other filesystems and infrastructure code in fs/. I think the ultimate goal would be to enable this by default across all of fs/.

The series was built and tested with Clang 23 with CONFIG_WARN_CONTEXT_ANALYSIS enabled. I based on 7.2-rc7.

Quoted 11 lines · Timothy Day

This description is mostly copied from v1:

This series adds annotations for Clang's context analysis to ext2. Clang context analysis was recently added in a series by Marco Elver [1]. This allows the compiler to validate different locking patterns at compile time.

This series enables context analysis, fixes pre-existing warnings, and adds new annotations. It is inspired by similar series in the block layer (NVMe host driver, for example [2]).

Quoted 7 lines

I'm starting with ext2 since it's smaller and simpler compared to ext4/btrfs/etc. After ext2, I'd be interested in converting the other filesystems and infrastructure code in fs/. I think the ultimate goal would be to enable this by default across all of fs/.

The series was built and tested with Clang 23 with CONFIG_WARN_CONTEXT_ANALYSIS enabled. I based on 7.2-rc7.

Thanks for the patches! They look good to me. Once the merge window is over I'll queue them to my tree. The only thing I'm not fully sure is how much I like the spinlock_init scoped guards - they looked quite confusing to me at the first sight (as much as I understand the convenience, conceptually how can initialization of a global lock be scoped?). I'll sleep over it, maybe I'll change them to just spinlock_init() + scoped_guard for the lock itself or maybe I'll get used to them. Anyway, no action on your side needed :).

Quoted 20 lines · Jan Kara

Quoted 11 lines · Timothy Day

This description is mostly copied from v1:

This series adds annotations for Clang's context analysis to ext2. Clang context analysis was recently added in a series by Marco Elver [1]. This allows the compiler to validate different locking patterns at compile time.

This series enables context analysis, fixes pre-existing warnings, and adds new annotations. It is inspired by similar series in the block layer (NVMe host driver, for example [2]).

Quoted 7 lines

I'm starting with ext2 since it's smaller and simpler compared to ext4/btrfs/etc. After ext2, I'd be interested in converting the other filesystems and infrastructure code in fs/. I think the ultimate goal would be to enable this by default across all of fs/.

The series was built and tested with Clang 23 with CONFIG_WARN_CONTEXT_ANALYSIS enabled. I based on 7.2-rc7.

Quoted 7 lines

Thanks for the patches! They look good to me. Once the merge window is over I'll queue them to my tree. The only thing I'm not fully sure is how much I like the spinlock_init scoped guards - they looked quite confusing to me at the first sight (as much as I understand the convenience, conceptually how can initialization of a global lock be scoped?). I'll sleep over it, maybe I'll change them to just spinlock_init() + scoped_guard for the lock itself or maybe I'll get used to them. Anyway, no action on your side needed :).

This series is now in -next, where I see the following warnings (or errors with CONFIG_WERROR=y / W=e) with various configurations, such as ARCH=arm allmodconfig, when building with LLVM 23.1.0

fs/ext2/xattr.c:825:6: error: rw_semaphore 'EXT2_I().xattr_sem' is not held on every path through here [-Werror,-Wthread-safety-analysis]

    825 |         if (WARN_ON_ONCE(!down_write_trylock(&EXT2_I(inode)->xattr_sem)))
        |             ^
  include/asm-generic/bug.h:180:2: note: expanded from macro 'WARN_ON_ONCE'
    180 |         DO_ONCE_LITE_IF(condition, WARN_ON, 1)
        |         ^
  include/linux/once_lite.h:30:7: note: expanded from macro 'DO_ONCE_LITE_IF'
     30 |                 if (__ONCE_LITE_IF(__ret_do_once))                      \
        |                     ^

include/linux/once_lite.h:23:3: note: expanded from macro '__ONCE_LITE_IF'

     23 |                 unlikely(__ret_once);                                   \
        |                 ^
  include/linux/compiler.h:77:22: note: expanded from macro 'unlikely'
     77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
        |                         ^
  fs/ext2/xattr.c:825:20: note: rw_semaphore acquired here
    825 |         if (WARN_ON_ONCE(!down_write_trylock(&EXT2_I(inode)->xattr_sem)))
        |                           ^

fs/ext2/super.c:1149:3: error: calling function 'ext2_rsv_window_add' requires holding spinlock 'EXT2_SB(sb).s_rsv_window_lock' exclusively [-Werror,-Wthread-safety-precise]

   1149 |                 ext2_rsv_window_add(sb, &sbi->s_rsv_window_head);
        |                 ^

fs/ext2/super.c:1149:3: note: found near match '_res->s_rsv_window_lock'

Quoted 4 lines · Nathan Chancellor

fs/ext2/super.c:1149:3: error: calling function 'ext2_rsv_window_add' requires holding spinlock 'EXT2_SB(sb).s_rsv_window_lock' exclusively [-Werror,-Wthread-safety-precise]

   1149 |                 ext2_rsv_window_add(sb, &sbi->s_rsv_window_head);
        |                 ^

fs/ext2/super.c:1149:3: note: found near match '_res->s_rsv_window_lock'

sbi was just allocated, and EXT2_SB(sb) and sbi are pointing to the same object (unless I misread the code), but the compiler can't tell since aliases can't be tracked through non-local objects. So that scoped_guard could just become:

  scoped_guard(spinlock, &EXT2_SB(sb)->s_rsv_window_lock) {
  ...

But I'll leave that to Jan and Tim.

Quoted 11 lines · Marco Elver

Quoted 4 lines · Nathan Chancellor

fs/ext2/super.c:1149:3: error: calling function 'ext2_rsv_window_add' requires holding spinlock 'EXT2_SB(sb).s_rsv_window_lock' exclusively [-Werror,-Wthread-safety-precise]

   1149 |                 ext2_rsv_window_add(sb, &sbi->s_rsv_window_head);
        |                 ^

fs/ext2/super.c:1149:3: note: found near match '_res->s_rsv_window_lock'

sbi was just allocated, and EXT2_SB(sb) and sbi are pointing to the same object (unless I misread the code), but the compiler can't tell since aliases can't be tracked through non-local objects. So that scoped_guard could just become:

Quoted 4 lines

  scoped_guard(spinlock, &EXT2_SB(sb)->s_rsv_window_lock) {
  ...

But I'll leave that to Jan and Tim.

Yes, at the beginning of ext4_fill_super() we do:

        sbi = kzalloc_obj(*sbi);
        ...
        sb->s_fs_info = sbi;

and EXT2_SB(sb) is just sb->s_fs_info. Are you saying that clang is not able to infer that sb->s_fs_info and sbi are still pointing to the same memory later in the function where we do scoped_guard()?

Quoted 20 lines · Jan Kara

Quoted 11 lines · Marco Elver

Quoted 4 lines · Nathan Chancellor

fs/ext2/super.c:1149:3: error: calling function 'ext2_rsv_window_add' requires holding spinlock 'EXT2_SB(sb).s_rsv_window_lock' exclusively [-Werror,-Wthread-safety-precise]

   1149 |                 ext2_rsv_window_add(sb, &sbi->s_rsv_window_head);
        |                 ^

fs/ext2/super.c:1149:3: note: found near match '_res->s_rsv_window_lock'

sbi was just allocated, and EXT2_SB(sb) and sbi are pointing to the same object (unless I misread the code), but the compiler can't tell since aliases can't be tracked through non-local objects. So that scoped_guard could just become:

Quoted 4 lines

  scoped_guard(spinlock, &EXT2_SB(sb)->s_rsv_window_lock) {
  ...

But I'll leave that to Jan and Tim.

Yes, at the beginning of ext4_fill_super() we do:

Quoted 7 lines

        sbi = kzalloc_obj(*sbi);
        ...
        sb->s_fs_info = sbi;

and EXT2_SB(sb) is just sb->s_fs_info. Are you saying that clang is not able to infer that sb->s_fs_info and sbi are still pointing to the same memory later in the function where we do scoped_guard()?

Yes - alias tracking is only done through local aliases. sb is non-local, along with additional member indirection; unfortunately, the C language doesn't give us the guarantees that it wasn't modified somewhere in between, say after a function call (this rule is applied also for local aliases if clang sees that they "escape" their local scope via non-const pointer to pointer).

Quoted 29 lines · Marco Elver

Quoted 20 lines · Jan Kara

Quoted 11 lines · Marco Elver

Quoted 4 lines · Nathan Chancellor

fs/ext2/super.c:1149:3: error: calling function 'ext2_rsv_window_add' requires holding spinlock 'EXT2_SB(sb).s_rsv_window_lock' exclusively [-Werror,-Wthread-safety-precise]

   1149 |                 ext2_rsv_window_add(sb, &sbi->s_rsv_window_head);
        |                 ^

fs/ext2/super.c:1149:3: note: found near match '_res->s_rsv_window_lock'

sbi was just allocated, and EXT2_SB(sb) and sbi are pointing to the same object (unless I misread the code), but the compiler can't tell since aliases can't be tracked through non-local objects. So that scoped_guard could just become:

Quoted 4 lines

  scoped_guard(spinlock, &EXT2_SB(sb)->s_rsv_window_lock) {
  ...

But I'll leave that to Jan and Tim.

Yes, at the beginning of ext4_fill_super() we do:

Quoted 7 lines

        sbi = kzalloc_obj(*sbi);
        ...
        sb->s_fs_info = sbi;

and EXT2_SB(sb) is just sb->s_fs_info. Are you saying that clang is not able to infer that sb->s_fs_info and sbi are still pointing to the same memory later in the function where we do scoped_guard()?

Quoted 6 lines

Yes - alias tracking is only done through local aliases. sb is non-local, along with additional member indirection; unfortunately, the C language doesn't give us the guarantees that it wasn't modified somewhere in between, say after a function call (this rule is applied also for local aliases if clang sees that they "escape" their local scope via non-const pointer to pointer).

Hrm, ok, understood (but still it's annoying ;)). I've pushed out the patch with the suggested fixup.