[bug report] btrfs: refactor __btrfs_run_delayed_refs loop

[Dan Carpenter <error27@gmail.com>]

[ 8 year old code and no one has complained...  It's surprising no
  one has hit this in real life. - dan ]

Hello Nikolay Borisov,

Commit 0110a4c43451 ("btrfs: refactor __btrfs_run_delayed_refs loop")
from Aug 15, 2018 (linux-next), leads to the following Smatch static
checker warning:

	fs/btrfs/extent-tree.c:2131 __btrfs_run_delayed_refs()
	error: 'locked_ref' dereferencing possible ERR_PTR()

fs/btrfs/extent-tree.c
    2084 static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
    2085                                              u64 min_bytes)
    2086 {
    2087         struct btrfs_fs_info *fs_info = trans->fs_info;
    2088         struct btrfs_delayed_ref_root *delayed_refs;
    2089         struct btrfs_delayed_ref_head *locked_ref = NULL;
    2090         int ret;
    2091         unsigned long count = 0;
    2092         unsigned long max_count = 0;
    2093         u64 bytes_processed = 0;
    2094 
    2095         delayed_refs = &trans->transaction->delayed_refs;
    2096         if (min_bytes == 0) {
    2097                 /*
    2098                  * We may be subject to a harmless race if some task is
    2099                  * concurrently adding or removing a delayed ref, so silence
    2100                  * KCSAN and similar tools.
    2101                  */
    2102                 max_count = data_race(delayed_refs->num_heads_ready);
    2103                 min_bytes = U64_MAX;
    2104         }
    2105 
    2106         do {
    2107                 if (!locked_ref) {
    2108                         locked_ref = btrfs_select_ref_head(fs_info, delayed_refs);
    2109                         if (IS_ERR_OR_NULL(locked_ref)) {
    2110                                 if (PTR_ERR(locked_ref) == -EAGAIN) {
    2111                                         continue;

In the old we set "locked_ref = NULL" on this path and I think we
probably should restore that line.  Now it looks like a crash.

    2112                                 } else {
    2113                                         break;
    2114                                 }
    2115                         }
    2116                         count++;
    2117                 }
    2118                 /*
    2119                  * We need to try and merge add/drops of the same ref since we
    2120                  * can run into issues with relocate dropping the implicit ref
    2121                  * and then it being added back again before the drop can
    2122                  * finish.  If we merged anything we need to re-loop so we can
    2123                  * get a good ref.
    2124                  * Or we can get node references of the same type that weren't
    2125                  * merged when created due to bumps in the tree mod seq, and
    2126                  * we need to merge them to prevent adding an inline extent
    2127                  * backref before dropping it (triggering a BUG_ON at
    2128                  * insert_inline_extent_backref()).
    2129                  */
    2130                 spin_lock(&locked_ref->lock);
                                    ^^^^^^^^^^^^^^^^
On the next iteration &locked_ref will be ERR_PTR(-EAGAIN).

--> 2131                 btrfs_merge_delayed_refs(fs_info, delayed_refs, locked_ref);
    2132 
    2133                 ret = btrfs_run_delayed_refs_for_head(trans, locked_ref, &bytes_processed);
    2134                 if (ret < 0 && ret != -EAGAIN) {
    2135                         /*
    2136                          * Error, btrfs_run_delayed_refs_for_head already
    2137                          * unlocked everything so just bail out
    2138                          */
    2139                         return ret;
    2140                 } else if (!ret) {
    2141                         /*
    2142                          * Success, perform the usual cleanup of a processed
    2143                          * head
    2144                          */
    2145                         ret = cleanup_ref_head(trans, locked_ref, &bytes_processed);
    2146                         if (ret > 0 ) {
    2147                                 /* We dropped our lock, we need to loop. */
    2148                                 ret = 0;
    2149                                 continue;
    2150                         } else if (ret) {
    2151                                 return ret;
    2152                         }
    2153                 }
    2154 
    2155                 /*
    2156                  * Either success case or btrfs_run_delayed_refs_for_head
    2157                  * returned -EAGAIN, meaning we need to select another head
    2158                  */
    2159 
    2160                 locked_ref = NULL;
    2161                 cond_resched();
    2162         } while ((min_bytes != U64_MAX && bytes_processed < min_bytes) ||
    2163                  (max_count > 0 && count < max_count) ||
    2164                  locked_ref);
                          ^^^^^^^^^^
We continue down to here and loop back since locked_ref is -EAGAIN.

    2165 
    2166         return 0;
    2167 }

This email is a free service from the Smatch-CI project [smatch.sf.net].

regards,
dan carpenter