Re: [PATCH v6 3/4] ntfs: protect attribute-list creation/teardown with attr_list_persist_lock

> +       /* Keep ALE removal and the follow-up need check in one transaction. */
> +       if (!attrlist_locked && type != AT_ATTRIBUTE_LIST &&
> +           NInoAttrList(base_ni)) {
> +               mutex_lock(&base_ni->attr_list_persist_lock);
> +               attrlist_locked = true;
> +       }
> +
>         /* Remove attribute itself. */
>         if (ntfs_attr_record_resize(ctx->mrec, ctx->attr, 0)) {
>                 ntfs_debug("Couldn't remove attribute record. Bug or damaged MFT record.\n");
> -               return -EIO;
> +               err = -EIO;
> +               goto out_unlock;
>         }
>         mark_mft_record_dirty(ni);
>
> @@ -2837,19 +2849,42 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>          * delete $ATTRIBUTE_LIST itself.
>          */
>         if (NInoAttrList(base_ni) && type != AT_ATTRIBUTE_LIST) {
> -               err = ntfs_attrlist_entry_rm(ctx);
> +               err = ntfs_attrlist_entry_rm_locked(ctx);
The first NInoAttrList() test does not prevent an attribute list from
being created immediately after the test.  In that case this function
later observes the new list and calls ntfs_attrlist_entry_rm_locked()
without holding attr_list_persist_lock.

For example:

  Thread A: ntfs_attr_record_rm()          Thread B: ntfs_inode_add_attrlist()
  --------------------------------------   -----------------------------------

  1. Checks NInoAttrList(base_ni)
     -> false
     -> does not take persist lock

                                            2. Takes persist lock
                                               Builds and publishes attr_list
                                               Sets NI_AttrList

  3. Removes the attribute record

  4. Checks NInoAttrList(base_ni)
     -> now true

  5. Calls ntfs_attrlist_entry_rm_locked()
     without holding attr_list_persist_lock

This violates the _locked() helper's contract and will trigger its lockdep
assertion.  It can also race with Thread B while Thread B is constructing and
persisting a list that may still contain the attribute being removed.

For every non-$ATTRIBUTE_LIST removal, the function should acquire
attr_list_persist_lock whenever persist_locked is false, regardless of the
initial NInoAttrList() value.  This serializes removal against both an existing
attribute list and concurrent attribute-list creation.

>         /* Free MFT record, if it doesn't contain attributes. */
> @@ -2857,17 +2892,18 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>                         le16_to_cpu(ctx->mrec->attrs_offset) == 8) {
>                 if (ntfs_mft_record_free(ni->vol, ni)) {
>                         ntfs_debug("Couldn't free MFT record.\n");
> -                       return -EIO;
> +                       err = -EIO;
> +                       goto out_unlock;
>                 }
>                 /* Remove done if we freed base inode. */
>                 if (ni == base_ni)
> -                       return 0;
> +                       goto out_unlock;
>                 ntfs_inode_close(ni);
>                 ctx->ntfs_ino = ni = NULL;
>         }
>
>         if (type == AT_ATTRIBUTE_LIST || !NInoAttrList(base_ni))
> -               return 0;
> +               goto out_unlock;
>
>         /* Remove attribute list if we don't need it any more. */
>         if (!ntfs_attrlist_need(base_ni)) {
> @@ -2878,7 +2914,7 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>                 if (ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, CASE_SENSITIVE,
>                                         0, NULL, 0, ctx)) {
>                         ntfs_debug("Couldn't find attribute list. Succeed anyway.\n");
> -                       return 0;
> +                       goto out_unlock;
>                 }
>                 /* Deallocate clusters. */
>                 if (ctx->attr->non_resident) {
> @@ -2889,16 +2925,16 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>                                         ctx->attr, NULL, &new_rl_count);
>                         if (IS_ERR(al_rl)) {
>                                 ntfs_debug("Couldn't decompress attribute list runlist. Succeed anyway.\n");
> -                               return 0;
> +                               goto out_unlock;
>                         }
>                         if (ntfs_cluster_free_from_rl(base_ni->vol, al_rl))
>                                 ntfs_debug("Leaking clusters! Run chkdsk. Couldn't free clusters from attribute list runlist.\n");
>                         kvfree(al_rl);
>                 }
>                 /* Remove attribute record itself. */
> -               if (ntfs_attr_record_rm(ctx)) {
> +               if (ntfs_attr_record_rm(ctx, true)) {
>                         ntfs_debug("Couldn't remove attribute list. Succeed anyway.\n");
> -                       return 0;
> +                       goto out_unlock;
>                 }
>
>                 na.mft_no = VFS_I(base_ni)->i_ino;
> @@ -2914,7 +2950,10 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>                 }
>
>         }
> -       return 0;
> +out_unlock:
> +       if (attrlist_locked && !persist_locked)
> +               mutex_unlock(&base_ni->attr_list_persist_lock);
> +       return err;
Several old `return 0` statements were converted to goto out_unlock,
but err is not initialized on those successful paths.

> +       /* Serialize the initial state check and attribute-list publication. */
> +       mutex_lock(&ni->attr_list_persist_lock);
> +       attrlist_locked = true;
> +
>         if (NInoAttrList(ni) || ni->nr_extents) {
>                 ntfs_error(ni->vol->sb, "Inode already has attribute list");
> +               mutex_unlock(&ni->attr_list_persist_lock);
>                 return -EEXIST;
>         }
The newly acquired attr_list_persist_lock is not released on the
existing early error paths.

  mutex_lock(attr_list_persist_lock)
          |
          +-- map_mft_record() fails
          |      -> return -EIO
          |         lock remains held
          |
          +-- search-context allocation fails
          |      -> goto err_out
          |         lock remains held
          |
          +-- attribute enumeration or kvrealloc() fails
                 -> goto put_err_out
                    lock remains held